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