[MDEV-29693] ANALYZE TABLE still flushes table definition cache when engine-independent statistics is used Created: 2022-10-04  Updated: 2024-01-05  Resolved: 2023-08-19

Status: Closed
Project: MariaDB Server
Component/s: Optimizer
Affects Version/s: 10.5.13, 10.5.18, 10.6.14
Fix Version/s: 10.6.16, 10.10.7, 10.11.6

Type: Bug Priority: Critical
Reporter: Valerii Kravchuk Assignee: Elena Stepanova
Resolution: Fixed Votes: 1
Labels: None

Attachments: File get_stat_values_no_need_to_be_virtual.diff    
Issue Links:
Relates
relates to MDEV-19061 table_share used for reading statisti... Closed
relates to MDEV-31957 Concurrent ALTER and ANALYZE collecti... Closed

 Description   

MDEV-15101 is supposed to be fixed in 10.5.4+, but if engine-independent statistics is collected by the ANALYZE TABLE we still have to wait for the table flush if there was some long running query in progress when such ANALYZE was executed, thus causing the same problems. Consider this simple table:

MariaDB [test]> show create table t1\G
 
*************************** 1. row ***************************
 
       Table: t1
 
Create Table: CREATE TABLE `t1` (
 
  `id` int(11) DEFAULT NULL,
 
  `c1` int(11) DEFAULT NULL
 
) ENGINE=InnoDB DEFAULT CHARSET=latin1
 
1 row in set (0.005 sec)
 
 
 
MariaDB [test]> select * from t1;
 
+------+------+
 
| id   | c1   |
 
+------+------+
 
|    1 |    0 |
 
|    2 |    0 |
 
|    3 |    0 |
 
|    4 |    0 |
 
+------+------+
 
4 rows in set (0.002 sec)

In one connection run a slow query against it, like this:

MariaDB [test]> select sleep(100) from t1;

Now in another one try this:

MariaDB [test]> select @@use_stat_tables;
 
+------------------------+
 
| @@use_stat_tables      |
 
+------------------------+
 
| PREFERABLY_FOR_QUERIES |
 
+------------------------+
 
1 row in set (0.000 sec)
 
 
 
MariaDB [test]> analyze table t1;
 
+---------+---------+----------+----------+
 
| Table   | Op      | Msg_type | Msg_text |
 
+---------+---------+----------+----------+
 
| test.t1 | analyze | status   | OK       |
 
+---------+---------+----------+----------+
 
1 row in set (0.044 sec)
 
 
 
MariaDB [test]> select count(*) from t1;
 
+----------+
 
| count(*) |
 
+----------+
 
|        4 |
 
+----------+
 
1 row in set (0.001 sec)

No problem so far, but if we with to 'preferable' to force engine-independent statistics collection even by default:

MariaDB [test]> set session use_stat_tables = 'preferably';
 
Query OK, 0 rows affected (0.005 sec)
 
 
 
MariaDB [test]> analyze table t1;
 
+---------+---------+----------+-----------------------------------------+
 
| Table   | Op      | Msg_type | Msg_text                                |
 
+---------+---------+----------+-----------------------------------------+
 
| test.t1 | analyze | status   | Engine-independent statistics collected |
 
| test.t1 | analyze | status   | OK                                      |
 
+---------+---------+----------+-----------------------------------------+
 
2 rows in set (0.053 sec)
 
 
 
MariaDB [test]> select count(*) from t1;
 
...

we are forced to wait for the query in the first connection to complete, this way:

Yuliyas-Air:maria10.5 Valerii$ bin/mysql test
 
Reading table information for completion of table and column names
 
You can turn off this feature to get a quicker startup with -A
 
 
 
Welcome to the MariaDB monitor.  Commands end with ; or \g.
 
Your MariaDB connection id is 21
 
Server version: 10.5.18-MariaDB MariaDB Server
 
 
 
Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.
 
 
 
Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.
 
 
 
MariaDB [test]> show processlist;
 
+----+---------+-----------+------+---------+------+-------------------------+---------------------------+----------+
 
| Id | User    | Host      | db   | Command | Time | State                   | Info                      | Progress |
 
+----+---------+-----------+------+---------+------+-------------------------+---------------------------+----------+
 
| 19 | Valerii | localhost | test | Query   |  114 | User sleep              | select sleep(100) from t1 |    0.000 |
 
