maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 09/15/2015 10:19 AM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Sep 15, Alexander Barkov wrote:
>
> > > > -              Lex->last_field->flags|= NOT_NULL_FLAG;
> > > > +              if (!thd->variables.explicit_defaults_for_timestamp)
> > > > +                Lex->last_field->flags|= NOT_NULL_FLAG;
> > >
> > > Hmm, I don't like that. You're making a run-time decision during the parsing.
> > > Generally this is wrong.
> ...
> > At this point I just merged the changes from MySQL,
> > and the variable is read only.
>
> Hmm, in that case why did you add it to thd->variables?
>
> That only makes sense for session variables that can have different
> values in different connections. Global (and read-only) variables don't
> need to have a copy of itself in every THD.

Hmm. Right.
The original MySQL changes have a replication related addon patch for the
slave that tries to guess what was explicit_defaults_for_timestamp
on the master, depending on its version and some other heuristic.
So they do update explicit_defaults_for_timestamp per thread on the slave.

As Monty proposed to make it dynamic (MDEV-8455), I did not
include this code and planned to make all replication related
changes in a single patch for MDEV-8455.
But I forgot to exclude the variable from the session variables.

Will do that.

> > > > diff --git a/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test b/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test
> > > > new file mode 100644
> > > > index 0000000..372e37c
> > > > --- /dev/null
> > > > +++ b/mysql-test/suite/sys_vars/t/explicit_defaults_for_timestamp_basic.test
> > > > @@ -0,0 +1,19 @@
> > >
> > > No need to add these boilerplate *_basic tests anymore.
> > > if there's something worth testing there - yes, but if you just
> > > copy another test like that and replace the variable name - don't bother
> >
> > I put the test that it's a read-only variable here.
> > I had to put it somewhere anyway. Would you like to
> > to rename this test somehow?
>
> I mean that we have sysvar_* tests now. These test shows already that
> explicit_defaults_for_timestamp is a read-only variable. I'd say it's
> sufficient.
>
> Regards,
> Sergei