Hi Sergei, On 05/14/2018 05:44 PM, Sergei Golubchik wrote:

Hi, Alexander!

Looks ok. A couple minor comments:

Thanks for the review. Please find answers to your comments below:

On May 07, Alexander Barkov wrote:

...

commit fa6cf08ad1c01e990cd6a497a0403c1bbfe0b27e Author: Alexander Barkov Date: Mon May 7 16:59:34 2018 +0400

MDEV-16094 Crash when using AS OF with a stored function MDEV-16100 FOR SYSTEM_TIME erroneously resolves string user variables as transaction IDs

diff --git a/sql/item.h b/sql/item.h index e1a8906..3759693 100644 --- a/sql/item.h +++ b/sql/item.h @@ -4212,6 +4216,10 @@ class Item_hex_hybrid: public Item_hex_constant { return &type_handler_longlong; } + const Type_handler *type_handler_for_system_time() const

why new method? you could've used cast_to_int_type_handler() looks like it has the same semantics.

Implementations of the top level Item has a subtle difference: virtual const Type_handler *cast_to_int_type_handler() const { return type_handler(); } virtual const Type_handler *type_handler_for_system_time() const { return real_type_handler(); } Notice, it's type_handler() vs real_type_handler(). The difference is that cast_to_int_type_handler() does not need to distinguish between ENUM/SET and VARCHAR, while type_handler_for_system_time() does. Later we'll probably add a dedicated Type_handler_hex_hybrid. So cast_to_int_type_handler() and type_handler_for_system_type() will likely disappear in Item. But this needs some efforts.

...

+ { + return &type_handler_longlong; + } void print(String *str, enum_query_type query_type); Item *get_copy(THD *thd) { return get_item_copy(thd, this); } diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 320e631..04abf54 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -839,11 +839,13 @@ int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables)

if (vers_conditions) { + thd->where= "for system_time clause";

better "FOR SYSTEM_TIME". Compare

ERROR 42S22: Неизвестный столбец 'now' в 'for system_time clause' ERROR 42S22: Неизвестный столбец 'now' в 'FOR SYSTEM_TIME'

Good idea. Thanks. I was trying to be inline with what we have in other places: sql_base.cc: thd->where= "from clause"; sql_base.cc: thd->where="on clause"; sql_base.cc: thd->where="where clause"; sql_select.cc: thd->where= "order clause"; sql_select.cc: thd->where="having clause"; sql_select.cc: thd->where="order clause"; But your way is more readable. Done.

...

/* TODO: do resolve fix_length_and_dec(), fix_fields(). This requires storing vers_conditions as Item and make some magic related to vers_system_time_t/VERS_TRX_ID at stage of fix_fields() (this is large refactoring). */ - vers_conditions.resolve_units(timestamps_only); + if (vers_conditions.resolve_units(thd)) + DBUG_RETURN(-1); if (timestamps_only && (vers_conditions.start.unit == VERS_TRX_ID || vers_conditions.end.unit == VERS_TRX_ID)) {

Regards, Sergei Chief Architect MariaDB and security@mariadb.org