MDEV-38796 Refactor: clean up THD::decide_logging_format() checks by bnestere · Pull Request #5136 · MariaDB/server

bnestere · 2026-05-27T18:34:25Z

PR is ongoing & draft for now, making this a PR so I can get intermittent BB results.

Test binlog_spurious_ddl_errors is updated here because now decide_logging_format doesn't error out when it can't log in statement format, but just switches to row (aa4b22c). @@ -7259,7 +7262,15 @@ int THD::decide_logging_format(TABLE_LIST *tables) wsrep_cs().mode() == wsrep::client_state::m_local),1)) { - my_error((error= ER_BINLOG_STMT_MODE_AND_ROW_ENGINE), MYF(0), ""); + if (is_current_stmt_binlog_format_stmt()) + { + my_printf_error(ER_BINLOG_STMT_MODE_AND_ROW_ENGINE, + "Statement cannot be logged as STATEMENT as at " + "least one table is limited to row-based logging." + " Switching temporarly to row format", + MYF(ME_NOTE)); + set_current_stmt_binlog_format_row(); + } }

CLAassistant · 2026-05-27T18:35:11Z

All committers have signed the CLA.

gemini-code-assist

Code Review

This pull request introduces Global Temporary Tables (GTT) support, implementing the syntax, lifecycle, transaction integration, and replication/binlogging adjustments. Review feedback highlights several critical issues: a debug fprintf statement was left active in thd_binlog_filter_ok(); the in-use check in drop_tmp_table_share() is bypassed when table is NULL; committed in All_tmp_tables_list and binlog_state in THD are uninitialized; thd->query() can return NULL in log_write_error(), potentially causing a crash; and table_ref->table should be passed to drop_tmp_table_share() during truncate table operations instead of NULL.

gemini-code-assist · 2026-05-27T18:39:02Z

+  bool new_value= !(thd->binlog_state & BINLOG_STATE_FILTER);
+  bool old_value= binlog_filter->db_ok(thd->db.str);
+  //if (filter_on != old_value_on)
+    fprintf(stderr,
+            "\n\tDB %s thd_binlog_filter return\n\t\tbinlog_state: %d "
+            "\n\t\tlegacy: %d\n",
+            thd->db.str, new_value, old_value);
+  return new_value;


The debug fprintf statement is left active and will spam stderr for every call to thd_binlog_filter_ok(). This should be removed or placed under a debug macro.

return !(thd->binlog_state & BINLOG_STATE_FILTER);

gemini-code-assist · 2026-05-27T18:39:02Z

+  if (table)
  {
-    if (tab != table && tab->query_id != 0)
+    /* Table might be in use by some outer statement. */
+    All_share_tables_list::Iterator it(share->all_tmp_tables);
+    while ((tab= it++))
    {
-      /* Found a table instance in use. This table cannot be be dropped. */
-      my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr());
-      result= true;
-      goto end;
+      if (tab != table && tab->query_id != 0)
+      {
+        /* Found a table instance in use. This table cannot be dropped. */
+        my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr());
+        result= true;
+        goto end;
+      }
    }
  }


If table is NULL (which happens when called from global_tmp_drop_database or truncate_table), the check for active table instances in use is completely bypassed because of the if (table) guard. This can lead to use-after-free or crashes if any of the global temporary table instances are currently in use by an outer query. The check should be performed regardless of whether table is NULL or not, and if table is NULL, we should check all instances.

/* Table might be in use by some outer statement. */ All_share_tables_list::Iterator it(share->all_tmp_tables); while ((tab= it++)) { if (tab != table && tab->query_id != 0) { /* Found a table instance in use. This table cannot be dropped. */ my_error(ER_CANT_REOPEN_TABLE, MYF(0), table ? table->alias.c_ptr() : share->table_name.str); result= true; goto end; } }

gemini-code-assist · 2026-05-27T18:39:02Z

