Re: [Maria-developers] Questions about AS OF (possibly bugs found)

[Sergei Golubchik]

Hi, Aleksey!

On Apr 11, Aleksey Midenkov wrote:
> Hi Sergei, Alexander!
> 
> Maybe it's better to remove TIMESTAMP/TRANSACTION auto-detection as it
> requires too much code to make it properly? And make TIMESTAMP by
> default.

I'd rather move it down when all items are fixed.
If possible.

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

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Questions about AS OF (possibly bugs found)

[Aleksey Midenkov]

Hi Sergei, Alexander!

Maybe it's better to remove TIMESTAMP/TRANSACTION auto-detection as it
requires too much code to make it properly? And make TIMESTAMP by
default.

On Mon, Apr 9, 2018 at 5:49 PM, Sergei Golubchik  wrote:
> Hi, Alexander!
>
> On Apr 09, Alexander Barkov wrote:
>>
>> Note, history_point adds around 20 shift/reduce conflicts.
>> I'd like to resolve them.
>
> good :)
>
>> > Might've been better to look at cmp_type() though.
>>
>> A new method in Type_handler would be even better.
>
> okay
>
>> >> 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?
>
> you do, please.
>
>> >> 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.
>
> I agree
>
>> Are you going to fix this? Is it reported?
>
> I don't think so.
>
>> 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();
>>   }
>
> makes sense
>
>> 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.
>
> this is clearly a bug too.
>
>> 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?
>
> I didn't look into it yet
>
>> >> 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?
>
> please do
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and secur...@mariadb.org



-- 
All the best,

Aleksey Midenkov
@midenok

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Questions about AS OF (possibly bugs found)

[Sergei Golubchik]

Hi, Alexander!

On Apr 09, Alexander Barkov wrote:
> 
> Note, history_point adds around 20 shift/reduce conflicts.
> I'd like to resolve them.

good :)

> > Might've been better to look at cmp_type() though.
> 
> A new method in Type_handler would be even better.

okay

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

you do, please.

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

I agree

> Are you going to fix this? Is it reported?

I don't think so.

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

makes sense

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

this is clearly a bug too.

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

I didn't look into it yet

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

please do

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

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Questions about AS OF (possibly bugs found)

[Alexander Barkov]

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 secur...@mariadb.org
> 

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Questions about AS OF (possibly bugs found)

[Sergei Golubchik]

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 secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp