maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hello Sergei,


On 04/09/2018 01:09 PM, Sergei Golubchik wrote:
> Hi, Alexander!
> 
> On Apr 09, Alexander Barkov wrote:
>> Hi Sergei,
>>
>> I'm looking at this use rule in sql_yacc.yy:
>>
>> history_point:
>>           temporal_literal
>>           { $$= Vers_history_point(VERS_TIMESTAMP, $1); }
>>         | function_call_keyword_timestamp
>>           { $$= Vers_history_point(VERS_TIMESTAMP, $1); }
>>         | opt_history_unit simple_expr
>>           { $$= Vers_history_point($1, $2); }
>>         ;
>>
>> AS OF DATE 'xxx'    - returns Vers_history_point(VERS_TIMESTAMP)
>> AS OF DATE('xxx')   - returns Vers_history_point(VERS_UNDEFINED)
> 
> I suspect that's ok, because with VERS_UNDEFINED it'll look at the
> result_type() and will change to VERS_TIMESTAMP.

Note, history_point adds around 20 shift/reduce conflicts.
I'd like to resolve them.

> 
> Might've been better to look at cmp_type() though.

A new method in Type_handler would be even better.

> 
>> AS OF TIMESTAMP('2001-01-01') - returns Vers_history_point(VERS_TIMESTAMP)
>> AS OF COALESCE(TIMESTAMP('2001-01-01')) - returns Vers_history_point(VERS_UNDEFINED)
> 
> Same.
> 
>> Perhaps this is not expected.
>>
>> Also, this code looks suspicious:
>>
>> void Vers_history_point::resolve_unit(bool timestamps_only)
>> {
>>   if (item && unit == VERS_UNDEFINED)
>>   {
>>     if (item->type() == Item::FIELD_ITEM || timestamps_only)
>>       unit= VERS_TIMESTAMP;
>>     else if (item->result_type() == INT_RESULT ||
>>              item->result_type() == REAL_RESULT)
>>       unit= VERS_TRX_ID;
>>     else
>>       unit= VERS_TIMESTAMP;
>>   }
>> }
>>
>> Why DECIMAL_RESULT is not handled?
> 
> Looks like a bug. I think it should be.

Should I report a bug, or will you do?

> 
>> Why only Item::FIELD_ITEM is checked?
> 
> https://github.com/tempesta-tech/mariadb/issues/397
> 
> Vers_history_point::resolve_unit is called too early, fields are not
> fixed yet, so item->result_type() crashed.

What about other Item types?
Their result_type() methods is called in non-fixed state.
I'd say this is a bug.

Are you going to fix this? Is it reported?


Btw, this makes me think that a DBUG_ASSERT
must be added into Field::result_type(), like this:


  Item_result result_type() const
  {
    DBUG_ASSERT(fixed);  --- to be added
    return type_handler()->result_type();
  }


> 
>> Can't "history_point" be simplified to something like:
>>
>> history_point:
>>     simple_expr { $$= Vers_history_point(VERS_UNDEFINED, $1); }
>>   | TRANSACTION_SYM simple_expr { $$= Vers_history_point(VERS_TRX_ID, $2); }
>>   ;


So, because of the problem with result_type() being called
in non-fixed state, we have a rule "history_point" which catches
most useful constant and functions and resolves them immediately
to VERS_TIMESTAMP, while other Item times are resolved in
Vers_history_point::resolve_unit().

In case of a fixed-type Items, for example INT and REAL functions,
they get resolved as VERS_TRX_ID.

Hybrid type Items, e.g. function COALESCE or CASE,
in non-fixed state return REAL_RESULT. So they
get resolved as VERS_TRX_ID. Even if their appear
to be of a temporal data type later.

I propose
1. Either we fix the code to call resolve_unit() in fixed state.
2. Or, if it's too hard, at least temporary disallow hybrid data type
Items to be used in AS OF.

Is N1 doable quickly?

If we go N2, a new method "virtual const Type_handler
*Item::fixed_result_type_handler()" can be added.
It can result a non-NULL pointer for Items with known data type,
whose result_type() does not change on fix_fields(),
and a NULL pointer for hybrid Item types which need fix_fields()
to know result_type(). If the new method returns NULL,
just throw an error about a bad AS OF expression.


>>
>> I'm saying "something like" because TRANSACTION_SYM  will cause a
>> conflict with simple_expr. So probably it should be cought somehow else,
>> by catching Item_ident and checking it name.
>>
>> Btw, an SP variable with name "TRANSACTION" does not work well:
> 
> Good point. That's a bug.

Should I report a bug, or will you do?

> 
> Now  I can think of more ways of introducing the identifier
> TRANSACTION there, like
> 
>  CREATE TABLE t1 (TRANSACTION int);
>  CREATE TABLE t2 (...) WITH SYSTEM VERSIONING;
>  ...
>  SELECT (SELECT * FROM t1 FOR SYSTEM TIME AS OF TRANSACTION) FROM t1;

Good catch :)



> 
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>