Details
-
Task
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
None
Description
The following ha_spider methods inside ha_spider.cc call spider_check_and_init_casual_read()
ft_read_internal multi_range_read_next_first
index_first_map_internal multi_range_read_next_next
index_last_internal range_read_first_internal
index_read_last_map_internal rnd_next_internal
index_read_map_internal
Near the callsite is about 80 lines of repeated code. Part of the repeated region is also a common pattern found in other spider files and functions. They need to be factored out.
See the attached gif for an IDE demo, as well as attached .cc files with these functions extracted out (at 11.0 941f91edbc3d3a0a184b9bfc58d5e4042c4b65f1).
Attachments
Issue Links
- relates to
-
-
- Closed
-
-
-
- Closed
-
Activity
Various stacks proved to be further variations of MDEV-27902, ref this comment.
FYI, this comment shows a new stack, for a previously known bug, in MDEV-31788 10.5 opt builds. This may require review.
holyfoot, thanks for the review.
Roel, can you test this change on 10.5, 10.6, 10.11, 11.1? See my previous comment for which commit to test. For example for 10.5 it is 13ca1ce565c. If you want to apply these commits to the latest main branches to test please let me know.
The commits also contain changes for MDEV-26178, so by testing them you'll be testing that ticket too.
They also happen to contain one cleanup commit for MDEV-32627.
Hi holyfoot, ptal thanks. The commits not marked with MDEV-31788 below are cleanups useful for fixing this task. The refactoring is not complete, but good enough for now imo given the legacy-ness of the code, and can be further improved over time.
13ca1ce565c upstream/bb-10.5-mdev-31788 upstream/bb-10.5-ycp MDEV-31788 Factor functions to reduce duplication around spider_check_and_init_casual_read in ha_spider.cc
|
1d19b2c4ef5 MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc
|
23f84250e23 MDEV-31788 Factor spider locking and unlocking code around sending queries
|
4de73931ea1 MDEV-28360 Spider: remove #ifdef SPIDER_use_LEX_CSTRING_for_KEY_Field_name
|
deef6c92acc MDEV-27643 Spider: remove #ifdef HA_CAN_BULK_ACCESS
|
765ea58f4dd MDEV-31788 Remove spider_file_pos
|
As expected, applying the changes to 11.1 is trickier, especially resolving the conflicts in spider_mbase_handler::show_table_status() and spider_mbase_handler::show_index() between a previous refactoring and the one in this ticket. Here are the 11.1 commits where the conflicts are resolved by overwriting the previous refactoring with that of this ticket:
c782e1f3daf upstream/bb-11.1-mdev-31788 MDEV-31788 Factor functions to reduce duplication around spider_check_and_init_casual_read in ha_spider.cc
|
72e54d928d7 MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc
|
a46fef08f72 MDEV-31788 Factor spider locking and unlocking code around sending queries
|
ce0e351f0ff MDEV-31788 Remove spider_file_pos
|
Roel: like MDEV-26178, after approval it would be good to test this one on at least 10.5 and 11.1 as well before we push the changes.
For reference, here are the 10.11 and 10.6 versions
f2e893c6cdf upstream/bb-10.11-mdev-31788 MDEV-31788 Factor functions to reduce duplication around spider_check_and_init_casual_read in ha_spider.cc
|
ff821ce8895 MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc
|
41ef6d6673f MDEV-31788 Factor spider locking and unlocking code around sending queries
|
40ab8b17d6e MDEV-31788 Remove spider_file_pos
|
104dbad2368 upstream/bb-10.6-mdev-31788 MDEV-31788 Factor functions to reduce duplication around spider_check_and_init_casual_read in ha_spider.cc
|
3329434acea MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc
|
1a44fd0263c MDEV-31788 Factor spider locking and unlocking code around sending queries
|
6d05c5cb4eb MDEV-28360 Spider: remove #ifdef SPIDER_use_LEX_CSTRING_for_KEY_Field_name
|
629fc724437 MDEV-27643 Spider: remove #ifdef HA_CAN_BULK_ACCESS
|
d2f1b953bef MDEV-31788 Remove spider_file_pos
|
The cleanup is a bit more involved than I expected which is why I opened this ticket separately from MDEV-31787.
We can just factor out the portion in the gif, but it is not a "logical" block by it self, but rather a block before a for loop, and the if branch inside the for loop. There are (often minor) differences in the else branches, which may not be real differences, but require a bit more effort to the cleanup properly.
For example, I suspect the following are not real differences, and just redundancy in data structure
--- /home/ycp/source/mariadb-server/mdev-26151/repeats/rnd_next_internal.cc 2023-07-28 12:49:48.384561252 +1000
|
+++ /home/ycp/source/mariadb-server/mdev-26151/repeats/ft_read_internal.cc 2023-07-28 12:49:56.304500405 +1000
|
} else {
|
+ uint dbton_id = share->use_sql_dbton_ids[roop_count];
|
+ spider_db_handler *dbton_hdl = dbton_handler[dbton_id];
|
SPIDER_CONN *conn = conns[roop_count];
|
- ulong sql_type;
|
- sql_type= SPIDER_SQL_TYPE_SELECT_SQL;
|
- spider_db_handler *dbton_hdl = dbton_handler[conn->dbton_id]; |
I suspect share->use_sql_dbton_ids[roop_count] and conn->dbton_id are always the same value.
Another issues is the pattern in calls to spider_ping_table_mon_from_table(), and the patterns like the following in setup and teardown of queries
pthread_mutex_lock(&conn->mta_conn_mutex);
|
SPIDER_SET_FILE_POS(&conn->mta_conn_mutex_file_pos);
|
conn->need_mon = &need_mons[roop_count];
|
DBUG_ASSERT(!conn->mta_conn_mutex_lock_already);
|
DBUG_ASSERT(!conn->mta_conn_mutex_unlock_later);
|
conn->mta_conn_mutex_lock_already = TRUE;
|
conn->mta_conn_mutex_unlock_later = TRUE;
|
if ((error_num = spider_db_set_names(this, conn, roop_count))) |
{
|
DBUG_ASSERT(conn->mta_conn_mutex_lock_already);
|
DBUG_ASSERT(conn->mta_conn_mutex_unlock_later);
|
conn->mta_conn_mutex_lock_already = FALSE;
|
conn->mta_conn_mutex_unlock_later = FALSE;
|
SPIDER_CLEAR_FILE_POS(&conn->mta_conn_mutex_file_pos);
|
pthread_mutex_unlock(&conn->mta_conn_mutex);
|
They come in variants (sometimes missing the set / clear file pos part, sometimes without assigning to conn->need_mon) and is scattered all around the codebase, and need a proper cleanup based on understanding (see also my previous attempt in MDEV-29502 https://github.com/MariaDB/server/commit/3b5cecbacd46ea8607b4e5379633e60a98d71bc5). Therefore we defer this ticket to later, maybe when relevant to another task
Thanks for the testing. Pushed the following to 10.5
cc0faa1e3ea * upstream/bb-10.5-mdev-31788 upstream/10.5 MDEV-31788 Factor functions to reduce duplication around spider_check_and_init_casual_read in ha_spider.cc0ba97e4dc62 * MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc9e1579788fc * MDEV-31788 Factor spider locking and unlocking code around sending queries84067291b4f * MDEV-28360 Spider: remove #ifdef SPIDER_use_LEX_CSTRING_for_KEY_Field_namef5b7c25e1e2 * MDEV-27643 Spider: remove #ifdef HA_CAN_BULK_ACCESSe7570c7759e * MDEV-31788 Remove spider_file_posa81f419b06d * 10.5 upstream/bb-10.5-mdev-26178 MDEV-27648 remove #define HASH_UPDATE_WITH_HASH_VALUE5d54e86c220 * MDEV-26178 spider: delete spd_environ.h869c501ac36 * MDEV-27644 Spider: remove HANDLER_HAS_DIRECT_AGGREGATE3a58291680c * MDEV-27662 remove SPIDER_SUPPORT_CREATE_OR_REPLACE_TABLE84977868b1e * MDEV-27809 remove SPIDER_I_S_USE_SHOW_FOR_COLUMN6287fb6e174 * MDEV-27652 remove #ifdef HA_HAS_CHECKSUM_EXTENDEDe8a5553cefd * MDEV-27808 remove SPIDER_LIKE_FUNC_HAS_GET_NEGATEDab49b46d014 * MDEV-27664 remove SPIDER_SQL_CACHE_IS_IN_LEXa1e5ee91117 * MDEV-27663 remove SPIDER_USE_CONST_ITEM_FOR_STRING_INT_REAL_DECIMAL_DATE_ITEM5e98471df13 * MDEV-27811: remove SPIDER_MDEV_162468c8684b17f4 * MDEV-28226 Remove HANDLER_HAS_NEED_INFO_FOR_AUTO_INCaffcb0713d4 * MDEV-26178 spider: remove PARTITION_HAS_GET_PART_SPEC6d0d09ebc2a * MDEV-26178 Spider: remove HANDLER_HAS_TOP_TABLE_FIELDS1cb75d9a33a * MDEV-27660 Remove #ifdef SPIDER_HANDLER_START_BULK_INSERT_HAS_FLAGSaaba68ac1ec * MDEV-28896 Spider: remove #ifdef SPIDER_UPDATE_ROW_HAS_CONST_NEW_DATAf16c037753e * MDEV-28895 Spider: remove #ifdef HANDLER_HAS_CAN_USE_FOR_AUTO_INC_INIT0650c87d9bb * MDEV-27647 Spider: remove HANDLER_HAS_DIRECT_UPDATE_ROWSd5d65b948b6 * MDEV-26178 Spider: remove HA_EXTRA_HAS_HA_EXTRA_USE_CMP_REFde3dd942c00 * MDEV-28894 Spider: remove #ifdef HA_EXTRA_HAS_STARTING_ORDERED_INDEX_SCAN64581c83e8c * MDEV-28893 Spider: remove #ifdef SPIDER_NET_HAS_THDba9bebd7194 * MDEV-28892 remove #ifdef SPIDER_Item_args_arg_count_IS_PROTECTED05fafaf82d2 * MDEV-27646 remove SPIDER_HAS_HASH_VALUE_TYPEI was planning to do it in two stages - the
MDEV-26178commits first. But I ended up pushing both sets of commits. I will still do the merge up in two stages.