maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hi, Sergei!

On Tue, Feb 5, 2019 at 10:37 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

>
> as discussed, let's give the constraint the same name as the period.
> but don't forget to add a test where a user explicitly creates a
> constraint with this name. Like
>
>   create or replace table t (id int primary key, s date, e date,
>   period for mytime(s,e), constraint mytime (id < 100));
>
> Oh, I thought You meant to leave it as is. Ok, but what about making it
not a CHECK constraint? I mean, to make its name not overlapping with CHECK
constraints namespace. This will mean that the user can create his own
CHECK constraint with the same name, and have no syntax to drop PERIOD
constraint. What do you think about it?


> > +create or replace temporary table t (s date, e date, period for
> mytime(s,e));
> > +ERROR HY000: Temporary tables with application-time period not allowed
>
> _are_ not allowed.
> or, better,
> Application-time period table cannot be temporary
>
>  Yes, that's better, thanks

> +# SQL16, Part 2, 11.3  <table definition>, Syntax Rules, 2)e)iii)
> > +# The <data type or domain name> contained in CD1 is either DATE or a
> > +# timestamp type and it is equivalent to the <data type or domain name>
> > +# contained in CD2.
> > +create or replace table t (id int primary key, s datetime, e date,
> > +period for mytime(s,e));
> > +ERROR HY000: PERIOD FOR `mytime` fields types mismatch
>
> _field_ type mismatch
> or
> Fields `s` and `t` of PERIOD FOR `mytime` have different types.
> or
> Fields of PERIOD FOR `mytime` have different types.
>
> I like the last one. Thanks for suggestions


> > +create or replace table t (s timestamp(2), e timestamp(6),
> > +period for mytime(s,e));
> > +ERROR HY000: PERIOD FOR `mytime` fields types mismatch
> > +create or replace table t (id int primary key, s int, e date,
> > +period for mytime(s,e));
> > +ERROR 42000: Incorrect column specifier for column 's'
>
> incorrect column _type_?
>
>  Just used already existed ER_WRONG_FIELD_SPEC. Any better ideas?

> +create or replace table t (id int primary key, s date, e date,
> > +period for mytime(s,x));
> > +ERROR 42S22: Unknown column 'x' in 'mytime'
> > +create or replace table t (id int primary key, s date, e date,
> > +period for mytime(s,e),
> > +period for mytime2(s,e));
> > +ERROR HY000: Cannot specify more than one application-time period
> > +# SQL16, Part 2, 11.3  <table definition>, Syntax Rules, 2)d)
> > +# No <column name> in any <column definition> shall be equivalent to PN.
> > +create or replace table t (mytime int, s date, e date,
> > +period for mytime(s,e));
> > +ERROR HY000: Could not specify name `mytime` for field. It is already
> used by period.
>
> Better "Column and period `mytime` have the same name".
>
> Because "already used" implies that period got the name first, while in
> the table definition the field was specified first.
>
> It's "Duplicate column name" now. Should be in last revision already


> > +ERROR HY000: Period field `st` cannot be GENERATED ALWAYS AS
> > +# SQL16, Part 2, 11.3  <table definition>, Syntax Rules, 2)
> > +# Let IDCN be an implementation-dependent <constraint name> that is not
> > +# equivalent to the <constraint name> of any table constraint descriptor
> > +# included in S.
> > +create or replace table t (x int, s date, e date,
> > +period for mytime(s, e),
> > +constraint mytime check (x > 1));
>
> please, add SHOW CREATE TABLE here (add it after every successful CREATE
> TABLE).
>
>  actually, that's the only place.

> +create or replace database test;
> > diff --git a/sql/datadict.cc b/sql/datadict.cc
> > --- a/sql/datadict.cc
> > +++ b/sql/datadict.cc
> > @@ -292,6 +292,12 @@ bool dd_read_extra2(const uchar *frm_image, size_t
> len, extra2_fields *fields)
>
> We've discussed that TRUNCATE TABLE bug fix, it won't need
> dd_read_extra2().
> You still want to have a separate function for reading extra2, fine, but
> as dd_frm_type() won't needed it, I'd rather move it into table.cc
> and declared it static.
>
> Ok, I'm good with it


> > +    if (period_info.name.streq(f->field_name))
> > +    {
> > +      // TODO this stub should be removed by MDEV-16976
>
> why removed?
>
> That comment is already removed. Again, I was thinking about implementing
MDEV-16976 with virtual fields. This'd do this error redundant. But anyway
the tests will break in that case, so this code will not be forgotten.

> +      my_error(ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD, MYF(0),
> f->field_name.str);
> > +      return true;
> > +    }
> > +  }
> >
> > diff --git a/sql/table.h b/sql/table.h
> > --- a/sql/table.h
> > +++ b/sql/table.h
> > @@ -1730,6 +1747,9 @@ class IS_table_read_plan;
> >  /** The threshold size a blob field buffer before it is freed */
> >  #define MAX_TDC_BLOB_SIZE 65536
> >
> > +/** number of bytes read by uint2korr and sint2korr */
> > +#define korr2size 2
>
> No, that's silly. It's like #define TWO 2
> Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
>
Of course! That's the way to say "This variable was stored as a 2-byte
tuple".

If you want to get rid of the magic number, don't put it into the name
> of the macro :)
>
> Well i'm ok with 2 or 3, actually, but not 14, for example


> That's why I suggested something like
>
>   #define fieldno_size   2
>   #define fieldno_korr   uint2korr
>   #define fieldno_store  int2store
>
Oh no, that'll add 9 new macros! And the number can grow...

Well, since we can't find common solution, let's rollback to the first
one:) I'll just leave comments in sensible places
Ok?

-- 
Yours truly,
Nikita Malyavin