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 <[email protected]> > committer: Nikita Malyavin <[email protected]> > 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 [email protected] _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