| 20 | Valerii | localhost | test | Query   |    6 | Waiting for table flush | select count(*) from t1   |    0.000 |
 
| 21 | Valerii | localhost | test | Query   |    0 | starting                | show processlist          |    0.000 |
 
+----+---------+-----------+------+---------+------+-------------------------+---------------------------+----------+
 
3 rows in set (0.003 sec)

So, the fix for MDEV-15101 had not taken the engine-independent table statistics collection case into account. I think it's a bug.

 Comments   
Comment by Oleg Smirnov [ 2022-12-10 ]

psergei, I have finally fixed all memory issued reported by sanitizers and am passing this task for review. Branch bb-10.5-MDEV-29693.

commit 4b607210a817bda8c556a0674dd51c545c45b89a (HEAD -> bb-10.5-MDEV-29693, origin/bb-10.5-MDEV-29693)
 
Author: Oleg Smirnov <olernov@gmail.com>
 
Date:   Sat Oct 15 17:54:40 2022 +0700
 
 
 
MDEV-29693 ANALYZE TABLE still flushes table definition cache when engine-independent statistics is used
 
    
    This commits enables reloading of engine-independent statistics
 
    without flushing the table from table definition cache. This is
 
    achieved by using shared ownership of the statistics between TABLE
 
    and TABLE_SHARE structures. The updated statistics replace the ones
 
    stored in TABLE_SHARE but TABLE objects continue using the old ones
 
    to guarantee consistency for the optimizer. Once there are no
 
    objects referencing the old statistics they are deallocated

serg suggested to discuss whether this a bug fix or a feature, I'm not sure but think this affects whether this should be merged into ES/CS and the target versions.
Since many parts of the code base were touched I believe it won't hurt to request testing, probably including performance testing 'cause some new mutex locks were added.

Also there is a minor question to discuss:

      else if (collect_eis && skip_flush)
 
      {
 
        DBUG_ASSERT(operator_func == &handler::ha_analyze &&
 
                    table->table->file->ha_table_flags() & HA_ONLINE_ANALYZE);
 
 
 
        // OLEGS: should we push warning/do something else in case of error
 
        // or just ignore it?
 
        read_statistics_for_tables(thd, table, true /* force_reload */);
 
      }

Comment by Sergei Petrunia [ 2023-01-16 ]

Part of review input: There is actually no point in get_stat_values being a virtual function. There is no scenario where we call X->get_stat_values() when we're not sure about the type of X. Proof: get_stat_values_no_need_to_be_virtual.diff

Comment by Oleg Smirnov [ 2023-07-20 ]

Discussions of performance impact of the change:
https://mariadb.slack.com/archives/CHTLSLQEP/p1675421251834179
https://mariadb.slack.com/archives/CHTLSLQEP/p1675422005081299

Comment by Oleg Smirnov [ 2023-07-26 ]

Fresh benchmarks of bb-10.5-MDEV-29693-v2 which is rebased on the most recent 10.5: https://mariadb.slack.com/archives/CHTLSLQEP/p1690381121753799?thread_ts=1675421251.834179&cid=CHTLSLQEP.
The regression is gone.

Comment by Oleg Smirnov [ 2023-08-01 ]

MDEV-19061 statistics locking using atomics does not make sense for this patch since mutexes are back. But I decided not to remove it for now (a subject for discussion).

Comment by Michael Widenius [ 2023-08-05 ]

Suggested final version pushed to bb-10.5-monty.
I have asked Sergei Petrunia to do a review.

Comment by Elena Stepanova [ 2023-08-17 ]

Run with --repeat=N if it doesn't fail right away.

--source include/have_sequence.inc
 
 
 
CREATE TABLE t (a INT, b VARCHAR(128));
 
INSERT INTO t SELECT seq, CONCAT('s',seq) FROM seq_1_to_100;
 
 
 
--connect (con1,localhost,root,,)
 
--send
 
  ALTER TABLE t MODIFY b BLOB;
 
 
 
--connection default
 
ANALYZE TABLE t PERSISTENT FOR ALL;
 
 
 
--connection con1
 
--reap
 
ANALYZE TABLE t PERSISTENT FOR ALL;

