MDEV-36483 Optimizer Trace Replay step #1: dump DDLs of tables/views #4034

bsrikanth-mariadb · 2025-05-06T02:48:13Z

Pull request created in: https://jira.mariadb.org/browse/MDEV-36483

spetrunia

As I've already requested: Please apply the output of

git diff -U0 --no-color --relative 4b79d7b8ee557d53a859aedec839b8673585b514 | clang-format-diff -p1 -i

Here it is:

diff --git a/sql/opt_trace_ddl_info.cc b/sql/opt_trace_ddl_info.cc
index c9ce9645781..d6b729efa60 100644
--- a/sql/opt_trace_ddl_info.cc
+++ b/sql/opt_trace_ddl_info.cc
@@ -12,7 +12,8 @@
 
    You should have received a copy of the GNU General Public License
    along with this program; if not, write to the Free Software
-   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1335  USA */
+   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1335
+   USA */
 
 #include "opt_trace_ddl_info.h"
 #include "sql_show.h"
@@ -26,7 +27,7 @@
   @file
 
   @brief
-    Stores the ddls of the tables, and views that are used 
+    Stores the ddls of the tables, and views that are used
     in either SELECT, INSERT, DELETE, and UPDATE queries,
     into the optimizer trace. All the ddls are stored together
     at one place as a JSON array object with name "list_ddls"
@@ -42,7 +43,8 @@ struct DDL_Info_Record
   helper function to know the key portion of the record
   that is stored in hash, and record list.
 */
-static const uchar *get_rec_key(const void *entry_, size_t *length, my_bool flags)
+static const uchar *get_rec_key(const void *entry_, size_t *length,
+                                my_bool flags)
 {
   auto entry= static_cast<const DDL_Info_Record *>(entry_);
   *length= strlen(entry->name);
@@ -52,7 +54,8 @@ static const uchar *get_rec_key(const void *entry_, size_t *length, my_bool flag
 /*
   helper function to free record from the hash
 */
-static void free_rec(void *entry_) {
+static void free_rec(void *entry_)
+{
   auto entry= static_cast<DDL_Info_Record *>(entry_);
   my_free(entry->name);
   my_free(entry->stmt);
@@ -110,15 +113,15 @@ static void create_view_def(THD *thd, TABLE_LIST *table, String *name,
 }
 
 /*
-  Stores the ddls of the tables, and views that are used 
+  Stores the ddls of the tables, and views that are used
   in either SELECT, INSERT, DELETE, and UPDATE queries,
   into the optimizer trace.
   Global query_tables are read in reverse order from the thd->lex,
   and a record with table_name, and ddl of the table are created.
   Hash is used to store the records, where in no duplicates
   are stored. dbName_plus_tableName is used as a key to discard any
-  duplicates. If a new record that is created is not in the hash, 
-  then that is dumped into the trace.  
+  duplicates. If a new record that is created is not in the hash,
+  then that is dumped into the trace.
 */
 void store_table_definitions_in_trace(THD *thd)
 {
diff --git a/sql/opt_trace_ddl_info.h b/sql/opt_trace_ddl_info.h
index 7e130ce6a05..8f4e8ef49c0 100644
--- a/sql/opt_trace_ddl_info.h
+++ b/sql/opt_trace_ddl_info.h
@@ -12,7 +12,8 @@
 
    You should have received a copy of the GNU General Public License
    along with this program; if not, write to the Free Software
-   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1335  USA */
+   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1335
+   USA */
 
 #ifndef OPT_TRACE_DDL_INFO
 #define OPT_TRACE_DDL_INFO
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
index 609e4ac242d..9644a912b2e 100644
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -92,7 +92,7 @@
 #include "opt_trace.h"
 #include "mysql/psi/mysql_sp.h"
 
-#include "my_json_writer.h" 
+#include "my_json_writer.h"
 #include "opt_trace_ddl_info.h"
 #define FLAGSTR(V,F) ((V)&(F)?#F" ":"")

spetrunia · 2025-05-26T07:54:43Z

sql/sql_show.h

@@ -82,7 +82,6 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond);
 int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet,
                      Table_specification_st *create_info_arg,
                      enum_with_db_name with_db_name);


Remove this change.

spetrunia · 2025-05-26T07:55:16Z

sql/sql_class.cc

@@ -1728,7 +1728,6 @@ void THD::cleanup(void)
  statement_rcontext_reinit();
  auto_inc_intervals_forced.empty();
  auto_inc_intervals_in_cur_stmt_for_binlog.empty();


Remove this change also please.

spetrunia

More smaller comments.

sql/opt_trace_ddl_info.cc

spetrunia · 2025-05-26T10:02:47Z

sql/opt_trace_ddl_info.cc

+            tbl->table->s->tmp_table == TRANSACTIONAL_TMP_TABLE)))
+      {
+        size_t name_len=
+            strlen(tbl->table_name.str) + strlen(tbl->get_db_name().str) + 1;


Please use tbl->table_name.length, not strlen. Same with get_db_name.length.

spetrunia · 2025-05-26T10:03:11Z

sql/opt_trace_ddl_info.cc

+          else
+            show_create_table(thd, tbl, &ddl, NULL, WITH_DB_NAME);
+
+          buf= ddl.c_ptr();


use ddl.length(), no need for strlen.

spetrunia · 2025-05-26T10:03:36Z

sql/opt_trace_ddl_info.h

+
+void store_table_definitions_in_trace(THD *thd);
+
+#endif


Add new line at end of file.

spetrunia · 2025-05-26T10:03:47Z

sql/opt_trace_ddl_info.cc

+    }
+    my_hash_free(&hash);
+  }
+}


