maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander,

On Jul 29, Alexander Barkov wrote:
>    Hello Sergei,
> 
> I have a couple of suggestions:
> 
> > commit 7b8304045272111a6f4d44196d6b37cbfef06f37
> > Author: Sergei Golubchik <serg@xxxxxxxxxxx>
> > Date:   Wed Jul 20 17:31:48 2022 +0200
> > 
> >     MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently
> >     
> >     move the OPTION_EXPLICIT_DEF_TIMESTAMP check from the parsing step
> >     to the execution
> 
> > > diff --git a/sql/field.cc b/sql/field.cc
> > index 2e0c70d3d13..23ebb07b7f7 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -10824,6 +10824,7 @@ Column_definition::Column_definition(
> >    comment=    old_field->comment;
> >    vcol_info=  old_field->vcol_info;
> >    option_list= old_field->option_list;
> > +  explicitly_nullable= !(old_field->flags & NOT_NULL_FLAG);
> 
> This introduces asymmetry in how NULL and NOT NULL attributes are handled.
> 
> I suggest considering these ways:
> 
> 1. Change "bool explicitly_nullable" to "bool explicit_nullability",
> which will be:
> 
> - false if nothing was specified
> - true if NULL or NOT NULL was specified.

I've done that, see commit 7120195ac1c,
but I don't like how it looks. When NOT_NULL_FLAG is set, the value is
redundant, it's not used anywhere and only adds few pointless
assignments to sql_yacc.yy.

We only need to know how to interpret the case when NOT_NULL_FLAG is not
set, that's what explicitly_nullable was doing.

> 2. Or don't store NOT_NULL_FLAG in Column_definition::flags at all.
> 
> Add a new Column_definition enum member with three values for:
> - not specified
> - explicit NULL
> - explicit NOT NULL
> 
> As discussed on slack, the latter will need some changes.
> But I think not that much, as only Column_definition needs changes,
> while Field does not seem to need at this point.

I don't know how to do it in a robust way. Column_definition and Field
have both the same flags member, one typically just copies it back and
forth. Having just one bit out of Column_definition::flags is very
error-prone, I'd prefer all flags to be stored very differently in
Column_definition, so that one couldn't just copy them. But this
explicitly_nullable thing cannot justify such a big change.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx