maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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 <bar@xxxxxxxxxxx>
>> 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<Item_hex_hybrid>(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@xxxxxxxxxxx
>