Add newline at the end of file.

spetrunia · 2025-05-26T10:04:34Z

sql/opt_trace_ddl_info.cc

+    return true;
+  else if (get_table_category(tbl->get_db_name(), tbl->get_table_name()) !=
+               TABLE_CATEGORY_USER ||
+           strcmp(tbl->get_db_name().str, "sys") == 0)


why strcmp(tbl->get_db_name().str, "sys") ? Please explain.

any "sys" table should return true. I felt, get_table_category() has fewer restrictions.

Looking at the "sys" database, I see only VIEWs (over information_schema and performance_schema tables) and one base table - sys.sys_config .
I don't think factoring out "sys" gives any value here, please remove it.

P_S and I_S tables are filtered out by the get_table_category()...

Looking at the "sys" database, I see only VIEWs (over information_schema and performance_schema tables) and one base table - sys.sys_config . I don't think factoring out "sys" gives any value here, please remove it.

P_S and I_S tables are filtered out by the get_table_category()...

Yes, I saw some of the tests having sys.sys_config actually failed, when invoking show_create_table().

You can use get_table_category or, if the TABLE_SHARE is somehow available at this point in the code, you can use TABLE_SHARE::table_category to find the type of table as given in the enum_table_category enumeration. It seems that system tables should be marked as TABLE_CATEGORY_SYSTEM.

You can use get_table_category or, if the TABLE_SHARE is somehow available at this point in the code, you can use TABLE_SHARE::table_category to find the type of table as given in the enum_table_category enumeration. It seems that system tables should be marked as TABLE_CATEGORY_SYSTEM.

Yes, removed the strcmp(tbl->get_db_name().str, "sys") == 0 check

spetrunia · 2025-05-26T10:24:44Z

sql/opt_trace_ddl_info.cc

+  The record has full table_name,
+  and ddl of the table/view.
+*/
+static void dump_table_definition(THD *thd, DDL_Info_Record *rec)


Apparently this is also used for dumping VIEWs. Please rename.

spetrunia · 2025-05-26T10:54:57Z

sql/opt_trace_ddl_info.cc

