maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

On Fri, Jan 4, 2019 at 6:29 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

>
> > MDEV-16975 Application period tables: ALTER TABLE
> >
> > * implicit period constraint is hidden and cannot be dropped
> independently
> > * create...like and create...select support
> >
> > diff --git a/mysql-test/suite/period/r/alter.result
> b/mysql-test/suite/period/r/alter.result
> > new file mode 100644
> > index 00000000000..6bb9f905358
> > --- /dev/null
> > +++ b/mysql-test/suite/period/r/alter.result
> > @@ -0,0 +1,101 @@
> > +set @s= '1992-01-01';
> > +set @e= '1999-12-31';
> > +create or replace table t (s date, e date);
> > +alter table t add period for a(s, e);
> > +ERROR HY000: Period field `s` cannot be NULL
> > +alter table t change s s date, add period for a(s, e);
> > +ERROR HY000: Period field `s` cannot be NULL
> > +alter table t change s s date, change e e date, add period for a(s, e);
> > +ERROR HY000: Period field `s` cannot be NULL
> > +alter table t change s s date not null, change e e date not null,
> > +add period for a(s, e);
> > +show create table t;
> > +Table        Create Table
> > +t    CREATE TABLE `t` (
> > +  `s` date NOT NULL,
> > +  `e` date NOT NULL,
> > +  PERIOD FOR `a` (`s`, `e`)
> > +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> > +alter table t add id int;
> > +alter table t drop id;
> > +insert t values(@e, @s);
> > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t`
>
> Hmm. The standard, indeed, says "there's an implicit constraint".
>
> But it does not say "CHECK contraint". UNIQUE is also a constraint,
> Foreign key is also a constraint.
>
> So, I think here it means really just *a* constraint (a limitation, in
> other words), meaning only that start value must be before the end
> value. It doesn't mean that a CHECK constraint must be created.
>
> In other words, it should not be a CHECK constraint, it should not
> produce a `CONSTRAINT xxx failed` message, and, of course, it should
> never ever be shown as a CHECK constraint.
>
>   11.3 <table definition>, Syntax Rules, 2)e)v)2)B)

> Let S be the schema identified by the explicit or implicit <schema name>
> of TN. Let IDCN be an implementation-dependent <constraint name> that is
> not equivalent to the <constraint name> of any table constraint descriptor
> included in S. The following <table constraint definition> is implicit:
> CONSTRAINT IDCN CHECK ( CN1 < CN2 )

And General Rules, 4) e)

> A table descriptor TDS is created that describes T. TDS includes:

 If a <table period definition> that contains an <application time period
> specification> is specified that contains <application time period name>
> ATPN, then a period descriptor that contains ATPN as the name of the
> period, the names of the ATPN period start and ATPN period end columns, and
> IDCN as the name of the implicit ATPN period constraint.


So it's CHECK constraint.
I think that the standart is quite inconvenient with naming here, but note
that it is implementation-dependant, so we might choose better naming rule
later. Or explicitly violate the rule and just name the constraint with
period name -- I already have such version, if you're interested.

> +alter table t drop constraint constraint_1;
> > +ERROR HY000: Can't DROP CONSTRAINT `CONSTRAINT_1`. Use DROP PERIOD `a`
> for this
> > +alter table t drop period for a;
> > +# Constraint is dropped
> > +insert t values(@e, @s);
> > +alter table t drop period for a;
> > +ERROR 42000: Can't DROP PERIOD `a`; check that it exists
> > +alter table t add period for a(s, e), drop period for a;
> > +ERROR 42000: Can't DROP PERIOD `a`; check that it exists
> > +truncate t;
> > +alter table t add period for a(s, e);
> > +insert t values(@e, @s);
> > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t`
> > +alter table t add period for a(s, e), drop period for a;
> > +insert t values(@e, @s);
> > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t`
> > +alter table t add s1 date not null, add period for b(s1, e), drop
> period for a;
> > +show create table t;
> > +Table        Create Table
> > +t    CREATE TABLE `t` (
> > +  `s` date NOT NULL,
> > +  `e` date NOT NULL,
> > +  `s1` date NOT NULL,
> > +  PERIOD FOR `b` (`s1`, `e`)
> > +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> > +insert t(s, s1, e) values(@e, @s, @e);
> > +insert t(s, s1, e) values(@e, @e, @s);
> > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t`
> > +alter table t add constraint check (s < s1 and s1 < e);
> > +ERROR HY000: CONSTRAINT `(null)` uses fields from PERIOD `b`
>
> huh?
> looks like a null-pointer dereference with a defensive crash-safe printf.
>
Name is not assigned to the moment of that check. AFAIR, it is done in
create_table_impl.

> and why is it an error?
>
> I was thinking that referencing period fields by virtual fialds, or
constraints, is forbidden. But can't find anything after lurking through
the whole standard. The only such restriction I found is imposed to
SYSTEM_TIME.

So I'm removing this check, and fixing the DELETE/UPDATE behavior as well
-- should enable constraints checks on INSERTs.

> +create table t1 like t;
> > +show create table t1;
> > +Table        Create Table
> > +t1   CREATE TABLE `t1` (
> > +  `s` date NOT NULL,
> > +  `e` date NOT NULL,
> > +  `s1` date NOT NULL,
> > +  PERIOD FOR `b` (`s1`, `e`)
> > +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> > +create table t2 (period for b(s,e)) select * from t;
> > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t2`
> > +create table t2 (period for b(s1,e)) select * from t;
> > +# SQL17 p.895
>
> please, fix all the references to the standard to mention the part
> number, section number and paragraph number instead of the page number.
>
> ✅

> > +# The declared type of BC1 shall be either DATE or a timestamp type
> > +# and shall be equivalent to the declared type of BC2.
> > +create or replace table t (s timestamp not null, e timestamp(6) not
> null);
> > +alter table t add period for a(s, e);
> > +ERROR HY000: PERIOD FOR `a` fields types mismatch
> > +# SQL17 p.895
> > +# No column of T shall have a column name that is equivalent to ATPN.
> > +create or replace table t (a int, s date, e date);
> > +alter table t add period for a(s, e);
> > +ERROR HY000: Period and field `a` cannot have same name.
> > +# SQL17 p.895
> > +# Neither BC1 nor BC2 shall be an identity column, a generated column,
> > +# a system-time period start column, or a system-time period end column.
> > +create or replace table t (id int primary key,
> > +s date,
> > +e date generated always as (s+1));
> > +alter table t add period for a(s, e);
> > +ERROR HY000: Period field `s` cannot be NULL
> > +create or replace table t (id int primary key,
> > +s date,
> > +e date as (s+1) VIRTUAL);
> > +alter table t add period for a(s, e);
> > +ERROR HY000: Period field `s` cannot be NULL
> > +create or replace table t (id int primary key, s timestamp(6), e
> timestamp(6),
> > +st timestamp(6) as row start,
> > +en timestamp(6) as row end,
> > +period for system_time (st, en)) with system versioning;
> > +alter table t add period for a(s, en);
> > +ERROR HY000: Period field `en` cannot be ROW END
> > +# SQL17 p.895
> > +# The table descriptor of T shall not include a period descriptor other
> > +# than a system-time period descriptor.
> > +alter table t add period for a(s, e);
> > +alter table t add period for b(s, e);
> > +ERROR HY000: cannot specify more than one application-time period
> > +create or replace database test;
> > diff --git a/mysql-test/suite/period/r/create.result
> b/mysql-test/suite/period/r/create.result
> > index 420eceb2e9a..8c06a0cc134 100644
> > --- a/mysql-test/suite/period/r/create.result
> > +++ b/mysql-test/suite/period/r/create.result
> > @@ -51,7 +51,7 @@ ERROR HY000: cannot specify more than one
> application-time period
> >  # 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.
> > +ERROR HY000: Period and field `mytime` cannot have same name.
>
> This is better. Although I'd just use an existing ER_DUP_FIELDNAME
>
> Made using  ✅

> >  # CD1 and CD2 both contain either an explicit or an implicit
> >  # <column constraint definition> that specifies
> >  # NOT NULL NOT DEFERRABLE ENFORCED.
> > diff --git a/sql/field.cc b/sql/field.cc
> > index d7214687e2d..e065d61e3a5 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -10545,6 +10545,8 @@ Column_definition::Column_definition(THD *thd,
> Field *old_field,
> >        unireg_check= Field::TMYSQL_COMPRESSED;
> >        compression_method_ptr= zlib_compression_method;
> >      }
> > +    if (orig_field->maybe_null())
> > +      flags|= EXPLICIT_NULL_FLAG;
>
> This shouldn't be needed, as I wrote in a CREATE TABLE review,
> NOT NULL should be implicit, not an error.
>
> ✅

> >    }
> >    else
> >    {
> > diff --git a/sql/sql_alter.h b/sql/sql_alter.h
> > index 108b98afdd7..d917793dd90 100644
> > --- a/sql/sql_alter.h
> > +++ b/sql/sql_alter.h
> > @@ -95,6 +95,8 @@ class Alter_info
> >      CHECK_CONSTRAINT_IF_NOT_EXISTS= 1
> >    };
> >    List<Virtual_column_info>     check_constraint_list;
> > +  List<Table_period_info>       period_list;
>
> looks unused (in this patch, at least)
>
> removed ✅

> > +
> >    // Type of ALTER TABLE operation.
> >    alter_table_operations        flags;
> >    ulong                         partition_flags;
> > diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> > index 4189131f801..211969647d4 100644
> > --- a/sql/sql_table.cc
> > +++ b/sql/sql_table.cc
> > @@ -6152,6 +6152,11 @@ handle_if_exists_options(THD *thd, TABLE *table,
> Alter_info *alter_info)
> >            }
> >          }
> >        }
> > +      else if (drop->type == Alter_drop::PERIOD)
> > +      {
> > +        if (table->s->period.name.streq(drop->name))
> > +          remove_drop= FALSE;
>
> you don't seem to have a single test for DROP PERIOD IF EXISTS
>
>  added test.

> > +      }
> >        else /* Alter_drop::KEY and Alter_drop::FOREIGN_KEY */
> >        {
> >          uint n_key;
> > @@ -8406,6 +8412,34 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
> >      }
> >    }
> >
> > +  if (table->s->period.name)
> > +  {
> > +    drop_it.rewind();
> > +    Alter_drop *drop;
> > +    for (bool found= false; !found && (drop= drop_it++); )
> > +    {
> > +      found= table->s->period.name.streq(drop->name);
> > +    }
> > +
> > +    if (drop)
> > +    {
> > +      drop_period= true;
> > +      drop_it.remove();
> > +    }
> > +    else if (create_info->period_info.is_set() && table->s->period.name
> )
> > +    {
> > +      my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application");
> > +      goto err;
> > +    }
> > +    else
> > +    {
> > +      Field *s= table->s->period.start_field(table->s);
> > +      Field *e= table->s->period.end_field(table->s);
> > +      create_info->period_info.set_period(s->field_name, e->field_name);
> > +      create_info->period_info.name= table->s->period.name;
>
> why couldn't you remember Field's, why only names?
> is it because period_info is also used on CREATE TABLE and you don't
> have Field's there?
>
> That's the Fields  from old table SHARE, which's going to be replaced. Why
should I remember them? Can't see any usage.

> > +    }
> > +  }
> > +
> >    /* Add all table level constraints which are not in the drop list */
> >    if (table->s->table_check_constraints)
> >    {
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 10535326ce5..545c8f831b9 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -8190,6 +8195,10 @@ alter_list_item:
> >            {
> >              Lex->alter_info.flags|= ALTER_ADD_PERIOD;
> >            }
> > +        | ADD period_for_application_time
>
> where's ADD PERIOD if_not_exists?
>
>  Not very clean with the behavior expected. Consider CREATE TABLE t (s
date, e date, period for a(s,e));
Should ALTER TABLE ADD IF NOT EXISTS PERIOD FOR b replace PERIOD FOR a, or
return ER_MORE_THAN_ONE_PERIOD?

Since it's not standard, maybe let's stay away from if_not_exists, as well
as if_exists maybe?

> +          {
> > +            Lex->alter_info.flags|= ALTER_ADD_CHECK_CONSTRAINT;
> > +          }
> >          | add_column '(' create_field_list ')'
> >            {
> >              LEX *lex=Lex;
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>


-- 
Yours truly,
Nikita Malyavin