maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Jan 18, Nikita Malyavin wrote:
> > > diff --git a/sql/table.cc b/sql/table.cc
> > > index ab1a04ccae4..87fa1ba4d61 100644
> > > --- a/sql/table.cc
> > > +++ b/sql/table.cc
> > > @@ -1228,6 +1228,14 @@ static const Type_handler *old_frm_type_handler(uint pack_flag,
> > >  }
> > >
> > >
> > > +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period,
> > > +                                          const uchar *data)
> > > +{
> > > +  period.start_fieldno= uint2korr(data);
> > > +  period.end_fieldno= uint2korr(data + sizeof(uint16));
> >
> > not "sizeof(uint16)" but 2. I don't think it's guaranteed that
> > sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes,
> > so data + 2 is always correct.
> >
> Yes, it's correct, and I've fixed it, but it wasn't about coorrectness
> -- but readability.
> Now what 4, or what is 8 -- is the question that the reader could
> сonsider. This numbers look magical.
> With using sizeof(uint16) the one could at least understand, that this
> is about several double-byte words read.

In a similar case I used something like

#define SIZE 2
#define uintSIZEkorr(X) uint2korr(X)

This way there are no magically-looking numbers. You can even
write `uint ## SIZE ## korr`, if you're feeling adventurous.

> The another problem is that sizeof(uint16) is already used in system
> versioning, few lines below, so the resulting code will look patchy
> here.
> 
> > > +    my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> > > +             f->field_name.str, "VIRTUAL or GENERATED");
> >
> > just use "GENERATED", no English in the error message parameters, please.
> >
> VIRTUAL/GENERATED? What'd You say?

You cannot have parts of an English phrase as a parameter.
And "VIRTUAL" is just a subset of "GENERATED" anyway.
So, just write

  my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
           f->field_name.str, "GENERATED");

or, better,

  my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
           f->field_name.str, "GENERATED ALWAYS AS");

> > > +  else if (f->flags & VERS_SYSTEM_FIELD)
> > > +  {
> > > +    my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str,
> > > +             f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END");
> >
> > No need to have a special case for that. Use the same error as for vcol_info:
> >
> > if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD)
> > {
> >   my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> >            f->field_name.str, "GENERATED ALWAYS AS");
> >
> Looks not very much user-friendly I think. Let's leave the verbosity as it was

No, it's illogical. You have columns, like

  a TIMESTAMP(6) GENERATED ALWAYS AS (NOW() + INTERVAL 1 DAY),
  b TIMESTAMP(6) GENERATED ALWAYS AS ROW START

for the first column the error is

  Period field %`s cannot be GENERATED

for the second column the error is

  Period field %`s cannot be ROW START

Why? They're both GENERATED. You don't say

  Period field %`s cannot be NOW() + INTERVAL 1 DAY

See? So just say in both cases that the period cannot be 
"GENERATED ALWAYS AS". This is quite sufficient.

> > > +ER_PERIOD_TYPES_MISMATCH
> > > +        eng "PERIOD FOR %`s fields types mismatch"
> > > +
> > > +ER_PERIOD_MAX_COUNT_EXCEEDED
> >
> > better say ER_MORE_THAN_ONE_PERIOD or something...
> > "max count exceeded" suggests there is some not-trivial
> > (and possibly variable) max count
> >
> > Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc.
> > "already defined" looks like a good one.
> >
> > And I've already commented (in another patch) on using English
> > words as arguments - it will look like a bug when the error message is
> > translated
> >
> I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods
> designed in the way to support a possibility of several periods
> definition, so i like to treat it as "N periods max supported, but now
> N=1"

The standard clearly says there can be at most one application-time period.
So "Application-time period already defined" is just as fine.
When the new standard will allow for many application-time periods, we
can rewrite the error message. But don't hold your breath.

And, again, you cannot have "application" as a parameter.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx