maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Fri, Apr 16, 2021 at 5:44 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> > index aecb00563f7..b23522ac830 100644
> > --- a/sql/share/errmsg-utf8.txt
> > +++ b/sql/share/errmsg-utf8.txt
> > @@ -7975,3 +7975,5 @@ ER_PK_INDEX_CANT_BE_IGNORED
> >          eng "A primary key cannot be marked as IGNORE"
> >  ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE
> >          eng "Function '%s' cannot be used in the %s clause"
> > +ER_ORACLE_COMPAT_FUNCTION_ERROR
> > +        eng "Oracle compatibility function error: %s"
>
> Why? We normally just say "invalid argument" or something, in no other case
> we say "oracle compatibility function" or "sybase compatibility function" or
> "odbc compatibility function".

Fixed by reusing ER_STD_INVALID_ARGUMENT

> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index 95a57017c53..9c57bb22085 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -1275,3 +1275,15 @@ void Binary_string::shrink(size_t arg_length)
> >      }
> >    }
> >  }
> > +
> > +bool Binary_string::strfill(char fill, size_t len)
> > +{
> > +  if (len)
> > +  {
> > +    if (alloc(length() + len))
> > +      return 1;
> > +    memset(Ptr + str_length, fill, len);
> > +    str_length+= (uint32) len;
> > +  }
> > +  return 0;
> > +}
>
> There's Binary_string::fill() already.
>
> better use it or, at the very least, declare
>
>  bool strfill(char fill, size_t len) { return fill(str_length + len, fill); }
>
> in sql_string.h. And this swapped order of arguments is confusing,
> please fix it too.

Agree, I did miss this when fixing a fill loop in the original commit.
I have now removed strfill(), as you requested.

<cut>

> > diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h
> > index af266956b05..9b78d6c159e 100644
> > --- a/sql/item_timefunc.h
> > +++ b/sql/item_timefunc.h

<cut>

> > +public:
> > +  Item_func_tochar(THD *thd, Item *a, Item *b):
> > +    Item_str_func(thd, a, b), locale(0)
> > +  {
> > +    /* NOTE: max length of warning message is 64 */
> > +    warning_message.alloc(64);
> > +    warning_message.length(0);
> > +  }
>
> As far as I understand, this warning_message was introduced to issue
> the same error for every row, even if the format string is const_item and
> is parsed only once, in fix_fields.
>
> I don't think it's worth the trouble. Two simpler approaches are:
> * if the format string is invalid - parse it every time even if const, or
> * if the const format string is invalid - issue the error only once.
>
> so, please, remove warning_message and just use push_warning or my_error
> where the error is discovered.

That may not work well for prepared statements or stored procdures
where we may not call fix_length_and_dec() twice.

I also think it is a bit wrong that one gets a different number of
warning messages depending on when the arguments are processed.

For now, I rather keep things as they are. Someone that has time to do
checking of the above cases and ensures that we do not miss any warnings
messages in some context can then consider what to do.

> > --- a/sql/item_timefunc.cc
> > +++ b/sql/item_timefunc.cc

<cut>

> > +  Models for datetime, used by TO_CHAR/TO_DATE. Normal format characters are
> > +  stored as short integer < 256, while format characters are stored as a
> > +  integer > 256
> > +*/
> > +
> > +#define FMT_BASE       128
>
> 128? or 256?

128. I have fixed the error message

> > +#define FMT_AD         FMT_BASE+1
> > +#define FMT_AD_DOT     FMT_BASE+2
> > +#define FMT_AM         FMT_BASE+3
> > +#define FMT_AM_DOT     FMT_BASE+4
<cut>

> Not enum? Not even safe (with parentheses) #define?
> enum would be ideal here but at the very least make these defines safe.

This is 100% safe in the context it is used.  However, I agree that enum
is better and I have now converted the above to an enum.

> > +
> > +/**
> > +  Modify the quotation flag and check whether the subsequent process is skipped
>
> Could you reword it please?

Changed to:

   Flip 'quotation_flag' if we found a quote (") character.

The old function comment explains this in more detail.
>
> > +
> > +  @param cftm             Character or FMT... format descriptor
> > +  @param quotation_flag   Points to 'true' if we are inside a quoted string
> > +
> > +  @return true  If we are inside a quoted string or if we found a '"' character
> > +  @return false Otherwise
> > +*/
> > +
> > +static inline bool check_quotation(uint16 cfmt, bool *quotation_flag)
> > +{
> > +  if (cfmt == '"')
> > +  {
> > +    *quotation_flag= !*quotation_flag;
> > +    return true;
> > +  }
> > +  return *quotation_flag;
> > +}
> > +
> > +#define INVALID_CHARACTER(x) (((x) >= 'A' && (x) <= 'Z') ||((x) >= '0' && (x) <= '9') || (x) >= 127 || ((x) < 32))
>
> why not to make this static inline too?
> side-effect safe, no risk of double evaluation of x.

The above is safe in the context it is used. For simple things like
this, which is only used in a few places in the current file, I
personally prefer #defines, so I am ok with the current code.

Regards,
Monty