On Fri, Jan 4, 2019 at 6:29 AM Sergei Golubchik <serg@mariadb.org> 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@mariadb.org

Yours truly,

Nikita Malyavin