Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-31788

Factor out code repeat in functions calling spider_check_and_init_casual_read()

Details

    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

        1. ft_read_internal.cc
          10 kB
        2. index_first_map_internal.cc
          10 kB
        3. index_last_internal.cc
          10 kB
        4. index_read_last_map_internal.cc
          10 kB
        5. index_read_map_internal.cc
          10 kB
        6. multi_range_read_next_first.cc
          38 kB
        7. multi_range_read_next_next.cc
          37 kB
        8. range_read_first_internal.cc
          10 kB
        9. rnd_next_internal.cc
          11 kB
        10. screen.gif
          screen.gif
          2.03 MB

        Issue Links

          relates to

          Task - A task that needs to be done. MDEV-26151 Documentation on spider_casual_read is insufficient/non-existing

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed

          Task - A task that needs to be done. MDEV-31787 Clean up and code documentation of casual_read related code

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed

          Activity

            Ascending order - Click to sort in descending order
            ycp Yuchen Pei added a comment -

            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

            ycp Yuchen Pei added a comment - 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
            ycp Yuchen Pei added a comment - - edited

            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

            ycp Yuchen Pei added a comment - - edited 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
            holyfoot Alexey Botchkov added a comment -

            ok to push.

            holyfoot Alexey Botchkov added a comment - ok to push.
            ycp Yuchen Pei added a comment -

            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.

            ycp Yuchen Pei added a comment - 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 .
            Roel Roel Van de Paar added a comment - - edited

            FYI, this comment shows a new stack, for a previously known bug, in MDEV-31788 10.5 opt builds. This may require review.

            Roel Roel Van de Paar added a comment - - edited FYI, this comment shows a new stack, for a previously known bug, in MDEV-31788 10.5 opt builds. This may require review.
            Roel Roel Van de Paar added a comment -

            Various stacks proved to be further variations of MDEV-27902, ref this comment.

            Roel Roel Van de Paar added a comment - Various stacks proved to be further variations of MDEV-27902 , ref this comment .
            Roel Roel Van de Paar added a comment -

            OK to push.

            Roel Roel Van de Paar added a comment - OK to push.
            ycp Yuchen Pei added a comment - - edited

            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.cc
            		 
            0ba97e4dc62 * MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc
            		 
            9e1579788fc * MDEV-31788 Factor spider locking and unlocking code around sending queries
            		 
            84067291b4f * MDEV-28360 Spider: remove #ifdef SPIDER_use_LEX_CSTRING_for_KEY_Field_name
            		 
            f5b7c25e1e2 * MDEV-27643 Spider: remove #ifdef HA_CAN_BULK_ACCESS
            		 
            e7570c7759e * MDEV-31788 Remove spider_file_pos
            		 
            a81f419b06d * 10.5 upstream/bb-10.5-mdev-26178 MDEV-27648 remove #define HASH_UPDATE_WITH_HASH_VALUE
            		 
            5d54e86c220 * MDEV-26178 spider: delete spd_environ.h
            		 
            869c501ac36 * MDEV-27644 Spider: remove HANDLER_HAS_DIRECT_AGGREGATE
            		 
            3a58291680c * MDEV-27662 remove SPIDER_SUPPORT_CREATE_OR_REPLACE_TABLE
            		 
            84977868b1e * MDEV-27809 remove SPIDER_I_S_USE_SHOW_FOR_COLUMN
            		 
            6287fb6e174 * MDEV-27652 remove #ifdef HA_HAS_CHECKSUM_EXTENDED
            		 
            e8a5553cefd * MDEV-27808 remove SPIDER_LIKE_FUNC_HAS_GET_NEGATED
            		 
            ab49b46d014 * MDEV-27664 remove SPIDER_SQL_CACHE_IS_IN_LEX
            		 
            a1e5ee91117 * MDEV-27663 remove SPIDER_USE_CONST_ITEM_FOR_STRING_INT_REAL_DECIMAL_DATE_ITEM
            		 
            5e98471df13 * MDEV-27811: remove SPIDER_MDEV_16246
            		 
            8c8684b17f4 * MDEV-28226 Remove HANDLER_HAS_NEED_INFO_FOR_AUTO_INC
            		 
            affcb0713d4 * MDEV-26178 spider: remove PARTITION_HAS_GET_PART_SPEC
            		 
            6d0d09ebc2a * MDEV-26178 Spider: remove HANDLER_HAS_TOP_TABLE_FIELDS
            		 
            1cb75d9a33a * MDEV-27660 Remove #ifdef SPIDER_HANDLER_START_BULK_INSERT_HAS_FLAGS
            		 
            aaba68ac1ec * MDEV-28896 Spider: remove #ifdef SPIDER_UPDATE_ROW_HAS_CONST_NEW_DATA
            		 
            f16c037753e * MDEV-28895 Spider: remove #ifdef HANDLER_HAS_CAN_USE_FOR_AUTO_INC_INIT
            		 
            0650c87d9bb * MDEV-27647 Spider: remove HANDLER_HAS_DIRECT_UPDATE_ROWS
            		 
            d5d65b948b6 * MDEV-26178 Spider: remove HA_EXTRA_HAS_HA_EXTRA_USE_CMP_REF
            		 
            de3dd942c00 * MDEV-28894 Spider: remove #ifdef HA_EXTRA_HAS_STARTING_ORDERED_INDEX_SCAN
            		 
            64581c83e8c * MDEV-28893 Spider: remove #ifdef SPIDER_NET_HAS_THD
            		 
            ba9bebd7194 * MDEV-28892 remove #ifdef SPIDER_Item_args_arg_count_IS_PROTECTED
            		 
            05fafaf82d2 * MDEV-27646 remove SPIDER_HAS_HASH_VALUE_TYPE

            I was planning to do it in two stages - the MDEV-26178 commits first. But I ended up pushing both sets of commits. I will still do the merge up in two stages.

            ycp Yuchen Pei added a comment - - edited 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.cc 0ba97e4dc62 * MDEV-31788 Factor out calls to spider_ping_table_mon_from_table in ha_spider.cc 9e1579788fc * MDEV-31788 Factor spider locking and unlocking code around sending queries 84067291b4f * MDEV-28360 Spider: remove #ifdef SPIDER_use_LEX_CSTRING_for_KEY_Field_name f5b7c25e1e2 * MDEV-27643 Spider: remove #ifdef HA_CAN_BULK_ACCESS e7570c7759e * MDEV-31788 Remove spider_file_pos a81f419b06d * 10.5 upstream/bb-10.5-mdev-26178 MDEV-27648 remove #define HASH_UPDATE_WITH_HASH_VALUE 5d54e86c220 * MDEV-26178 spider: delete spd_environ.h 869c501ac36 * MDEV-27644 Spider: remove HANDLER_HAS_DIRECT_AGGREGATE 3a58291680c * MDEV-27662 remove SPIDER_SUPPORT_CREATE_OR_REPLACE_TABLE 84977868b1e * MDEV-27809 remove SPIDER_I_S_USE_SHOW_FOR_COLUMN 6287fb6e174 * MDEV-27652 remove #ifdef HA_HAS_CHECKSUM_EXTENDED e8a5553cefd * MDEV-27808 remove SPIDER_LIKE_FUNC_HAS_GET_NEGATED ab49b46d014 * MDEV-27664 remove SPIDER_SQL_CACHE_IS_IN_LEX a1e5ee91117 * MDEV-27663 remove SPIDER_USE_CONST_ITEM_FOR_STRING_INT_REAL_DECIMAL_DATE_ITEM 5e98471df13 * MDEV-27811: remove SPIDER_MDEV_16246 8c8684b17f4 * MDEV-28226 Remove HANDLER_HAS_NEED_INFO_FOR_AUTO_INC affcb0713d4 * MDEV-26178 spider: remove PARTITION_HAS_GET_PART_SPEC 6d0d09ebc2a * MDEV-26178 Spider: remove HANDLER_HAS_TOP_TABLE_FIELDS 1cb75d9a33a * MDEV-27660 Remove #ifdef SPIDER_HANDLER_START_BULK_INSERT_HAS_FLAGS aaba68ac1ec * MDEV-28896 Spider: remove #ifdef SPIDER_UPDATE_ROW_HAS_CONST_NEW_DATA f16c037753e * MDEV-28895 Spider: remove #ifdef HANDLER_HAS_CAN_USE_FOR_AUTO_INC_INIT 0650c87d9bb * MDEV-27647 Spider: remove HANDLER_HAS_DIRECT_UPDATE_ROWS d5d65b948b6 * MDEV-26178 Spider: remove HA_EXTRA_HAS_HA_EXTRA_USE_CMP_REF de3dd942c00 * MDEV-28894 Spider: remove #ifdef HA_EXTRA_HAS_STARTING_ORDERED_INDEX_SCAN 64581c83e8c * MDEV-28893 Spider: remove #ifdef SPIDER_NET_HAS_THD ba9bebd7194 * MDEV-28892 remove #ifdef SPIDER_Item_args_arg_count_IS_PROTECTED 05fafaf82d2 * MDEV-27646 remove SPIDER_HAS_HASH_VALUE_TYPE I was planning to do it in two stages - the MDEV-26178 commits first. But I ended up pushing both sets of commits. I will still do the merge up in two stages.

            People

              ycp Yuchen Pei
              ycp Yuchen Pei
              Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.