maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

---------- Forwarded message ---------
From: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
Date: пт, 18 янв. 2019 г. в 01:46
Subject: Re: ce4f8955d8d: MDEV-17082 Application period tables: CREATE
To: Sergei Golubchik <serg@xxxxxxxxxxx>


Hi, Sergei!

> > +# SQL17 p.832
> > +# Let IDCN be an implementation-dependent <constraint name> that is not
> > +# equivalent to the <constraint name> of any table constraint descriptor
> > +# included in S.
>
> Not enough context. It was quite difficult to find this quote.
>
> When you're quoting the standard, please, specify the part name and the
> section name. And there's no need to specify a page. For example:
>
> SQL2016, Part 2, 11.27 <add table period definition>, General Rules, 2)a)
>
Fixed, as well as all the rest cites

> > +    Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1),
>
> We have a macro for that, use
>
>    Table_period_info(STRING_WITH_LEN("SYSTEM_TIME"))
>
✅

> > +   then no promition is done.
>
> promotion
✅


> > +  Table_period_info& get_table_period_info()
> > +  {
> > +    return create_info.period_info;
> > +  }
>
> really? :) you've introduced a new method get_table_period_info
> only to use it two lines below and nowhere else?
>
Not really, of course. But agree, it's not worth to have such a shortcut.
> > +
> > +  int add_period(Lex_ident name, Lex_ident_sys_st start, Lex_ident_sys_st end)
> > +  {
> > +    Table_period_info &info= get_table_period_info();
> > +    if (info.is_set())
> > +    {
> > +       my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application");
> > +       return 1;
> > +    }
> > +    info.set_period(start, end);
> > +    info.name= name;
> > +
> > +    Virtual_column_info *constr= new Virtual_column_info();
> > +    constr->expr= lt_creator.create(thd, create_item_ident_nosp(thd, &start),
> > +                                    create_item_ident_nosp(thd, &end));
> > +    add_constraint(&null_clex_str, constr, false);
> > +    return 0;
> > +  }
> > +
> >    sp_package *get_sp_package() const;
> >
> >    /**
> > 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.

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?

> > +    my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> > +             f->field_name.str, "NULL");
>
> why is that? it's unrelated and inconsistent with cases like
>
>   create table t1 (a int null, primary key (a));
>
> where a column is silently created NOT NULL.
> So, you should ignore explicit NULL and create columns NOT NULL.
>
> If you want to change this, it should be consistent in all cases.
> create an MDEV, suggest there that the statement above should be an error.
> but I don't think it's worth spending time on now.
>
Ok, I understand. Fixed.

> > +  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

> What does this error have to do with MDEV-16976?
> SQL standard clearly says (part 2, 11.3 <table definition>, syntax rules, 2)d)
>
>    d) No <column name> in any <column definition> shall be equivalent to PN.
>
> (here PN is "period name"). So MDEV-16976 or not, a period name should
> never match a column name.
>
I was thinking to implement it through virtual column. Then this check
will become redundant, and we'll get duplicate field error

> > +        eng "Temporary tables with application period not allowed"
>
> "application time period"
>
✅
> > +
> > +ER_PERIOD_FIELD_NOT_FOUND
> > +        eng "Field %`s in PERIOD FOR %`s is not found in table"
>
> I don't think this needs a new error message, just use ER_BAD_FIELD_ERROR
>
✅
> > +
> > +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"

> > +        eng "cannot specify more than one %s-time period"
> > +
> > +ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD
> > +        eng "Could not specify name %`s for field. It is already used by period."
>
> I don't think this needs a new error message, just use ER_DUP_FIELDNAME
>
✅
> > +
> > +ER_PERIOD_FIELD_WRONG_ATTRIBUTES
> > +        eng "Period field %`s cannot be %s"
> > +
> > +ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD
> > +        eng "Field %`s is specified for more than one period"
>
> This looks unused
✅

Sincerely
Nikita Malyavin