maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hi, Sergei!

On Sat, Jan 19, 2019 at 10:14 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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.
>
> Thanks for advice! Looks suitable enough
I will just make #define korr2size 2. But it should be shared between
table.cc and unireg.cc.
Can You suggest me where to put it?

> 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.
>
Well, I agree that 'slash' ('/') is a sintactic element, but was
considering it language-independent. Ok then.

> 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.
>
See. Thanks for exhaustive description.
That's done


>
> > > > +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.
>
Yes, already fixed that.


-- 
Yours truly,
Nikita Malyavin