+                                      I_P_List_null_counter,
+                                      I_P_List_fast_push_back<TMP_TABLE_SHARE>>
+{
+  bool committed;


committed is not initialized in-class or via a constructor, which means it will contain an undefined/garbage value when All_tmp_tables_list is instantiated. It should be initialized to false.

bool committed= false;

gemini-code-assist · 2026-05-27T18:39:02Z


+void log_write_error(THD *thd, const char *msg)
+{
+  my_error(ER_BINLOG_LOGGING_IMPOSSIBLE, MYF(ME_ERROR_LOG), msg, thd->query());


thd->query() can return NULL if there is no active query (e.g., in background or replication threads). Passing NULL to %s in my_error can lead to undefined behavior or crashes on some platforms. It should be guarded with a null check.

my_error(ER_BINLOG_LOGGING_IMPOSSIBLE, MYF(ME_ERROR_LOG), msg, thd->query() ? thd->query() : "");

gemini-code-assist · 2026-05-27T18:39:02Z

+  {
+    TABLE_SHARE *s= table_ref->table->s;
+    DBUG_ASSERT(s->tmp_table);
+    error= thd->drop_tmp_table_share(NULL, (TMP_TABLE_SHARE *)s, true);


We should pass table_ref->table instead of NULL to drop_tmp_table_share so that the function can correctly exclude the current table instance from the "in-use" check while still validating other active instances.

error= thd->drop_tmp_table_share(table_ref->table, (TMP_TABLE_SHARE *)s, true);

gemini-code-assist · 2026-05-27T18:39:02Z

+    Describe the current state of binary logging for the current statement.
+    Includes state of binary logging and also what disables it
+  */
+  enum_binlog_state binlog_state;


binlog_state is not initialized in-class, which can lead to undefined behavior if it is read before THD::init() is called. It should be initialized to BINLOG_STATE_NONE.

enum_binlog_state binlog_state= BINLOG_STATE_NONE;

Addresses TODO - Update binlog_state with BINLOG_STATE_FILTER only when database or filter is changed. This means we do not have to call binlog_filter->db_ok() for every statement in decide_binlog_format().

TODO Addressed: - Remove clear/reset xxx_binlog_local_stmt_filter() as this is now handled by binlog_state.

TODO addressed - Remove 'silent' option from mysql_create_db_internal and instead use tmp_disable_binlog() / reenable_binlog()

Addresses TODO - Remove all testing of mysql_bin_log.is_open(). Instead test for binlog_ready() in main code and add testing of is_open() when trying to commit the binary log. This is needed as currently mysql_bin_log.is_open() is tested without a mutex which makes it unreliable. And also adds tests for the binlog closing for various types of statements in-between a thd->binlog_ready() check and the commit-time acquiring of mysql_bin_log.LOCK_log. Still TODO from the above request: * WSREP related mysql_bin_log.is_open() changes as a part of MDEV-38865

bnestere self-assigned this May 27, 2026

bnestere added MariaDB Corporation Replication Patches involved in replication labels May 27, 2026

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

bnestere changed the base branch from main to gtt-monty May 27, 2026 18:45

bnestere added 4 commits May 28, 2026 14:15

MDEV-38796 Update BINLOG_STATE_FILTER at DB change time

b122852

Addresses TODO - Update binlog_state with BINLOG_STATE_FILTER only when database or filter is changed. This means we do not have to call binlog_filter->db_ok() for every statement in decide_binlog_format().

MDEV-38796 Remove *_binlog_local_stmt_filter()

fe705ac

TODO Addressed: - Remove clear/reset xxx_binlog_local_stmt_filter() as this is now handled by binlog_state.

MDEV-38796 Remove silent option from mysql_create_db_internal

2247f9d

TODO addressed - Remove 'silent' option from mysql_create_db_internal and instead use tmp_disable_binlog() / reenable_binlog()

bnestere force-pushed the main-MDEV-38796 branch from 1ffea61 to e983ad2 Compare May 28, 2026 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MDEV-38796 Refactor: clean up THD::decide_logging_format() checks#5136

MDEV-38796 Refactor: clean up THD::decide_logging_format() checks#5136
bnestere wants to merge 5 commits into
gtt-montyfrom
main-MDEV-38796

bnestere commented May 27, 2026

Uh oh!

CLAassistant commented May 27, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bnestere commented May 27, 2026

Uh oh!

CLAassistant commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

CLAassistant commented May 27, 2026 •

edited

Loading