bb-10.6-monty 901cf96446816cae944e73df62bbdf6aa4ff2414

 
Direct leak of 32 byte(s) in 1 object(s) allocated from:
 
    #0 0x7f88fb0b89cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
 
    #1 0x5561915952fd in my_malloc /data/src/bb-10.6-monty/mysys/my_malloc.c:91
 
    #2 0x55618f2b8637 in Binary_string::real_alloc(unsigned long) /data/src/bb-10.6-monty/sql/sql_string.cc:45
 
    #3 0x55618ec286b0 in Binary_string::alloc(unsigned long) /data/src/bb-10.6-monty/sql/sql_string.h:792
 
    #4 0x55618fa546a7 in Field_blob::store(char const*, unsigned long, charset_info_st const*) /data/src/bb-10.6-monty/sql/field.cc:8668
 
    #5 0x55618f4dc89c in Field::store_text(char const*, unsigned long, charset_info_st const*) /data/src/bb-10.6-monty/sql/field.h:950
 
    #6 0x55618fa01060 in Field_blob::store_from_statistical_minmax_field(Field*, String*, st_mem_root*) /data/src/bb-10.6-monty/sql/field.cc:2024
 
    #7 0x55618f2b062f in Column_stat::get_stat_values(Column_statistics*, st_mem_root*, bool) /data/src/bb-10.6-monty/sql/sql_statistics.cc:1147
 
    #8 0x55618f2a057a in read_statistics_for_table /data/src/bb-10.6-monty/sql/sql_statistics.cc:2883
 
    #9 0x55618f2a2a67 in read_statistics_for_tables(THD*, TABLE_LIST*, bool) /data/src/bb-10.6-monty/sql/sql_statistics.cc:3137
 
    #10 0x55618f58237e in mysql_admin_table /data/src/bb-10.6-monty/sql/sql_admin.cc:1315
 
    #11 0x55618f58405b in Sql_cmd_analyze_table::execute(THD*) /data/src/bb-10.6-monty/sql/sql_admin.cc:1492
 
    #12 0x55618efd02d2 in mysql_execute_command(THD*, bool) /data/src/bb-10.6-monty/sql/sql_parse.cc:6024
 
    #13 0x55618efe1a0f in mysql_parse(THD*, char*, unsigned int, Parser_state*) /data/src/bb-10.6-monty/sql/sql_parse.cc:8053
 
    #14 0x55618efa49ed in dispatch_command(enum_server_command, THD*, char*, unsigned int, bool) /data/src/bb-10.6-monty/sql/sql_parse.cc:1896
 
    #15 0x55618efa0bea in do_command(THD*, bool) /data/src/bb-10.6-monty/sql/sql_parse.cc:1409
 
    #16 0x55618f52f4a1 in do_handle_one_connection(CONNECT*, bool) /data/src/bb-10.6-monty/sql/sql_connect.cc:1416
 
    #17 0x55618f52eb4c in handle_one_connection /data/src/bb-10.6-monty/sql/sql_connect.cc:1318
 
    #18 0x5561904b0c02 in pfs_spawn_thread /data/src/bb-10.6-monty/storage/perfschema/pfs.cc:2201
 
    #19 0x7f88fa4a7fd3 in start_thread nptl/pthread_create.c:442

Comment by Michael Widenius [ 2023-08-19 ]

Working with Elena to fix issue in patch

  • Elena found a not freed memory issue in a 'impossible code path'
  • I found an old bug in updating column_statistics with ALTER TABLE.
    This is described in MDEV-31957 Concurrent ALTER and ANALYZE collecting statistics can result in stale statistical data
Comment by Marko Mäkelä [ 2023-08-22 ]

Was any 10.10 version of this prepared or tested before this was pushed to 10.5? These changes seem to conflict with MDEV-21130.

Comment by Sergei Petrunia [ 2023-09-07 ]

Note for the changelog: Running a variant of ANALYZE TABLE statement that collects EITS statistics used to cause a table definition cache flush. Now it is no longer done. A user-visible effect of this is as follows: if one connection runs a long query Q1 that uses the table and the other connection runs ANALYZE TABLE statement that collects EITS statistics for that table, all further queries had to wait until Q1 finishes. Now it's no longer the case.

Comment by Marko Mäkelä [ 2023-10-10 ]

This was finally merged to 10.10.

Generated at Thu Feb 08 10:10:34 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.