Hi, Nikita! On Jan 18, Nikita Malyavin wrote:
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.
In a similar case I used something like #define SIZE 2 #define uintSIZEkorr(X) uint2korr(X) This way there are no magically-looking numbers. You can even write `uint ## SIZE ## korr`, if you're feeling adventurous.
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?
You cannot have parts of an English phrase as a parameter. And "VIRTUAL" is just a subset of "GENERATED" anyway. So, just write my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, "GENERATED"); or, better, my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, "GENERATED ALWAYS AS");
+ 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
No, it's illogical. You have columns, like a TIMESTAMP(6) GENERATED ALWAYS AS (NOW() + INTERVAL 1 DAY), b TIMESTAMP(6) GENERATED ALWAYS AS ROW START for the first column the error is Period field %`s cannot be GENERATED for the second column the error is Period field %`s cannot be ROW START Why? They're both GENERATED. You don't say Period field %`s cannot be NOW() + INTERVAL 1 DAY See? So just say in both cases that the period cannot be "GENERATED ALWAYS AS". This is quite sufficient.
+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"
The standard clearly says there can be at most one application-time period. So "Application-time period already defined" is just as fine. When the new standard will allow for many application-time periods, we can rewrite the error message. But don't hold your breath. And, again, you cannot have "application" as a parameter. Regards, Sergei Chief Architect MariaDB and security@mariadb.org