maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

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 <serg@xxxxxxxxxxx> 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 security@xxxxxxxxxxx



-- 
All the best,

Aleksey Midenkov
@midenok