Re: [Maria-developers] 83444b747f1: MDEV-16975 Application period tables: ALTER TABLE
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@gmail.com> committer: Nikita Malyavin <nikitamalyavin@gmail.com> 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@mariadb.org
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.
+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
+alter table t drop constraint constraint_1; 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?
I was thinking that referencing period fields by virtual fialds, or constraints, is forbidden. But can't find anything after lurking through
Name is not assigned to the moment of that check. AFAIR, it is done in create_table_impl. 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
Hi, Nikita! On Feb 04, Nikita Malyavin wrote:
On Fri, Jan 4, 2019 at 6:29 AM Sergei Golubchik <serg@mariadb.org> wrote:
+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 )
Good catch.
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.
Yes, I was thinking that making constraint name the same as a period name would be good. I'm still not sure that an "implicit constraint" means an actual constraint is created and it's visible in SHOW CREATE TABLE. Usually in such cases the standard says "the following statement is effectively executed: INSERT ..." instead of "the followig INSERT is implicit" But saying "constraint XXX is violated" and not showing the constraint XXX in the SHOW CREATE TABLE will be confusing as hell for every single user. So let's add it explicitly, all right.
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?
ER_MORE_THAN_ONE_PERIOD of course. It adds a period name `b`, it's a different period, and there can be only one. So - an error. If it'd be ALTER TABLE ADD IF NOT EXISTS PERIOD FOR a(....) it'd be a no-op. I cannot see of a case when ALTER TABLE ADD IF NOT EXISTS should replace stuff. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! On Mon, Feb 4, 2019 at 11:49 PM Sergei Golubchik <serg@mariadb.org> wrote:
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?
ER_MORE_THAN_ONE_PERIOD of course. It adds a period name `b`, it's a different period, and there can be only one. So - an error.
If it'd be
ALTER TABLE ADD IF NOT EXISTS PERIOD FOR a(....)
it'd be a no-op.
I cannot see of a case when ALTER TABLE ADD IF NOT EXISTS should replace stuff.
Made it ADD PERIOD IF NOT EXISTS FOR a(....) First, it is more consistent with the way other entities handled, e.g. ADD CONSTRAINT IF NOT EXISTS; second, no shift/reduce conflicts:) -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik