Hi, Nikita!
Looks good. See few minor comments below
On Nov 28, Nikita Malyavin wrote:
> revision-id: ce4f8955d8d4a556f8d1a7b271da5f7de8a35c42 (versioning-1.0.7-2-gce4f8955d8d)
> parent(s): 9661692d3384e22ea6c5be1a8612feaa9605b0aa
> author: Nikita Malyavin <nikitamalyavin(a)gmail.com>
> committer: Nikita Malyavin <nikitamalyavin(a)gmail.com>
> timestamp: 2018-10-29 15:48:01 +1000
> message:
>
> MDEV-17082 Application period tables: CREATE
>
> * add syntax `CREATE TABLE ... PERIOD FOR <apptime>`
> * add table period entity
>
> diff --git a/include/mysql_com.h b/include/mysql_com.h
> index 902c0ff2706..5451db88b0b 100644
> --- a/include/mysql_com.h
> +++ b/include/mysql_com.h
> @@ -203,6 +203,7 @@ enum enum_indicator_type
> #define VERS_UPDATE_UNVERSIONED_FLAG (1 << 29) /* column that doesn't support
> system versioning when table
> itself supports it*/
> +#define EXPLICIT_NULL_FLAG (1 << 30) /* Field explicitly marked as NULL */
Not needed, see below
> #define REFRESH_GRANT (1ULL << 0) /* Refresh grant tables */
> #define REFRESH_LOG (1ULL << 1) /* Start on new log file */
> diff --git a/mysql-test/suite/period/r/create.result b/mysql-test/suite/period/r/create.result
> new file mode 100644
> index 00000000000..420eceb2e9a
> --- /dev/null
> +++ b/mysql-test/suite/period/r/create.result
> @@ -0,0 +1,88 @@
> +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`)
I agree it should be hidden. But I'd prefer you did it in this commit,
not in ALTER TABLE commit. You could've just committed it separately
and then used interactive rebase to fixup the CREATE TABLE commit.
Ok for now, but for the future if you realize your older commit had a
deficiency, please fixup the original commit, instead of fixing it
post-factum as a part of a later feature commit. As long as the old commit
isn't pushed, of course :)
> +) 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
> +# SQL17 p. 832
> +# 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 period not allowed
> +# SQL17 p. 831
> +# 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
> +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'
> +create or replace table t (id int primary key, s date, e date,
> +period for mytime(s,x));
> +ERROR HY000: Field `x` in PERIOD FOR `mytime` is not found in table
> +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
> +# SQL17 p. 831
> +# 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.
> +# CD1 and CD2 both contain either an explicit or an implicit
> +# <column constraint definition> that specifies
> +# NOT NULL NOT DEFERRABLE ENFORCED.
> +create or replace table t (id int primary key, s date NULL, e date,
> +period for mytime(s,e));
> +ERROR HY000: Period field `s` cannot be NULL
> +# SQL17 p.832
> +# 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 VIRTUAL or GENERATED
> +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 VIRTUAL or GENERATED
> +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 ROW START
> +# SQL17 p.832
> +# Let IDCN be an implementation-dependent <constraint name> that is not
> +# equivalent to the <constraint name> of any table constraint descriptor
> +# included in S.
Not enough context. It was quite difficult to find this quote.
When you're quoting the standard, please, specify the part name and the
section name. And there's no need to specify a page. For example:
SQL2016, Part 2, 11.27 <add table period definition>, General Rules, 2)a)
> +create or replace table t (x int, s date, e date,
> +period for mytime(s, e),
> +constraint mytime check (x > 1));
> +create or replace database test;
> diff --git a/sql/handler.h b/sql/handler.h
> index 8d244f2784e..69419901810 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -1906,46 +1906,61 @@ enum vers_sys_type_t
> VERS_TRX_ID
> };
>
> -struct Vers_parse_info
> +struct Table_period_info
> {
> - Vers_parse_info() :
> - check_unit(VERS_UNDEFINED),
> - versioned_fields(false),
> - unversioned_fields(false)
> - {}
> + Table_period_info() {}
> + Table_period_info(const char *name_arg, size_t size) :
> + name(name_arg, size) {}
> +
> + Lex_ident name;
>
> struct start_end_t
> {
> - start_end_t()
> - {}
> + start_end_t() {};
> - start_end_t(LEX_CSTRING _start, LEX_CSTRING _end) :
> + start_end_t(const LEX_CSTRING& _start, const LEX_CSTRING& _end) :
> start(_start),
> end(_end) {}
> Lex_ident start;
> Lex_ident end;
> };
> + start_end_t period;
>
> - start_end_t system_time;
> - start_end_t as_row;
> - vers_sys_type_t check_unit;
> + bool is_set() const
> + {
> + DBUG_ASSERT(bool(period.start) == bool(period.end));
> + return period.start;
> + }
>
> - void set_system_time(Lex_ident start, Lex_ident end)
> + void set_period(const Lex_ident& start, const Lex_ident& end)
> {
> - system_time.start= start;
> - system_time.end= end;
> + period.start= start;
> + period.end= end;
> }
> +};
> +
> +struct Vers_parse_info: public Table_period_info
> +{
> + Vers_parse_info() :
> + Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1),
We have a macro for that, use
Table_period_info(STRING_WITH_LEN("SYSTEM_TIME"))
> + check_unit(VERS_UNDEFINED),
> + versioned_fields(false),
> + unversioned_fields(false)
> + {}
> +
> + Table_period_info::start_end_t as_row;
> + vers_sys_type_t check_unit;
>
> protected:
> friend struct Table_scope_and_contents_source_st;
> void set_start(const LEX_CSTRING field_name)
> {
> as_row.start= field_name;
> - system_time.start= field_name;
> + period.start= field_name;
> }
> void set_end(const LEX_CSTRING field_name)
> {
> as_row.end= field_name;
> - system_time.end= field_name;
> + period.end= field_name;
> }
> bool is_start(const char *name) const;
> bool is_end(const char *name) const;
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index b04d869bf1d..4189131f801 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -3026,7 +3026,8 @@ CHARSET_INFO* get_sql_field_charset(Column_definition *sql_field,
> by adding the features DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP.
>
> If the first TIMESTAMP column appears to be nullable, or to have an
> - explicit default, or to be a virtual column, then no promition is done.
> + explicit default, or to be a virtual column, or to be part of table period,
> + then no promition is done.
promotion
>
> @param column_definitions The list of column definitions, in the physical
> order in which they appear in the table.
> diff --git a/sql/sql_lex.h b/sql/sql_lex.h
> index edb409d6354..aec83dc6f6a 100644
> --- a/sql/sql_lex.h
> +++ b/sql/sql_lex.h
> @@ -4185,6 +4185,30 @@ struct LEX: public Query_tables_list
> {
> return create_info.vers_info;
> }
> +
> + Table_period_info& get_table_period_info()
> + {
> + return create_info.period_info;
> + }
really? :) you've introduced a new method get_table_period_info
only to use it two lines below and nowhere else?
> +
> + int add_period(Lex_ident name, Lex_ident_sys_st start, Lex_ident_sys_st end)
> + {
> + Table_period_info &info= get_table_period_info();
> + if (info.is_set())
> + {
> + my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application");
> + return 1;
> + }
> + info.set_period(start, end);
> + info.name= name;
> +
> + Virtual_column_info *constr= new Virtual_column_info();
> + constr->expr= lt_creator.create(thd, create_item_ident_nosp(thd, &start),
> + create_item_ident_nosp(thd, &end));
> + add_constraint(&null_clex_str, constr, false);
> + return 0;
> + }
> +
> sp_package *get_sp_package() const;
>
> /**
> diff --git a/sql/table.cc b/sql/table.cc
> index ab1a04ccae4..87fa1ba4d61 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -1228,6 +1228,14 @@ static const Type_handler *old_frm_type_handler(uint pack_flag,
> }
>
>
> +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period,
> + const uchar *data)
> +{
> + period.start_fieldno= uint2korr(data);
> + period.end_fieldno= uint2korr(data + sizeof(uint16));
not "sizeof(uint16)" but 2. I don't think it's guaranteed that
sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes,
so data + 2 is always correct.
> + return period.start_fieldno >= fields || period.end_fieldno >= fields;
> +}
> +
> /**
> Read data from a binary .frm file image into a TABLE_SHARE
>
> @@ -1773,27 +1781,36 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
> }
>
> /* Set system versioning information. */
> + vers.name= Lex_ident(STRING_WITH_LEN("SYSTEM_TIME"));
> if (extra2.system_period == NULL)
> {
> versioned= VERS_UNDEFINED;
> - row_start_field= 0;
> - row_end_field= 0;
> + vers.start_fieldno= 0;
> + vers.end_fieldno= 0;
> }
> else
> {
> DBUG_PRINT("info", ("Setting system versioning informations"));
> - uint16 row_start= uint2korr(extra2.system_period);
> - uint16 row_end= uint2korr(extra2.system_period + sizeof(uint16));
> - if (row_start >= share->fields || row_end >= share->fields)
> + if (init_period_from_extra2(vers, extra2.system_period))
> goto err;
> - DBUG_PRINT("info", ("Columns with system versioning: [%d, %d]", row_start, row_end));
> + DBUG_PRINT("info", ("Columns with system versioning: [%d, %d]",
> + vers.start_fieldno, vers.end_fieldno));
> versioned= VERS_TIMESTAMP;
> vers_can_native= plugin_hton(se_plugin)->flags & HTON_NATIVE_SYS_VERSIONING;
> - row_start_field= row_start;
> - row_end_field= row_end;
> status_var_increment(thd->status_var.feature_system_versioning);
> } // if (system_period == NULL)
>
> + if (extra2.application_period.str)
> + {
> + period.name.length= extra2.application_period.length - 2*sizeof(uint16);
same
> + period.name.str= strmake_root(&mem_root,
> + (char*)extra2.application_period.str,
> + period.name.length);
> + const uchar *field_pos= extra2.application_period.str + period.name.length;
> + if (init_period_from_extra2(period, field_pos))
> + goto err;
> + }
> +
> for (i=0 ; i < share->fields; i++, strpos+=field_pack_length, field_ptr++)
> {
> uint interval_nr= 0, recpos;
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 4692b2d74d1..ca7e005455d 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -176,6 +170,7 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING *table,
> ulong data_offset;
> uint options_len;
> uint gis_extra2_len= 0;
> + uint period_info_len= create_info->period_info.name.length + 2*sizeof(uint16);
I won't comment on it any longer, but please everywhere s/sizeof(uint16)/2/g
> uchar fileinfo[FRM_HEADER_SIZE],forminfo[FRM_FORMINFO_SIZE];
> const partition_info *part_info= IF_PARTITIONING(thd->work_part_info, 0);
> bool error;
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 2ced4e959a2..a4c1684ccb8 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -7352,3 +7352,113 @@ bool Vers_parse_info::check_sys_fields(const Lex_table_name &table_name,
> "ROW END" : found_flag ? "ROW START" : "ROW START/END");
> return true;
> }
> +
> +static bool check_period_field(const Create_field* f, const char* name,
> + const char* period_name)
> +{
> + bool res= false;
> + if (!f)
> + {
> + my_error(ER_PERIOD_FIELD_NOT_FOUND, MYF(0), name, period_name);
> + res= true;
> + }
> + else if (f->type_handler()->mysql_timestamp_type() == MYSQL_TIMESTAMP_ERROR)
> + {
> + my_error(ER_WRONG_FIELD_SPEC, MYF(0), name);
> + res= true;
> + }
> + else if (f->vcol_info)
> + {
> + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> + f->field_name.str, "VIRTUAL or GENERATED");
just use "GENERATED", no English in the error message parameters, please.
> + res= true;
> + }
> + else if (f->flags & EXPLICIT_NULL_FLAG)
> + {
> + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
> + f->field_name.str, "NULL");
why is that? it's unrelated and inconsistent with cases like
create table t1 (a int null, primary key (a));
where a column is silently created NOT NULL.
So, you should ignore explicit NULL and create columns NOT NULL.
If you want to change this, it should be consistent in all cases.
create an MDEV, suggest there that the statement above should be an error.
but I don't think it's worth spending time on now.
> + res= true;
> + }
> + else if (f->flags & VERS_SYSTEM_FIELD)
> + {
> + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str,
> + f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END");
No need to have a special case for that. Use the same error as for vcol_info:
if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD)
{
my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0),
f->field_name.str, "GENERATED ALWAYS AS");
> + res= true;
> + }
> +
> + return res;
> +}
> +
> +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
> + my_error(ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD, MYF(0), f->field_name.str);
> + return true;
What does this error have to do with MDEV-16976?
SQL standard clearly says (part 2, 11.3 <table definition>, syntax rules, 2)d)
d) No <column name> in any <column definition> shall be equivalent to PN.
(here PN is "period name"). So MDEV-16976 or not, a period name should
never match a column name.
> + }
> + }
> +
> + res= check_period_field(row_start, period.start.str, period_info.name.str);
> + res= res || check_period_field(row_end, period.end.str, period_info.name.str);
> + if (res)
> + return true;
> +
> + if (row_start->type_handler() != row_end->type_handler()
> + || row_start->length != row_end->length)
> + {
> + my_error(ER_PERIOD_TYPES_MISMATCH, MYF(0), period_info.name.str);
> + res= true;
> + }
> +
> + return res;
> +}
> +
> +bool
> +Table_scope_and_contents_source_st::fix_create_fields(THD *thd,
> + Alter_info *alter_info,
> + const TABLE_LIST &create_table,
> + bool create_select)
> +{
> + if (vers_fix_system_fields(thd, alter_info, create_table, create_select))
> + return true;
> +
> + if (!period_info.name)
> + return false;
> +
> + Table_period_info::start_end_t &period= period_info.period;
> + List_iterator<Create_field> it(alter_info->create_list);
> + while (Create_field *f= it++)
> + {
> + if (period.start.streq(f->field_name) || period.end.streq(f->field_name))
> + {
> + f->period= &period_info;
> + if (!(f->flags & EXPLICIT_NULL_FLAG))
> + f->flags|= NOT_NULL_FLAG;
> + }
> + }
> + return false;
> +}
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 121eb0a0726..ceb3a58a427 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7924,3 +7924,24 @@ ER_ALTER_OPERATION_TABLE_OPTIONS_NEED_REBUILD
>
> ER_TRUNCATE_ILLEGAL_VERS
> eng "Cannot truncate a versioned table"
> +
> +ER_PERIOD_TEMPORARY_NOT_ALLOWED
> + eng "Temporary tables with application period not allowed"
"application time period"
> +
> +ER_PERIOD_FIELD_NOT_FOUND
> + eng "Field %`s in PERIOD FOR %`s is not found in table"
I don't think this needs a new error message, just use ER_BAD_FIELD_ERROR
> +
> +ER_PERIOD_TYPES_MISMATCH
> + eng "PERIOD FOR %`s fields types mismatch"
> +
> +ER_PERIOD_MAX_COUNT_EXCEEDED
better say ER_MORE_THAN_ONE_PERIOD or something...
"max count exceeded" suggests there is some not-trivial
(and possibly variable) max count
Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc.
"already defined" looks like a good one.
And I've already commented (in another patch) on using English
words as arguments - it will look like a bug when the error message is
translated
> + eng "cannot specify more than one %s-time period"
> +
> +ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD
> + eng "Could not specify name %`s for field. It is already used by period."
I don't think this needs a new error message, just use ER_DUP_FIELDNAME
> +
> +ER_PERIOD_FIELD_WRONG_ATTRIBUTES
> + eng "Period field %`s cannot be %s"
> +
> +ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD
> + eng "Field %`s is specified for more than one period"
This looks unused
Regards,
Sergei
Chief Architect MariaDB
and security(a)mariadb.org
