---------- Forwarded message ---------
From: Nikita Malyavin
+# 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)
Fixed, as well as all the rest cites
+ Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1),
We have a macro for that, use
Table_period_info(STRING_WITH_LEN("SYSTEM_TIME"))
✅
+ then no promition is done.
promotion ✅
+ 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?
Not really, of course. But agree, it's not worth to have such a shortcut.
+ + 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.
Yes, it's correct, and I've fixed it, but it wasn't about coorrectness -- but readability. Now what 4, or what is 8 -- is the question that the reader could сonsider. This numbers look magical. With using sizeof(uint16) the one could at least understand, that this is about several double-byte words read. The another problem is that sizeof(uint16) is already used in system versioning, few lines below, so the resulting code will look patchy here.
+ 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.
VIRTUAL/GENERATED? What'd You say?
+ 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.
Ok, I understand. Fixed.
+ 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");
Looks not very much user-friendly I think. Let's leave the verbosity as it was
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.
I was thinking to implement it through virtual column. Then this check will become redundant, and we'll get duplicate field error
+ 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
I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods designed in the way to support a possibility of several periods definition, so i like to treat it as "N periods max supported, but now N=1"
+ 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 ✅
Sincerely Nikita Malyavin