maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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