[Maria-developers] Questions about AS OF (possibly bugs found)
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) 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) 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? Why only Item::FIELD_ITEM is checked? 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: This script works fine: DELIMITER $$ BEGIN NOT ATOMIC DECLARE TRANSACTION int DEFAULT 10; SELECT TRANSACTION; END; $$ DELIMITER ; This script: DELIMITER $$ BEGIN NOT ATOMIC DECLARE TRANSACTION int DEFAULT 10; SELECT * FROM t1 FOR SYSTEM_TIME AS OF TRANSACTION; END; $$ DELIMITER ; returns an error: ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '; Note, if I rename "TRANSACTION" to "TRANSACTION1", it works fine. Thanks!
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@mariadb.org
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@mariadb.org
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@mariadb.org
Hi Aleksey, You added Type_handler_hybrid_field_type::m_vers_trx_id. Is it really needed? It seems to be always "false". So this code in aggregate_for_comparison() seems to be a dead code: if (m_vers_trx_id && (a == STRING_RESULT || b == STRING_RESULT)) m_type_handler= &type_handler_datetime; Can I remove m_vers_trx_id and this dead code? Thanks!
Hi Alexander! On Tue, Apr 10, 2018 at 12:15 PM, Alexander Barkov <bar@mariadb.com> wrote:
Hi Aleksey,
You added Type_handler_hybrid_field_type::m_vers_trx_id.
Is it really needed? It seems to be always "false". So this code in aggregate_for_comparison() seems to be a dead code:
if (m_vers_trx_id && (a == STRING_RESULT || b == STRING_RESULT)) m_type_handler= &type_handler_datetime;
Can I remove m_vers_trx_id and this dead code?
Sure, please remove. Thanks!
Thanks!
Hi Aleksey, Sergei, On 04/11/2018 01:34 AM, Aleksey Midenkov wrote:
Hi Alexander!
On Tue, Apr 10, 2018 at 12:15 PM, Alexander Barkov <bar@mariadb.com> wrote:
Hi Aleksey,
You added Type_handler_hybrid_field_type::m_vers_trx_id.
Is it really needed? It seems to be always "false". So this code in aggregate_for_comparison() seems to be a dead code:
if (m_vers_trx_id && (a == STRING_RESULT || b == STRING_RESULT)) m_type_handler= &type_handler_datetime;
Can I remove m_vers_trx_id and this dead code?
Sure, please remove. Thanks!
Please find a patch attached. It does the following: 1. Makes Field_vers_trx_id::type_handler() return &type_handler_vers_trx_id rather than &type_handler_longlong. Fixes Item_func::convert_const_compared_to_int_field() to test field_item->type_handler() against &type_handler_vers_trx_id, instead of testing field_item->vers_trx_id(). 2. Removes VERS_TRX_ID related code from Type_handler_hybrid_field_type::aggregate_for_comparison(), because "BIGINT UNSIGNED GENERATED ALWAYS AS ROW {START|END}" columns behave just like a BIGINT in a regular comparison, i.e. when not inside AS OF. 3. Removes - Type_handler_hybrid_field_type::m_vers_trx_id; - Type_handler_hybrid_field_type::m_flags; because a "BIGINT UNSIGNED GENERATED ALWAYS AS ROW {START|END}" behaves like a regular BIGINT column when in UNION. 4. Removes Field::vers_trx_id(), Item::vers_trx_id(), Item::field_flags() They are not needed anymore. See N1. All tests pass. Thanks!
Thanks!
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@mariadb.org> 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@mariadb.org
-- All the best, Aleksey Midenkov @midenok
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 security@mariadb.org
participants (3)
-
Aleksey Midenkov -
Alexander Barkov -
Sergei Golubchik