+            tbl->table->s->tmp_table == TRANSACTIONAL_TMP_TABLE)))
+      {
+        size_t name_len=
+            tbl->get_table_name().length + tbl->get_db_name().length + 1;


So, this allocates memory for db_name, the '.' and the table_name. Where is the space for \0 at the end?
Why not use String object here?

So, this allocates memory for db_name, the '.' and the table_name. Where is the space for \0 at the end? Why not use String object here?

Yes, I could have used the String object. But, this requires allocating new memory anyways, so as to copy the string contents, as the string object is freed after each iteration.

spetrunia

I do not see any tests for querying tables located in different databases. If they are not there, please add them.

This feature stores the ddls of the tables/views that are used in a query, to the optimizer trace. It is currently controlled by a system variable store_ddls_in_optimizer_trace, and is not enabled by default. All the ddls will be stored in a single json array, with each element having table/view name, and the associated create definition of the table/view. The approach taken is to read global query_tables from the thd->lex, and read them in reverse. Create a record with table_name, ddl of the table and add the table_name to the hash, along with dumping the information to the trace. dbName_plus_tableName is used as a key, and the duplicate entries are not added to the hash. The main suite tests are also run with the feature enabled, and they all succeed.

DaveGosselin-MariaDB

In general, there's no need to add new commits to the PR for each review update. You can amend the commit locally and force-push to the PR. GitHub still provides a way to compare intermediate changes in such a case. And it's easier for reviewers to see the changes in one commit (or in two, if there was some prior refactoring that had to occur before the feature work). In this case, please squash all of the commits to one commit as that's what will be merged to main in the end (and be sure to update the commit message so that it's clean for the final product).

The tests are not passing on my Linux system, here is the result I see:

CURRENT_TEST: main.opt_trace_store_ddls
--- /Users/dgosselin/1-server/mysql-test/main/opt_trace_store_ddls.result	2025-05-30 10:11:28.972377130 -0400
+++ /Users/dgosselin/1-server/mysql-test/main/opt_trace_store_ddls.reject	2025-05-30 10:15:56.882183940 -0400
@@ -32,9 +32,6 @@
 select json_detailed(json_extract(trace, '$**.list_ddls'))
 from information_schema.optimizer_trace;
 json_detailed(json_extract(trace, '$**.list_ddls'))
-[
-    []
-]
 #
 # disable optimizer_trace, but enable store_ddls_in_optimizer_trace
 # there should be no trace here as well
@@ -47,9 +44,6 @@
 select json_detailed(json_extract(trace, '$**.list_ddls'))
 from information_schema.optimizer_trace;
 json_detailed(json_extract(trace, '$**.list_ddls'))
-[
-    []
-]
 #
 # enable optimizer_trace, but disable store_ddls_in_optimizer_trace
 # trace result should be empty

Result length mismatch

DaveGosselin-MariaDB · 2025-05-30T14:14:03Z

sql/sys_vars.cc

@@ -3108,6 +3108,13 @@ static Sys_var_ulong Sys_optimizer_trace_max_mem_size(
    SESSION_VAR(optimizer_trace_max_mem_size), CMD_LINE(REQUIRED_ARG),
    VALID_RANGE(0, ULONG_MAX), DEFAULT(1024 * 1024), BLOCK_SIZE(1));

