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

dict_table_stats_lock() has unnecessarily long scope

Details

    Description

      The following test demonstrates that while InnoDB is collecting statistics are collected on a large table, any access from SQL to that table will be unnecessarily blocked by dict_table_stats_lock():

      diff a/mysql-test/suite/innodb/t/stats_persistent.test b/mysql-test/suite/innodb/t/stats_persistent.test
      		 
      new file mode 100644
      		 
      --- /dev/null
      		 
      +++ b/mysql-test/suite/innodb/t/stats_persistent.test
      		 
      @@ -0,0 +1,26 @@
      		 
      +--source include/have_innodb.inc
      		 
      +--source include/have_debug.inc
      		 
      +--source include/have_debug_sync.inc
      		 
      +--source include/count_sessions.inc
      		 
      +
      		 
      +CREATE TABLE t1(a int) ENGINE=INNODB STATS_PERSISTENT=1;
      		 
      +
      		 
      +SET DEBUG_SYNC='dict_stats_update_persistent SIGNAL stop WAIT_FOR go';
      		 
      +send ANALYZE TABLE t1;
      		 
      +
      		 
      +--connect(con1, localhost, root)
      		 
      +SET DEBUG_SYNC='now WAIT_FOR stop';
      		 
      +
      		 
      +SELECT * FROM information_schema.TABLES WHERE ENGINE='InnoDB';
      		 
      +
      		 
      +SET DEBUG_SYNC='now SIGNAL go';
      		 
      +--disconnect con1
      		 
      +
      		 
      +--connection default
      		 
      +--reap
      		 
      +SET DEBUG_SYNC= 'RESET';
      		 
      +DROP TABLE t1;
      		 
      +
      		 
      +--source include/wait_until_count_sessions.inc
      		 
      diff a/storage/innobase/dict/dict0stats.cc b/storage/innobase/dict/dict0stats.cc
      		 
      --- a/storage/innobase/dict/dict0stats.cc
      		 
      +++ b/storage/innobase/dict/dict0stats.cc
      		 
      @@ -2207,6 +2207,8 @@ dict_stats_update_persistent(
      		 
       
       	dict_table_stats_lock(table, RW_X_LATCH);
      		 
       
      +	DEBUG_SYNC_C("dict_stats_update_persistent");
      		 
      +
      		 
       	/* analyze the clustered index first */
      		 
       
       	index = dict_table_get_first_index(table);

      We can allow multiple threads to collect statistics for the same table in parallel, but only one thread at a time may be allowed to update the statistics. To prevent unnecessary work for multiple threads collecting statistics for the same table, we might introduce a counter of pending statistics-gathering operations. MySQL 5.7.32 preferred to introduce a lazily initialized mutex, which would not be easy to merge to 10.4.

      Attachments

        Issue Links

          blocks

          Bug - A problem which impairs or prevents the functions of the product. MDEV-23989 Merge new release of InnoDB 5.7.32 to 10.2

          • Blocker - Blocks development and/or testing work, production could not run.
          • Closed
          causes

          Bug - A problem which impairs or prevents the functions of the product. MDEV-24275 InnoDB persistent stats analyze forces full scan forcing lock crash

          • Major - Major loss of function.
          • Closed

          Activity

            Ascending order - Click to sort in descending order
            marko Marko Mäkelä added a comment -

            I think that we should properly separate the statistics collection and the updates. If we do that, updating and accessing the statistics in dict_table_t and dict_index_t should be quick enough to be protected by dict_sys->mutex. Updating the persistent statistics tables will be holding it already.

            Collecting that statistics can be protected by normal dict_index_t::lock and buf_block_t::lock. I do not think that multiple concurrent threads collecting statistics on the same index are going to be an issue.

            marko Marko Mäkelä added a comment - I think that we should properly separate the statistics collection and the updates. If we do that, updating and accessing the statistics in dict_table_t and dict_index_t should be quick enough to be protected by dict_sys->mutex . Updating the persistent statistics tables will be holding it already. Collecting that statistics can be protected by normal dict_index_t::lock and buf_block_t::lock . I do not think that multiple concurrent threads collecting statistics on the same index are going to be an issue.
            marko Marko Mäkelä added a comment -

            The fix so far looks good, except that in a few places we seem to be violating the latching order, by acquiring dict_sys->mutex while holding an index->lock. Be sure to release the latch via mtr_t::commit() or similar, to fix this. Run tests with ./mtr --mysqld=--loose-innodb-debug-sync to catch this.

            marko Marko Mäkelä added a comment - The fix so far looks good, except that in a few places we seem to be violating the latching order, by acquiring dict_sys->mutex while holding an index->lock . Be sure to release the latch via mtr_t::commit() or similar, to fix this. Run tests with ./mtr --mysqld=--loose-innodb-debug-sync to catch this.

            People

              kevg Eugene Kosov (Inactive)
              marko Marko Mäkelä
              Votes:
              0 Vote for this issue
              Watchers:
              2 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.