Re: [Maria-developers] 4b01d3aee60: MDEV-17082 Application-time periods: CREATE
Hi, Nikita! On Jan 31, Nikita Malyavin wrote:
revision-id: 4b01d3aee60 (versioning-1.0.7-2-g4b01d3aee60) parent(s): 44144533e50 author: Nikita Malyavin <nikitamalyavin@gmail.com> committer: Nikita Malyavin <nikitamalyavin@gmail.com> timestamp: 2019-01-30 22:53:43 +1000 message:
MDEV-17082 Application-time periods: CREATE
* add syntax `CREATE TABLE ... PERIOD FOR <apptime>` * add table period entity
diff --git a/mysql-test/suite/period/r/create.result b/mysql-test/suite/period/r/create.result --- /dev/null +++ b/mysql-test/suite/period/r/create.result @@ -0,0 +1,82 @@ +create or replace table t (id int primary key, s date, e date, +period for mytime(s,e)); +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `id` int(11) NOT NULL, + `s` date NOT NULL, + `e` date NOT NULL, + PRIMARY KEY (`id`), + PERIOD FOR `mytime` (`s`, `e`), + CONSTRAINT `CONSTRAINT_1` CHECK (`s` < `e`)
as discussed, let's give the constraint the same name as the period. but don't forget to add a test where a user explicitly creates a constraint with this name. Like create or replace table t (id int primary key, s date, e date, period for mytime(s,e), constraint mytime (id < 100));
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 +create or replace table t (id int primary key, s timestamp(6), e timestamp(6), +period for mytime(s,e)); +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `id` int(11) NOT NULL, + `s` timestamp(6) NOT NULL DEFAULT '0000-00-00 00:00:00.000000', + `e` timestamp(6) NOT NULL DEFAULT '0000-00-00 00:00:00.000000', + PRIMARY KEY (`id`), + PERIOD FOR `mytime` (`s`, `e`), + CONSTRAINT `CONSTRAINT_1` CHECK (`s` < `e`) +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)a) +# 2) If a <table period definition> TPD is specified, then: +# a) <table scope> shall not be specified. +create or replace temporary table t (s date, e date, period for mytime(s,e)); +ERROR HY000: Temporary tables with application-time period not allowed
_are_ not allowed. or, better, Application-time period table cannot be temporary
+# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)e)iii) +# The <data type or domain name> contained in CD1 is either DATE or a +# timestamp type and it is equivalent to the <data type or domain name> +# contained in CD2. +create or replace table t (id int primary key, s datetime, e date, +period for mytime(s,e)); +ERROR HY000: PERIOD FOR `mytime` fields types mismatch
_field_ type mismatch or Fields `s` and `t` of PERIOD FOR `mytime` have different types. or Fields of PERIOD FOR `mytime` have different types.
+create or replace table t (s timestamp(2), e timestamp(6), +period for mytime(s,e)); +ERROR HY000: PERIOD FOR `mytime` fields types mismatch +create or replace table t (id int primary key, s int, e date, +period for mytime(s,e)); +ERROR 42000: Incorrect column specifier for column 's'
incorrect column _type_?
+create or replace table t (id int primary key, s date, e date, +period for mytime(s,x)); +ERROR 42S22: Unknown column 'x' in 'mytime' +create or replace table t (id int primary key, s date, e date, +period for mytime(s,e), +period for mytime2(s,e)); +ERROR HY000: Cannot specify more than one application-time period +# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)d) +# 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.
Better "Column and period `mytime` have the same name". Because "already used" implies that period got the name first, while in the table definition the field was specified first.
+# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)e)v)2)A) +# Neither CD1 nor CD2 shall contain an <identity column specification>, a +# <generation clause>, a <system time period start column specification>, +# or a <system time period end column specification>. +create or replace table t (id int primary key, +s date, +e date generated always as (s+1), +period for mytime(s,e)); +ERROR HY000: Period field `e` cannot be GENERATED ALWAYS AS +create or replace table t (id int primary key, +s date, +e date as (s+1) VIRTUAL, +period for mytime(s,e)); +ERROR HY000: Period field `e` cannot be GENERATED ALWAYS AS +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), +period for mytime(st,e)) with system versioning; +ERROR HY000: Period field `st` cannot be GENERATED ALWAYS AS +# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2) +# Let IDCN be an implementation-dependent <constraint name> that is not +# equivalent to the <constraint name> of any table constraint descriptor +# included in S. +create or replace table t (x int, s date, e date, +period for mytime(s, e), +constraint mytime check (x > 1));
please, add SHOW CREATE TABLE here (add it after every successful CREATE TABLE).
+create or replace database test; diff --git a/sql/datadict.cc b/sql/datadict.cc --- a/sql/datadict.cc +++ b/sql/datadict.cc @@ -292,6 +292,12 @@ bool dd_read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields)
We've discussed that TRUNCATE TABLE bug fix, it won't need dd_read_extra2(). You still want to have a separate function for reading extra2, fine, but as dd_frm_type() won't needed it, I'd rather move it into table.cc and declared it static. In fact, if you'd like to keep extra2 code separate, you could move everything extra2 related to a separate object (like your extra2_fields) with methods for reading and writing it. But, please, don't do this refactoring before March.
fields->field_flags.str= extra2; fields->field_flags.length= length; break; + case EXTRA2_APPLICATION_TIME_PERIOD: + if (fields->application_period.str) + DBUG_RETURN(true); + fields->application_period.str= extra2; + fields->application_period.length= length; + break; default: /* abort frm parsing if it's an unknown but important extra2 value */ if (type >= EXTRA2_ENGINE_IMPORTANT) diff --git a/sql/handler.cc b/sql/handler.cc --- a/sql/handler.cc +++ b/sql/handler.cc @@ -7352,3 +7352,99 @@ bool Vers_parse_info::check_sys_fields(const Lex_table_name &table_name,
+bool Table_scope_and_contents_source_st::check_fields( + THD *thd, Alter_info *alter_info, TABLE_LIST &create_table) +{ + bool res= vers_check_system_fields(thd, alter_info, create_table); + if (res) + return true; + + if (!period_info.name) + return false; + + if (tmp_table()) + { + my_error(ER_PERIOD_TEMPORARY_NOT_ALLOWED, MYF(0)); + return true; + } + + Table_period_info::start_end_t &period= period_info.period; + const Create_field *row_start= NULL; + const Create_field *row_end= NULL; + List_iterator<Create_field> it(alter_info->create_list); + while (const Create_field *f= it++) + { + if (period.start.streq(f->field_name)) row_start= f; + else if (period.end.streq(f->field_name)) row_end= f; + + if (period_info.name.streq(f->field_name)) + { + // TODO this stub should be removed by MDEV-16976
why removed?
+ my_error(ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD, MYF(0), f->field_name.str); + return true; + } + }
diff --git a/sql/table.h b/sql/table.h --- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4. If you want to get rid of the magic number, don't put it into the name of the macro :) That's why I suggested something like #define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! On Tue, Feb 5, 2019 at 10:37 AM Sergei Golubchik <serg@mariadb.org> wrote:
as discussed, let's give the constraint the same name as the period. but don't forget to add a test where a user explicitly creates a constraint with this name. Like
create or replace table t (id int primary key, s date, e date, period for mytime(s,e), constraint mytime (id < 100));
Oh, I thought You meant to leave it as is. Ok, but what about making it
not a CHECK constraint? I mean, to make its name not overlapping with CHECK constraints namespace. This will mean that the user can create his own CHECK constraint with the same name, and have no syntax to drop PERIOD constraint. What do you think about it?
+create or replace temporary table t (s date, e date, period for mytime(s,e)); +ERROR HY000: Temporary tables with application-time period not allowed
_are_ not allowed. or, better, Application-time period table cannot be temporary
Yes, that's better, thanks
+# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)e)iii)
+# The <data type or domain name> contained in CD1 is either DATE or a +# timestamp type and it is equivalent to the <data type or domain name> +# contained in CD2. +create or replace table t (id int primary key, s datetime, e date, +period for mytime(s,e)); +ERROR HY000: PERIOD FOR `mytime` fields types mismatch
_field_ type mismatch or Fields `s` and `t` of PERIOD FOR `mytime` have different types. or Fields of PERIOD FOR `mytime` have different types.
I like the last one. Thanks for suggestions
+create or replace table t (s timestamp(2), e timestamp(6), +period for mytime(s,e)); +ERROR HY000: PERIOD FOR `mytime` fields types mismatch +create or replace table t (id int primary key, s int, e date, +period for mytime(s,e)); +ERROR 42000: Incorrect column specifier for column 's'
incorrect column _type_?
Just used already existed ER_WRONG_FIELD_SPEC. Any better ideas?
+create or replace table t (id int primary key, s date, e date,
+period for mytime(s,x)); +ERROR 42S22: Unknown column 'x' in 'mytime' +create or replace table t (id int primary key, s date, e date, +period for mytime(s,e), +period for mytime2(s,e)); +ERROR HY000: Cannot specify more than one application-time period +# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2)d) +# 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.
Better "Column and period `mytime` have the same name".
Because "already used" implies that period got the name first, while in the table definition the field was specified first.
It's "Duplicate column name" now. Should be in last revision already
+ERROR HY000: Period field `st` cannot be GENERATED ALWAYS AS +# SQL16, Part 2, 11.3 <table definition>, Syntax Rules, 2) +# Let IDCN be an implementation-dependent <constraint name> that is not +# equivalent to the <constraint name> of any table constraint descriptor +# included in S. +create or replace table t (x int, s date, e date, +period for mytime(s, e), +constraint mytime check (x > 1));
please, add SHOW CREATE TABLE here (add it after every successful CREATE TABLE).
actually, that's the only place.
+create or replace database test;
diff --git a/sql/datadict.cc b/sql/datadict.cc --- a/sql/datadict.cc +++ b/sql/datadict.cc @@ -292,6 +292,12 @@ bool dd_read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields)
We've discussed that TRUNCATE TABLE bug fix, it won't need dd_read_extra2(). You still want to have a separate function for reading extra2, fine, but as dd_frm_type() won't needed it, I'd rather move it into table.cc and declared it static.
Ok, I'm good with it
+ if (period_info.name.streq(f->field_name)) + { + // TODO this stub should be removed by MDEV-16976
why removed?
That comment is already removed. Again, I was thinking about implementing MDEV-16976 with virtual fields. This'd do this error redundant. But anyway the tests will break in that case, so this code will not be forgotten.
+ my_error(ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD, MYF(0), f->field_name.str);
+ return true; + } + }
diff --git a/sql/table.h b/sql/table.h --- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
Of course! That's the way to say "This variable was stored as a 2-byte tuple". If you want to get rid of the magic number, don't put it into the name
of the macro :)
Well i'm ok with 2 or 3, actually, but not 14, for example
That's why I suggested something like
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
Oh no, that'll add 9 new macros! And the number can grow... Well, since we can't find common solution, let's rollback to the first one:) I'll just leave comments in sensible places Ok? -- Yours truly, Nikita Malyavin
Hi, Nikita! On Feb 08, Nikita Malyavin wrote:
On Tue, Feb 5, 2019 at 10:37 AM Sergei Golubchik <serg@mariadb.org> wrote:
as discussed, let's give the constraint the same name as the period. but don't forget to add a test where a user explicitly creates a constraint with this name. Like
create or replace table t (id int primary key, s date, e date, period for mytime(s,e), constraint mytime (id < 100));
Oh, I thought You meant to leave it as is. Ok, but what about making it not a CHECK constraint? I mean, to make its name not overlapping with CHECK constraints namespace. This will mean that the user can create his own CHECK constraint with the same name, and have no syntax to drop PERIOD constraint. What do you think about it?
I thought you've wanted it a CHECK constraint. You've quoted the standard proving your point of view.
+create or replace table t (s timestamp(2), e timestamp(6), +period for mytime(s,e)); +ERROR HY000: PERIOD FOR `mytime` fields types mismatch +create or replace table t (id int primary key, s int, e date, +period for mytime(s,e)); +ERROR 42000: Incorrect column specifier for column 's'
incorrect column _type_?
Just used already existed ER_WRONG_FIELD_SPEC. Any better ideas?
ok, let's keep it this way for now
diff --git a/sql/table.h b/sql/table.h --- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
Of course! That's the way to say "This variable was stored as a 2-byte tuple".
I mean, you didn't remove the magic number. It's still 2, and if you'd like to change it to be 3, for example, you'd still need to go and change the code everywhere.
That's why I suggested something like
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
Oh no, that'll add 9 new macros! And the number can grow...
What do you mean, it's just three new macros, like above. Where's 9? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Nikita!
On Feb 08, Nikita Malyavin wrote:
On Tue, Feb 5, 2019 at 10:37 AM Sergei Golubchik <serg@mariadb.org> wrote:
as discussed, let's give the constraint the same name as the period. but don't forget to add a test where a user explicitly creates a constraint with this name. Like
create or replace table t (id int primary key, s date, e date, period for mytime(s,e), constraint mytime (id < 100));
Oh, I thought You meant to leave it as is. Ok, but what about making it not a CHECK constraint? I mean, to make its name not overlapping with CHECK constraints namespace. This will mean that the user can create his own CHECK constraint with the same name, and have no syntax to drop PERIOD constraint. What do you think about it?
I thought you've wanted it a CHECK constraint. You've quoted the standard proving your point of view.
I've also quoted that it's name shouldn't match any existing constraint name:) So if to choose what to violate -- I'd choose the CHECK word, so adding a
diff --git a/sql/table.h b/sql/table.h
--- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
Of course! That's the way to say "This variable was stored as a 2-byte tuple".
I mean, you didn't remove the magic number. It's still 2, and if you'd like to change it to be 3, for example, you'd still need to go and change the code everywhere.
Oh, so You're really thinking on such refactoring, to change the sizes of
Hi, Sergei! On Fri, Feb 8, 2019 at 2:18 AM Sergei Golubchik <serg@mariadb.org> wrote: period wouldn't harm any possible existing standard-conformant table metainfo types rapidly? Then that 👇makes sense:
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
But I only care for self-documentation here. Besides, maybe it then shouldn't be fixed-size, but with unicode-like continuation marks? AFAIK current restriction is 4k columns per table, so there should be around four free bits. It will be even backwards compatible
That's why I suggested something like
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
Oh no, that'll add 9 new macros! And the number can grow...
What do you mean, it's just three new macros, like above. Where's 9?
3 for fieldno, 3 for field_name_len, 3 for key_count (WITHOUT OVERLAPS commit), and who knows -- I've also planned an flag in FOREIGN KEY support.
-- Yours truly, Nikita Malyavin
Hi, Nikita! On Feb 08, Nikita Malyavin wrote:
On Fri, Feb 8, 2019 at 2:18 AM Sergei Golubchik <serg@mariadb.org> wrote:
I thought you've wanted it a CHECK constraint. You've quoted the standard proving your point of view.
I've also quoted that it's name shouldn't match any existing constraint name:)
yes, we have this issue for all auto-generated names, and the usual workaround is to append _1, _2, etc until there's a free name.
diff --git a/sql/table.h b/sql/table.h
--- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
Of course! That's the way to say "This variable was stored as a 2-byte tuple".
I mean, you didn't remove the magic number. It's still 2, and if you'd like to change it to be 3, for example, you'd still need to go and change the code everywhere.
Oh, so You're really thinking on such refactoring, to change the sizes of metainfo types rapidly? Then that 👇makes sense:
No, I don't. Changing storage format is very heavy and thus very rare operation. I was just using it as an example, to show that embedding a magical number in the name of a constant doesn't make it any less magical or the code any easier to understand. "Easy to change" is a good rule, even if you don't actually plan to change :)
That's why I suggested something like
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
Oh no, that'll add 9 new macros! And the number can grow...
What do you mean, it's just three new macros, like above. Where's 9?
3 for fieldno, 3 for field_name_len, 3 for key_count (WITHOUT OVERLAPS commit), and who knows -- I've also planned an flag in FOREIGN KEY support.
sure, they're different entities, there is no inherent rule why they all should use the same storage width. So yes, 9 macros. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! On Fri, Feb 8, 2019 at 5:02 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
On Feb 08, Nikita Malyavin wrote:
On Fri, Feb 8, 2019 at 2:18 AM Sergei Golubchik <serg@mariadb.org> wrote:
I thought you've wanted it a CHECK constraint. You've quoted the standard proving your point of view.
I've also quoted that it's name shouldn't match any existing constraint name:)
yes, we have this issue for all auto-generated names, and the usual workaround is to append _1, _2, etc until there's a free name.
Okay, added to ALTER TABLE commit
diff --git a/sql/table.h b/sql/table.h
--- a/sql/table.h +++ b/sql/table.h @@ -1730,6 +1747,9 @@ class IS_table_read_plan; /** The threshold size a blob field buffer before it is freed */ #define MAX_TDC_BLOB_SIZE 65536
+/** number of bytes read by uint2korr and sint2korr */ +#define korr2size 2
No, that's silly. It's like #define TWO 2 Of course korr2size is 2. And korr3size is 3, and korr4size is 4.
Of course! That's the way to say "This variable was stored as a 2-byte tuple".
I mean, you didn't remove the magic number. It's still 2, and if you'd like to change it to be 3, for example, you'd still need to go and change the code everywhere.
Oh, so You're really thinking on such refactoring, to change the sizes of metainfo types rapidly? Then that 👇makes sense:
No, I don't. Changing storage format is very heavy and thus very rare operation.
I was just using it as an example, to show that embedding a magical number in the name of a constant doesn't make it any less magical or the code any easier to understand. "Easy to change" is a good rule, even if you don't actually plan to change :)
That's why I suggested something like
#define fieldno_size 2 #define fieldno_korr uint2korr #define fieldno_store int2store
Oh no, that'll add 9 new macros! And the number can grow...
What do you mean, it's just three new macros, like above. Where's 9?
3 for fieldno, 3 for field_name_len, 3 for key_count (WITHOUT OVERLAPS commit), and who knows -- I've also planned an flag in FOREIGN KEY support.
sure, they're different entities, there is no inherent rule why they all should use the same storage width. So yes, 9 macros.
Seems, the consensual solution is just to add size consts, one per category. I've updated the code. Are You good with that? -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik