Re: [Maria-developers] ce4f8955d8d: MDEV-17082 Application period tables: CREATE
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
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.
Thanks for advice! Looks suitable enough I will just make #define korr2size 2. But it should be shared between
Hi, Sergei! On Sat, Jan 19, 2019 at 10:14 AM Sergei Golubchik <serg@mariadb.org> wrote: table.cc and unireg.cc. Can You suggest me where to put it?
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,
The another problem is that sizeof(uint16) is already used in system please.
VIRTUAL/GENERATED? What'd You say?
You cannot have parts of an English phrase as a parameter.
Well, I agree that 'slash' ('/') is a sintactic element, but was considering it language-independent. Ok then.
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.
See. Thanks for exhaustive description. That's done
+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.
Yes, already fixed that. -- Yours truly, Nikita Malyavin
Hi, Nikita! On Jan 30, Nikita Malyavin wrote:
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.
Thanks for advice! Looks suitable enough I will just make #define korr2size 2. But it should be shared between table.cc and unireg.cc. Can You suggest me where to put it?
As you like. table.h, perhaps?
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.
Well, I agree that 'slash' ('/') is a sintactic element, but was considering it language-independent. Ok then.
Sorry, I misunderstood. I meant that you should not write "VIRTUAL or GENERATED" because it's English. If you'd like to write "VIRTUAL/GENERATED" - that's language independent, and from that point of view it's fine. But writing "VIRTUAL/GENERATED" is like writing "apples/fruits" or "dogs/animals". VIRTUAL and GENERATED are not two alternatives. VIRTUAL is a specialization of GENERATED. Every VIRTUAL column is necessarily GENERATED. And any GENERATED column is necessarily either VIRTUAL or PERSISTENT. That's why I said there's no need to write "VIRTUAL/GENERATED", it's enough to say simply "GENERATED", because it covers all possibilities. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! On Wed, Jan 30, 2019 at 7:33 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
On Jan 30, Nikita Malyavin wrote:
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.
Thanks for advice! Looks suitable enough I will just make #define korr2size 2. But it should be shared between table.cc and unireg.cc. Can You suggest me where to put it?
As you like. table.h, perhaps?
Sounds good ✅
Thanks for the review! -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik