Hi Aleksey, Please see below for further review comments. On Wed 2024-10-02 17:44:36 +1000, Yuchen Pei wrote:
[... 90 lines elided]
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.
I mean warn users about it in the spider section of the knowledge base.
[... 66 lines elided]
+--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
Scratch that. I did some debugging and understand that the check_rows function is actually an UDF, so this can be fixed by issuing set spider_use_pushdown_udf=1; before issuing the select statements calling check_row(). Please update the test with this and remove the comment block starting with the --echo # spider cannot call functions...
+--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.
I checked this, and it is caused by a missing "for system_time all" in the spider query construction when the sql layer calls rnd_next() on the handler: --8<---------------cut here---------------start------------->8--- 5 in spider_mbase_handler::append_minimum_select_part of /home/ycp/source/mariadb-server/10.5/src/storage/spider/spd_db_mysql.cc:8949 6 in ha_spider::append_minimum_select_sql_part of /home/ycp/source/mariadb-server/10.5/src/storage/spider/ha_spider.cc:10231 7 in spider_db_append_select_columns of /home/ycp/source/mariadb-server/10.5/src/storage/spider/spd_db_conn.cc:1366 8 in ha_spider::rnd_next_internal of /home/ycp/source/mariadb-server/10.5/src/storage/spider/ha_spider.cc:5161 9 in ha_spider::rnd_next of /home/ycp/source/mariadb-server/10.5/src/storage/spider/ha_spider.cc:5298 10 in handler::ha_rnd_next of /home/ycp/source/mariadb-server/10.5/src/sql/handler.cc:3187 11 in find_all_keys of /home/ycp/source/mariadb-server/10.5/src/sql/filesort.cc:904 12 in filesort of /home/ycp/source/mariadb-server/10.5/src/sql/filesort.cc:352 13 in create_sort_index of /home/ycp/source/mariadb-server/10.5/src/sql/sql_select.cc:24709 14 in st_join_table::sort_table of /home/ycp/source/mariadb-server/10.5/src/sql/sql_select.cc:22314 15 in join_init_read_record of /home/ycp/source/mariadb-server/10.5/src/sql/sql_select.cc:22253 16 in sub_select of /home/ycp/source/mariadb-server/10.5/src/sql/sql_select.cc:21307 --8<---------------cut here---------------end--------------->8--- A quick fix could be: --8<---------------cut here---------------start------------->8--- modified storage/spider/spd_db_mysql.cc @@ -7713,6 +7713,12 @@ int spider_mbase_handler::append_table_name_with_adjusting( } else { error_num = mysql_share->append_table_name_with_adjusting(str, spider->conn_link_idx[link_idx]); + const auto& vers_conditions= + spider->get_table()->pos_in_table_list->vers_conditions; + if (spider->wide_handler->sql_command == SQLCOM_SELECT && + (vers_conditions.type == SYSTEM_TIME_ALL || + vers_conditions.orig_type == SYSTEM_TIME_ALL)) + str->append(STRING_WITH_LEN(" for system_time all ")); } DBUG_RETURN(error_num); } --8<---------------cut here---------------end--------------->8--- It looks a bit ad-hoc, but for the purpose of this patch it may be sufficient. One should also check for potential segv in spider or wide_handler. The condition spider->wide_handler->sql_command == SQLCOM_SELECT is needed because the DELETE statement delete from t1_sp where x = 1 also has vers_conditions.type == SYSTEM_TIME_ALL (why?)
[... 98 lines elided]
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?
I see it is because table is used the next line which I missed. My apologies - please disregard this comment.
[... 4 lines elided]
BTW, the part of the patch mentioning MDEV-16546 such as in spider_mbase_handler::append_insert_values(), spider_mbase_handler::append_into(), and the set timestamp... issue mentioned in my previous email could be part of one separate task that fix the timestamp timezone issue. Best, Yuchen