+static Sys_var_mybool Sys_store_ddls_in_optimizer_trace(
+    "store_ddls_in_optimizer_trace",
+    "Controls storing of ddls of all the tables "


Elsewhere in this file, "ddls" is capitalized as "DDLs". Please change this to "DDLs" for consistency.

DaveGosselin-MariaDB · 2025-05-30T14:36:55Z

sql/opt_trace_ddl_info.cc

+{
+  if (tbl->table_function)
+    return true;
+  else if (tbl->schema_table)


It's not necessary to have else if after a return: https://blog.mozilla.org/nnethercote/2009/08/31/no-else-after-return-considered-harmful/

You can write just

if (tbl->table_function || tbl->schema_table) return true; if (get_table_category...

You can also replace the last if statement with a return:

return (get_table_category(tbl->get_db_name(), tbl->get_table_name()) != TABLE_CATEGORY_USER || tbl->table->s->tmp_table == INTERNAL_TMP_TABLE);

DaveGosselin-MariaDB · 2025-05-30T14:42:00Z

sql/opt_trace_ddl_info.cc

+    return true;
+  else if (get_table_category(tbl->get_db_name(), tbl->get_table_name()) !=
+               TABLE_CATEGORY_USER ||
+           strcmp(tbl->get_db_name().str, "sys") == 0)


You can use get_table_category or, if the TABLE_SHARE is somehow available at this point in the code, you can use TABLE_SHARE::table_category to find the type of table as given in the enum_table_category enumeration. It seems that system tables should be marked as TABLE_CATEGORY_SYSTEM.

DaveGosselin-MariaDB · 2025-05-30T14:50:41Z

sql/opt_trace_ddl_info.cc

+  Json_writer_object ddl_wrapper(thd);
+  ddl_wrapper.add("name", rec->name);
+  size_t non_esc_stmt_len= strlen(rec->stmt);
+  size_t escape_stmt_len= 4 * non_esc_stmt_len;


Please add a comment describing what 4 is. If possible, please define it in terms of some other type (e.g., sizeof(uint32_t)).

sql/opt_trace_ddl_info.cc

DaveGosselin-MariaDB · 2025-05-30T15:07:44Z

sql/opt_trace_ddl_info.cc

+               get_rec_key, free_rec, HASH_UNIQUE);
+  while (TABLE_LIST *tbl= li++)
+  {
+    if (tbl->is_view() ||


Please invert the logic of this if and make it continue. You'll improve legibility by keeping the happy path of the code "to the left" (as de-dented as possible): https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88
This entire loop is inside this conditional as it is.

sure, done.

DaveGosselin-MariaDB · 2025-05-30T15:19:49Z

sql/opt_trace_ddl_info.cc

+          tbl->table->s->tmp_table == TRANSACTIONAL_TMP_TABLE)))
+    {
+      size_t name_len=
+          strlen(tbl->table_name.str) + strlen(tbl->get_db_name().str) + 1;


Is the +1 for a dot separator or for NULL terminator?

Is the +1 for a dot separator or for NULL terminator?

for the dot separator

DaveGosselin-MariaDB · 2025-05-30T15:21:49Z

sql/opt_trace_ddl_info.cc

+        my_free(full_name);
+      else
+      {
+        String ddl(2048);


Is 2048 arbitrarily chosen?

DaveGosselin-MariaDB · 2025-05-30T15:22:20Z

sql/opt_trace_ddl_info.cc

+          (char *) my_malloc(key_memory_trace_ddl_info, name_len, 0);
+      strcpy(full_name, tbl->get_db_name().str);
+      strcat(full_name, ".");
+      strcat(full_name, tbl->table_name.str);


It seems that there's no byte allocated for null terminator.

This was an old change. It got corrected in the later commit.

DaveGosselin-MariaDB · 2025-05-30T15:23:23Z

sql/opt_trace_ddl_info.cc

+        buf_copy= (char *) my_malloc(key_memory_trace_ddl_info, buf_len, 0);
+        strcpy(buf_copy, buf);
+        rec->name= full_name;
+        rec->stmt= buf_copy;


Consider storing the name and statement lengths as members in the record, so there will be no need to compute strlen again elsewhere (if needed).

bsrikanth-mariadb · 2025-06-01T13:54:16Z

In general, there's no need to add new commits to the PR for each review update. You can amend the commit locally and force-push to the PR. GitHub still provides a way to compare intermediate changes in such a case. And it's easier for reviewers to see the changes in one commit (or in two, if there was some prior refactoring that had to occur before the feature work). In this case, please squash all of the commits to one commit as that's what will be merged to main in the end (and be sure to update the commit message so that it's clean for the final product).

The tests are not passing on my Linux system, here is the result I see:
CURRENT_TEST: main.opt_trace_store_ddls
--- /Users/dgosselin/1-server/mysql-test/main/opt_trace_store_ddls.result	2025-05-30 10:11:28.972377130 -0400
+++ /Users/dgosselin/1-server/mysql-test/main/opt_trace_store_ddls.reject	2025-05-30 10:15:56.882183940 -0400
@@ -32,9 +32,6 @@
 select json_detailed(json_extract(trace, '$**.list_ddls'))
 from information_schema.optimizer_trace;
 json_detailed(json_extract(trace, '$**.list_ddls'))
-[
-    []
-]
 #
 # disable optimizer_trace, but enable store_ddls_in_optimizer_trace
 # there should be no trace here as well
@@ -47,9 +44,6 @@
 select json_detailed(json_extract(trace, '$**.list_ddls'))
 from information_schema.optimizer_trace;
 json_detailed(json_extract(trace, '$**.list_ddls'))
-[
-    []
-]
 #
 # enable optimizer_trace, but disable store_ddls_in_optimizer_trace
 # trace result should be empty

Result length mismatch

This is strange. This shouldn't have happened. Test when ran locally forced me to include this change in the result file. Changed the test a bit now. Everything is successful now.

spetrunia · 2025-06-02T11:08:19Z

Please apply this patch:

diff --git a/sql/CMakeLists.txt b/sql/CMakeLists.txt
index 74ee3adf73c..856e51c596b 100644
--- a/sql/CMakeLists.txt
+++ b/sql/CMakeLists.txt
@@ -155,7 +155,7 @@ SET (SQL_SOURCE
                sql_profile.cc event_parse_data.cc sql_alter.cc
                sql_signal.cc mdl.cc sql_admin.cc
                transaction.cc sys_vars.cc sql_truncate.cc datadict.cc
-               sql_reload.cc opt_trace_ddl_info.cc
+               sql_reload.cc
 
                # added in MariaDB:
                grant.cc
@@ -187,6 +187,7 @@ SET (SQL_SOURCE
                rowid_filter.cc rowid_filter.h
                optimizer_costs.h optimizer_defaults.h
                opt_trace.cc
+               opt_trace_ddl_info.cc
                table_cache.cc encryption.cc temporary_tables.cc
                json_table.cc
                proxy_protocol.cc backup.cc xa.cc
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index fe7b143971d..5901e3fa5aa 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -2376,7 +2376,17 @@ void ha_partition::update_create_info(HA_CREATE_INFO *create_info)
         DBUG_ASSERT(part < m_file_tot_parts);
         DBUG_ASSERT(m_file[part]);
         dummy_info.data_file_name= dummy_info.index_file_name = NULL;
-        m_file[part]->update_create_info(&dummy_info);
+        /*
+          store_table_definitions_in_trace()/show_create_table() may attempt
+          to produce DDL for a table which has only some partitions open.
+
+          We can't get options for unopened partitions. They are not relevant
+          for purpose, so it's ok to skip printing their options.
+        */
+        if (m_file[part]->is_open())
+          m_file[part]->update_create_info(&dummy_info);
+        else
+          dummy_info.init();
         sub_elem->data_file_name = (char*) dummy_info.data_file_name;
         sub_elem->index_file_name = (char*) dummy_info.index_file_name;
       }
@@ -2385,7 +2395,14 @@ void ha_partition::update_create_info(HA_CREATE_INFO *create_info)
     {
       DBUG_ASSERT(m_file[i]);
       dummy_info.data_file_name= dummy_info.index_file_name= NULL;
-      m_file[i]->update_create_info(&dummy_info);
+      /*
+        A partition might not be open, see above note about
+        store_table_definitions_in_trace()
+      */
+      if (m_file[i]->is_open())
+        m_file[i]->update_create_info(&dummy_info);
+      else
+        dummy_info.init();
       part_elem->data_file_name = (char*) dummy_info.data_file_name;
       part_elem->index_file_name = (char*) dummy_info.index_file_name;
     }
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
index 6ff65a18f63..d26a8ca734c 100644
--- a/storage/innobase/handler/ha_innodb.cc
+++ b/storage/innobase/handler/ha_innodb.cc
@@ -11446,9 +11446,6 @@ ha_innobase::update_create_info(
 /*============================*/
 	HA_CREATE_INFO*	create_info)	/*!< in/out: create info */
 {
-	if (!this->is_open())
-		return;
-
 	if (!(create_info->used_fields & HA_CREATE_USED_AUTO)) {
 		info(HA_STATUS_AUTO);
 		create_info->auto_increment_value = stats.auto_increment_value;
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
index 4def56484c9..83b8045f16f 100644
--- a/storage/myisam/ha_myisam.cc
+++ b/storage/myisam/ha_myisam.cc
@@ -2228,9 +2228,6 @@ THR_LOCK_DATA **ha_myisam::store_lock(THD *thd,
 
 void ha_myisam::update_create_info(HA_CREATE_INFO *create_info)
 {
-  if (!this->is_open())
-    return;
-
   ha_myisam::info(HA_STATUS_AUTO | HA_STATUS_CONST);
   if (!(create_info->used_fields & HA_CREATE_USED_AUTO))
   {

Please make create_view_def() just accept TABLE_LIST *view and extract definition from there.

DaveGosselin-MariaDB

My suggestion to use the mem root is not a blocker, so if it doesn't work in this case don't worry about it. But we have some other efforts afoot by another team to track memory fragmentation and ideally all of our allocations would be via the mem root interface, in part to centralize memory allocations and simplify memory usage statistics.

DaveGosselin-MariaDB · 2025-06-02T17:02:50Z

sql/opt_trace_ddl_info.cc

+}
+
+/*
+  Determines whether the tbl is a USER Defined table or not.


Consider simplifying this comment to read something like this:

@return true when the tbl argument is not a user-defined table, or if tbl is an internal temporary table or if tbl is a table belonging to the information_schema database.

DaveGosselin-MariaDB · 2025-06-02T17:16:37Z

sql/opt_trace_ddl_info.cc

+void store_table_definitions_in_trace(THD *thd)
+{
+  LEX *lex= thd->lex;
+  if (thd->variables.optimizer_trace &&


Invert the logic of this if statement and return early. Then the function will be less indented and easier to read.

DaveGosselin-MariaDB · 2025-06-02T17:21:28Z

sql/opt_trace_ddl_info.cc

+    for (TABLE_LIST *tbl= thd->lex->query_tables; tbl; tbl= tbl->next_global)
+      tables_list.push_front(tbl);
+
+    if (tables_list.is_empty() || tables_list.elements == 0)


Did checking thd->lex->query_tables == thd->lex->query_tables_last at the start of this function not work?. You could return early and also get rid of this check here. I noticed on the PR that GitHub chose to hide four of my review comments behind another link, I don't know why it does that because it is not helpful for anybody.

sorry, didn't realize this.

It actually works.

DaveGosselin-MariaDB · 2025-06-02T17:23:29Z

sql/opt_trace_ddl_info.cc

+        continue;
+
+      ddl_key=
+          (DDL_Key *) my_malloc(key_memory_trace_ddl_info, sizeof(DDL_Key), 0);


This is another candidate for using the mem root.

DaveGosselin-MariaDB · 2025-06-02T17:26:48Z

sql/opt_trace_ddl_info.cc

+        show_create_table(thd, tbl, &ddl, NULL, WITH_DB_NAME);
+
+      name_copy=
+          (char *) my_malloc(key_memory_trace_ddl_info, name.length() + 1, 0);


(and here too)

sql/opt_trace_ddl_info.cc

spetrunia · 2025-06-02T20:23:35Z

Please look at these and squash into the patch if there are no objections:

commit c87e89bbf6a81997accabc5d9e99952313efa25e (HEAD -> bb-12.1-MDEV-36483-review-input, origin/bb-12.1-MDEV-36483-review-input)
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 23:22:08 2025 +0300

    Review input: code cleanup

commit 2e41bbae36a719dbdee6aafb52d1515fb9f32fe1
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 23:21:32 2025 +0300

    Review input: Rename/restructure object names in optimizer trace.

commit 6ff23ec646b0e67781afe419bb3ec4dc4e053a9b
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 22:56:10 2025 +0300

    Review input part 2

commit 02c925952c22c81c0fe892c8e3c86b4e1930b3f4
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 22:23:20 2025 +0300

    Review input part #1.

spetrunia · 2025-06-02T20:28:53Z

Also, let's make it future-proof: please rename store_ddls_in_optimizer_trace into optimizer_record_context .

bsrikanth-mariadb · 2025-06-03T04:50:53Z

My suggestion to use the mem root is not a blocker, so if it doesn't work in this case don't worry about it. But we have some other efforts afoot by another team to track memory fragmentation and ideally all of our allocations would be via the mem root interface, in part to centralize memory allocations and simplify memory usage statistics.

Yeah, sure Dave. Changed the code to use thd->alloc(), and it works.

bsrikanth-mariadb · 2025-06-03T04:51:09Z

Also, let's make it future-proof: please rename store_ddls_in_optimizer_trace into optimizer_record_context .

sure. done

bsrikanth-mariadb · 2025-06-03T08:14:14Z

Please look at these and squash into the patch if there are no objections:

commit c87e89bbf6a81997accabc5d9e99952313efa25e (HEAD -> bb-12.1-MDEV-36483-review-input, origin/bb-12.1-MDEV-36483-review-input)
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 23:22:08 2025 +0300

    Review input: code cleanup

commit 2e41bbae36a719dbdee6aafb52d1515fb9f32fe1
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 23:21:32 2025 +0300

    Review input: Rename/restructure object names in optimizer trace.

commit 6ff23ec646b0e67781afe419bb3ec4dc4e053a9b
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 22:56:10 2025 +0300

    Review input part 2

commit 02c925952c22c81c0fe892c8e3c86b4e1930b3f4
Author: Sergei Petrunia <[email protected]>
Date:   Mon Jun 2 22:23:20 2025 +0300

    Review input part #1.

sure, done

spetrunia

Please look though this patch https://gist.github.com/spetrunia/85aae76ba071221b9d5dc203562ba68a

doing

more error handling! particularly checking of json_escape return value is important
cleanups
removal of spaces-at-line-ends.

Apply it and then it looks good.

bsrikanth-mariadb · 2025-06-03T11:50:16Z

Please look though this patch https://gist.github.com/spetrunia/85aae76ba071221b9d5dc203562ba68a

doing

more error handling! particularly checking of json_escape return value is important

cleanups

removal of spaces-at-line-ends.

Apply it and then it looks good.

sure, Thanks Sergei.

It was observed that, when doing multiple DELETE on tables: -> if there was duplicate data to delete, then no ddls were dumped to the trace. -> However, when tested with no data being deleted from the tables, then ddls of the tables were getting dumped to the trace. The problem is that store_tables_context_in_trace() is not getting invoked, from mysql_execute_command() in all the situations. The reason is that multi-table DELETE returned error when it had deleted duplicate rows. Multi-table DELETE actually finds record combinations to delete, and when it has found let's say {t1.rowX, t2.rowY, t3.rowZ}, it will attempt to save t1.rowX in the temptable for t1, t2.rowY in the temptable for t2 and so forth. When saving the row to be deleted, in the temp table, it can encounter error 121 (HA_ERR_FOUND_DUPP_KEY), and it is propagated to the caller. As a fix, I have marked that there is no error when HA_ERR_FOUND_DUPP_KEY error_code is noticed in the multi_delete::send_data()

svoj added the MariaDB Corporation label May 6, 2025

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from eec9242 to 969a713 Compare May 12, 2025 03:30

spetrunia self-requested a review May 14, 2025 13:18

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch 4 times, most recently from c31b8fd to c1c1a62 Compare May 23, 2025 10:58

spetrunia reviewed May 26, 2025

View reviewed changes

spetrunia self-requested a review May 26, 2025 12:07

spetrunia requested changes May 26, 2025

View reviewed changes

DaveGosselin-MariaDB requested changes May 30, 2025

View reviewed changes

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from 848a670 to 1c05ec7 Compare June 1, 2025 14:07

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from 1c05ec7 to 551a4bd Compare June 2, 2025 16:10

DaveGosselin-MariaDB reviewed Jun 2, 2025

View reviewed changes

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch 2 times, most recently from f4b2e4d to dd68efd Compare June 3, 2025 08:13

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from dd68efd to 902737f Compare June 3, 2025 11:00

spetrunia self-requested a review June 3, 2025 11:26

spetrunia approved these changes Jun 3, 2025

View reviewed changes

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from 902737f to eb4d1ac Compare June 3, 2025 11:47

bsrikanth-mariadb force-pushed the 12.1-MDEV-36483-dump-ddls-of-tables-views branch from eb4d1ac to 71dba7d Compare June 3, 2025 13:24

Uh oh!

MDEV-36483 Optimizer Trace Replay step #1: dump DDLs of tables/views #4034

Are you sure you want to change the base?

MDEV-36483 Optimizer Trace Replay step #1: dump DDLs of tables/views #4034

Uh oh!

Conversation

bsrikanth-mariadb commented May 6, 2025

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

DaveGosselin-MariaDB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb May 26, 2025 •

edited

Loading

bsrikanth-mariadb Jun 1, 2025 •

edited

Loading