Hi,
If any dev here is interested in the topic of ensuring the major
version upgrades (and essentially running mariadb-upgrade) continues
to work flawlessly and regressions are less likely to surface
unnoticed, please review and consider adding your approvals to
https://github.com/MariaDB/server/pull/3503 and share feedback on WIP
https://github.com/MariaDB/server/pull/3563.
Thanks!
Hi Aleksey,
Thanks for the patch. Please see below for some initial review comments.
There are some todos for myself noted below which I will look into
tomorrow.
> From b4465fc6b51dfc4e645569201e6026ec6405367b Mon Sep 17 00:00:00 2001
> From: Aleksey Midenkov <midenok(a)gmail.com>
> Date: Mon, 30 Sep 2024 22:26:51 +0300
> Subject: [PATCH] MDEV-28413 System versioning is incorrect in Spider
> Basic functioning of system versioning in Spider: UPDATE, DELETE,
> DELETE …
[View More]HISTORY, AS OF, FROM .. TO, BETWEEN .. AND.
> More testing should be done for:
> LOAD DATA, joins, derived tables, multi-update/delete and what else?
> Note that we are still using HA_CAN_DIRECT_UPDATE_AND_DELETE, but not
> for periods in DELETE (fwiw there is condition
> !table_list->has_period(), but it is missing in UDPATE).
> ---
> .../mysql-test/spider/r/versioning.result | 142 ++++++++++++++++
> .../mysql-test/spider/t/versioning.test | 151 ++++++++++++++++++
> storage/spider/spd_db_conn.cc | 19 ++-
> storage/spider/spd_db_include.h | 1 +
> storage/spider/spd_db_mysql.cc | 25 ++-
> 5 files changed, 335 insertions(+), 3 deletions(-)
> create mode 100644 storage/spider/mysql-test/spider/r/versioning.result
> create mode 100644 storage/spider/mysql-test/spider/t/versioning.test
> [... 149 lines elided]
> diff --git a/storage/spider/mysql-test/spider/t/versioning.test b/storage/spider/mysql-test/spider/t/versioning.test
> new file mode 100644
> index 00000000000..6093169bff4
> --- /dev/null
> +++ b/storage/spider/mysql-test/spider/t/versioning.test
> @@ -0,0 +1,151 @@
> +--source include/have_innodb.inc
> +--source suite/versioning/common.inc
> +
> +install plugin spider soname 'ha_spider';
For init the spider plugin in spider tests we commonly do
--disable_query_log
--disable_result_log
--source ../t/test_init.inc
--enable_result_log
--enable_query_log
> +set spider_same_server_link= on;
> +
> +--replace_result $MASTER_1_MYPORT MASTER_1_MYPORT
> +eval create server s foreign data wrapper mysql options
> +(host '127.0.0.1', database 'test', user 'root', port $MASTER_1_MYPORT);
Use evalp instead of eval and you can save the --replace_result line
> +
> +
> +create table t1 (
> + x int, y int
> +) with system versioning;
> +
> +create table t2 (
> + x int, y int,
> + row_start timestamp(6) as row start invisible,
> + row_end timestamp(6) as row end invisible,
> + period for system_time (row_start, row_end)
> +) with system versioning;
> +
> +create or replace table t1_sp (
> + x int, y int)
> +engine=spider comment='wrapper "mysql", srv "s", table "t1"'
> +with system versioning;
> +
> +create or replace table t2_sp (
> + x int, y int,
> + row_start timestamp(6) as row start invisible,
> + row_end timestamp(6) as row end invisible,
> + period for system_time (row_start, row_end))
> +engine=spider comment='wrapper "mysql", srv "s", table "t2"'
> +with system versioning;
> +
> +--echo # Timestamps are not propagated (subject of MDEV-16546, but not for implicit system fields)
> +set timestamp= unix_timestamp('2000-01-01 00:00:00');
I tried executing directly using
select spider_direct_sql('set timestamp= unix_timestamp("2000-01-01 00:00:00");', '', 'srv "s"');
But then I get extra rows in subsequent select queries and I think it is
caused by the timezones. It could be a separate task to fix this, but we
should warn users about it.
> +insert into t1_sp values (1, 1), (2, 2);
It may be obvious to you, but I find the timestamps at times hard to
keep track of, so I suggest adding a comment to each DML statement
indicating the time, as well as the select row_start / row_end
statements. For example, change the line above to
insert into t1_sp values (1, 1), (2, 2); # T1
> +insert into t2_sp values (1, 1), (2, 2);
and change this line to
insert into t2_sp values (1, 1), (2, 2); # T2
> +--sleep 0.01
> +
> +set timestamp= unix_timestamp('2000-01-01 00:11:11');
> +--echo # check dml generating history
> +delete from t1_sp where x = 1;
add # T3
> +--sleep 0.01
> +delete from t2_sp where x = 1;
# T4
> +--sleep 0.01
> +
> +select row_end into @t1 from t2 for system_time all where x = 1;
And so we can annotate this line with the timestamp above, like so:
select row_end into @t1 from t2 for system_time all where x = 1; # @t1 = T4
> +
> +update t1_sp set y= y + 1;
# T5
> +select row_start into @t2 from t1 where x = 2;
# @t2 = T5
> +--sleep 0.01
> +update t2_sp set y= y + 1;
# T6
With the annotations I suggested, results from the following block of
selects becomes easier to understand.
> +
> +select * from t1_sp;
> +select * from t2_sp;
> +select * from t1_sp for system_time as of timestamp @t1;
> +select * from t2_sp for system_time as of timestamp @t1;
> +select * from t1_sp for system_time as of timestamp @t2;
> +select * from t2_sp for system_time as of timestamp @t2;
> +select * from t1_sp for system_time from timestamp @t1 to timestamp @t2;
> +select * from t2_sp for system_time from timestamp @t1 to timestamp @t2;
> +select * from t1_sp for system_time between timestamp @t1 and timestamp @t2;
> +select * from t2_sp for system_time between timestamp @t1 and timestamp @t2;
> +
> +--echo # spider cannot call functions, it fails in spider_create_group_by_handler()
> + # at:
> + #
> + # 1555 if (spider_db_print_item_type(item, null, spider, null, null, 0,
> + # 1556 roop_count, true, fields_arg))
> + # 1557 {
> + # 1558 dbug_print("info",("spider dbton_id=%d can't create select", roop_count));
> + # 1559 spider_clear_bit(dbton_bitmap, roop_count);
> + # 1560 keep_going = false;
> + # 1561 break;
> + # 1562 }
> + #
This is fine. The Spider group-by-handler (GBH) is not meant to handle
every situation. Either remove this block starting from the --echo, or
make it more succinct by replacing it with something like (Note that GBH
supports certain functions like trim):
# The spider group-by-handler does not support the check_row function
> +--echo # but somehow it doesn't fail the query and continues without FOR SYSTEM_TIME ALL clause
> +select *, check_row(row_start, row_end) from t1_sp for system_time all order by row_end, y, x;
> +select *, check_row(row_start, row_end) from t2_sp for system_time all order by row_end, y, x;
I will check why the execution without the group-by-handler results in
wrong results.
> +--echo # SELECT from base table works as usual:
> +select *, check_row(row_start, row_end) from t1 for system_time all order by row_end, y, x;
> +select *, check_row(row_start, row_end) from t2 for system_time all order by row_end, y, x;
> +
> +--echo # here it works nice (append_table_list() has the chance to add FOR SYSTEM_TIME ALL)
> +
> +# Usage: SHOW_TIMESTAMPS=1 mtr versioning.spider
Shouldn't the test name be spider.versioning?
> +if ($SHOW_TIMESTAMPS)
> +{
> + select @t1, @t2;
> +}
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t1_sp for system_time all order by row_end, y, x;
> +
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t2_sp for system_time all order by row_end, y, x;
> +
> +delete history from t1_sp before system_time @t1;
> +delete history from t2_sp before system_time @t1;
> +
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t1_sp for system_time all order by row_end, y, x;
> +
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t2_sp for system_time all order by row_end, y, x;
> +
> +select * from t1 for system_time all;
> +select * from t2 for system_time all;
> +
> +delete history from t1_sp;
> +delete history from t2_sp;
> +
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t1_sp for system_time all order by row_end, y, x;
> +
> +if (!$SHOW_TIMESTAMPS)
> +{
> +--replace_regex /\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d.\d\d\d\d\d\d/TIMESTAMP/
> +}
> +select *, row_start, row_end from t2_sp for system_time all order by row_end, y, x;
> +
> +select * from t1 for system_time all;
> +select * from t2 for system_time all;
> +
> +drop tables t1, t2, t1_sp, t2_sp;
> +set timestamp= default;
> +--source suite/versioning/common_finish.inc
> +
> +uninstall plugin spider;
> +drop table mysql.spider_link_failed_log, mysql.spider_link_mon_servers, mysql.spider_tables, mysql.spider_table_crd, mysql.spider_table_position_for_recovery, mysql.spider_table_sts, mysql.spider_xa, mysql.spider_xa_failed_log, mysql.spider_xa_member;
> +drop function spider_direct_sql;
> +drop function spider_bg_direct_sql;
> +drop function spider_ping_table;
> +drop function spider_copy_tables;
> +drop function spider_flush_table_mon_cache;
For deinit the spider plugin in spider tests we commonly do
--disable_query_log
--disable_result_log
--source ../t/test_deinit.inc
--enable_result_log
--enable_query_log
Please replace the lines from "uninstall plugin spider;" to the end of
the test with this block.
> diff --git a/storage/spider/spd_db_conn.cc b/storage/spider/spd_db_conn.cc
> index 8750e87237e..896fce8adf5 100644
> --- a/storage/spider/spd_db_conn.cc
> +++ b/storage/spider/spd_db_conn.cc
> @@ -1515,9 +1515,8 @@ int spider_db_append_key_where_internal(
> ) {
> SPIDER_RESULT_LIST *result_list = &spider->result_list;
> SPIDER_SHARE *share = spider->share;
> -#ifndef DBUG_OFF
> TABLE *table = spider->get_table();
> -#endif
Why remove the #ifndef and #endif?
> [... 99 lines elided]
Best,
Yuchen
[View Less]