maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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.

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

> 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.

> 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.

> 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); }
>   ;
> 
> 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.

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;

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx