maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

Please, see my comments below

On Dec 28, Nikita Malyavin wrote:
> revision-id: 83444b747f1 (versioning-1.0.7-5-g83444b747f1)
> parent(s): c39f74ce0d9
> author: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> timestamp: 2018-12-03 13:19:18 +1000
> message:
> 
> 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.

> +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.
and why is it an error?

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

>  # 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)

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

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

> +    }
> +  }
> +
>    /* 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?

> +          {
> +            Lex->alter_info.flags|= ALTER_ADD_CHECK_CONSTRAINT;
> +          }
>          | add_column '(' create_field_list ')'
>            {
>              LEX *lex=Lex;

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx