Re: [Maria-developers] Please review MDEV-8468 MDEV-8466
Hi, Alexander! On Jul 21, Alexander Barkov wrote:
I have one question about "HANDLER FOR SQLWARNING". It does not catch messages of level "Note". Looks like a bug. Please confirm, I'll file a report if so.
Hmm, I don't know. According to the standard, SQLWARNING should match sqlstates in the category W, while SQLEXCEPTION should be for sqlstates in the category X. But we don't do that, 22007 is an exception (category X), but we treat it as a warning. So, the standard doesn't help to answer your question. We seem to use the logic where a level "error" means an SQL exception and a level "warning" means an SQL warning. Using this logic, "notes" are neither exceptions nor warnings, and neither SQLEXCEPTION nor SQLWARNING should match them. I'd say it's not a bug. The patch itself looks great, these changes in behavior are very welcomed. Just one small comment regarding THD::check_edom_and_truncation(), see below.
diff --git a/sql/sql_class.h b/sql/sql_class.h index a8d8444..c19174b 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3270,6 +3270,53 @@ class THD :public Statement, (!transaction.stmt.modified_non_trans_table || (variables.sql_mode & MODE_STRICT_ALL_TABLES))); } + /** + Check string-to-number conversion and produce a warning if + - could not convert any digits (EDOM-alike error) + - found garbage at the end of the string + See also Field_num::check_edom_and_truncation() for a similar function. + + @param thd - the thread + @param type - name of the data type (e.g. "INTEGER", "DECIMAL", "DOUBLE") + @param edom - of a EDOM-alike error happened + @param cs - character set of the original string + @param str - the original string + @param end_of_num - the position where the string-to-number routine stopped. + @param end - the end of the string + + Unlike Field_num::check_edom_and_truncation(), this function does not + distinguish between EDOM and truncation and reports the same warning for + both cases. Perhaps we should eventually print different warnings, to make + the explicit CAST work closer to the implicit cast in Field_xxx::store().
Why haven't you done it now? It causes too many changes in tests?
+ */ + void check_edom_and_truncation(const char *type, bool edom, + CHARSET_INFO *cs, + const char *str, + const char *end_of_num, + const char *end)
Move this out of THD please. You don't need to call current_thd every time for it, but only when a warning should be issued.
+ { + DBUG_ASSERT(str <= end_of_num); + DBUG_ASSERT(end_of_num <= end); + if (edom || + (end_of_num < end && !check_if_only_end_space(cs, end_of_num, end))) + { + /* + We can use err.ptr() here as ErrConvString is guranteed to put an + end \0 here. + */ + push_warning_printf(this, Sql_condition::WARN_LEVEL_WARN, + ER_TRUNCATED_WRONG_VALUE, + ER(ER_TRUNCATED_WRONG_VALUE), type, + ErrConvString(str, end - str, cs).ptr()); + } + else if (end_of_num < end) + { + push_warning_printf(this, Sql_condition::WARN_LEVEL_NOTE, + ER_TRUNCATED_WRONG_VALUE, + ER(ER_TRUNCATED_WRONG_VALUE), type, + ErrConvString(str, end - str, cs).ptr()); + } + }
Regards, Sergei
Hi Sergei, On 09/13/2015 05:17 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jul 21, Alexander Barkov wrote:
I have one question about "HANDLER FOR SQLWARNING". It does not catch messages of level "Note". Looks like a bug. Please confirm, I'll file a report if so.
Hmm, I don't know.
According to the standard, SQLWARNING should match sqlstates in the category W, while SQLEXCEPTION should be for sqlstates in the category X.
But we don't do that, 22007 is an exception (category X), but we treat it as a warning. So, the standard doesn't help to answer your question.
We seem to use the logic where a level "error" means an SQL exception and a level "warning" means an SQL warning. Using this logic, "notes" are neither exceptions nor warnings, and neither SQLEXCEPTION nor SQLWARNING should match them.
I'd say it's not a bug.
All right. We can think about it later. Perhaps, for symmetry, we should add HANDLER FOR SQLNOTE. I've added a new task, with a quote from this our discussion. MDEV-8809 HANDLER FOR SQLNOTE
The patch itself looks great, these changes in behavior are very welcomed. Just one small comment regarding THD::check_edom_and_truncation(), see below.
diff --git a/sql/sql_class.h b/sql/sql_class.h index a8d8444..c19174b 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3270,6 +3270,53 @@ class THD :public Statement, (!transaction.stmt.modified_non_trans_table || (variables.sql_mode & MODE_STRICT_ALL_TABLES))); } + /** + Check string-to-number conversion and produce a warning if + - could not convert any digits (EDOM-alike error) + - found garbage at the end of the string + See also Field_num::check_edom_and_truncation() for a similar function. + + @param thd - the thread + @param type - name of the data type (e.g. "INTEGER", "DECIMAL", "DOUBLE") + @param edom - of a EDOM-alike error happened + @param cs - character set of the original string + @param str - the original string + @param end_of_num - the position where the string-to-number routine stopped. + @param end - the end of the string + + Unlike Field_num::check_edom_and_truncation(), this function does not + distinguish between EDOM and truncation and reports the same warning for + both cases. Perhaps we should eventually print different warnings, to make + the explicit CAST work closer to the implicit cast in Field_xxx::store().
Why haven't you done it now? It causes too many changes in tests?
Yeah, I was afraid that. I'd like to do this, but in a separate change. I just created an MDEV for this: MDEV-8810 Warnings for EDOM vs truncation in val_int, val_real, val_decimal
+ */ + void check_edom_and_truncation(const char *type, bool edom, + CHARSET_INFO *cs, + const char *str, + const char *end_of_num, + const char *end)
Move this out of THD please. You don't need to call current_thd every time for it, but only when a warning should be issued.
Right, thanks for the idea. Moved. Now current_thd is called only if a warning/note is really needed, and there's no a "thd" available in the call context. Btw, while moving the code, I realized that it's easy to reuse the same code in: Field_string::val_real Field_string::val_int Field_string::val_decimal Field_varstring::val_real Field_varstring::val_int Field_varstring::val_decimal Field_blob::val_real Field_blob::val_int Field_blob::val_decimal and I did it. Now they are all just as small as two statements. Thanks!
+ { + DBUG_ASSERT(str <= end_of_num); + DBUG_ASSERT(end_of_num <= end); + if (edom || + (end_of_num < end && !check_if_only_end_space(cs, end_of_num, end))) + { + /* + We can use err.ptr() here as ErrConvString is guranteed to put an + end \0 here. + */ + push_warning_printf(this, Sql_condition::WARN_LEVEL_WARN, + ER_TRUNCATED_WRONG_VALUE, + ER(ER_TRUNCATED_WRONG_VALUE), type, + ErrConvString(str, end - str, cs).ptr()); + } + else if (end_of_num < end) + { + push_warning_printf(this, Sql_condition::WARN_LEVEL_NOTE, + ER_TRUNCATED_WRONG_VALUE, + ER(ER_TRUNCATED_WRONG_VALUE), type, + ErrConvString(str, end - str, cs).ptr()); + } + }
Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik