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

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

This PR sits on top of the binlog state refactor (base branch tip:
MDEV-38865 Simplify testing if binary logging is enabled). Only the top
six commits are new; everything beneath them is the base work. Each commit
addresses one TODO item from MDEV-38796 and is intended to be reviewed on
its own, in commit order. The first commit contains only test updates, the
middle four change server code, and the last is a small build fix.

Commit walkthrough

Test updates for forced row logging (ed7f657). Test changes
only. The base branch changed ER_BINLOG_STMT_MODE_AND_ROW_ENGINE from
an error into a note: when a statement in STATEMENT mode touches a table
limited to row logging, the server now switches the statement to row
format temporarily instead of failing it. This commit updates
binlog.binlog_spurious_ddl_errors and the sql_sequence tests that
relied on the old error.
Update BINLOG_STATE_FILTER at DB change time (ec517d4).
decide_logging_format() used to call binlog_filter->db_ok() for
every statement. The result is now cached in binlog_state and
recomputed only when the default database changes (THD::set_db() and
THD::reset_db(), via the new update_binlog_filter_state());
thd_binlog_filter_ok() reads the cached bit. The flag is renamed from
BINLOG_STATE_FILTER to BINLOG_STATE_FILTER_DB, since it reflects
only the binlog_do_db/binlog_ignore_db decision on the current default
database. A new comment block in decide_logging_format() documents the
statement mode filtering semantics. Adds rpl.rpl_lsu_binlog_filter, a
chain replication test (three servers) verifying that a middle server
configured with log_slave_updates and binlog filters keeps filtered
transactions out of its own binlog.
Remove *_binlog_local_stmt_filter() (7fe6738). Deletes
THD::m_binlog_filter_state and its four accessors, since the
information is fully carried by binlog_state (BINLOG_STATE_FILTER_DB
and BINLOG_STATE_READONLY). Pure removal, net 52 lines deleted.
Remove silent option from mysql_create_db_internal
(53c847f). Replaces the special silent parameter with the standard
tmp_disable_binlog() / reenable_binlog() mechanism.
mysql_create_db_internal() now checks BINLOG_STATE_TMP_DISABLED, and
the one internal caller (mysql_upgrade_db()) wraps its call
accordingly.
Replace mysql_bin_log.is_open with binlog_ready()
(f7a90e8). The widest commit, touching about fifteen files.
mysql_bin_log.is_open() is read without a mutex on main code paths,
which makes it unreliable; those sites now use the state cached on the
THD (thd->binlog_ready_no_wsrep()). is_open() remains only where it
is legitimate: at commit time under LOCK_log, in ddl_log recovery, and
at WSREP sites deliberately deferred to MDEV-38865 (each marked with a
TODO: MDEV-38865 comment). Adds a DEBUG_SYNC point
(commit_after_binlog_ready_before_LOCK_log) and the new test
binlog.binlog_close_during_commit, which closes the binlog from a
second connection between a statement's binlog_ready() check and its
acquisition of LOCK_log, covering five statement types. The
transaction must still commit in the engine, and the error log must
report the closed binlog once per close event rather than once per
transaction.
Fix Windows compiler error (423acf7). One line: wraps a
binlog_state bit test in MY_TEST() so the comparison against bool
compiles on Windows.

Notes for review

Commits 2 and 3 form a sequence: commit 2 introduces the cached filter
state, and commit 3 removes the machinery it replaces. Commits 1, 4, 5,
and 6 are independent of that sequence and of each other.
New test coverage lives in commits 2 (rpl.rpl_lsu_binlog_filter) and 5
(binlog.binlog_close_during_commit). Commit 1 only adapts existing
tests to base branch behavior.
Two items are known and deliberately out of scope: the WSREP related
is_open() sites (tracked as MDEV-38865), and the CREATE TABLE case in
binlog_close_during_commit, which is commented out pending MDEV-40171.

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;

ParadoxV5 · 2026-07-21T20:38:41Z

PR is ongoing & draft for now

gentle nudge

bnestere · 2026-07-21T21:05:57Z

@ParadoxV5 thanks for the reminder! Also it looks like there are new test failures, so I'll keep it draft until I fix them

ER_BINLOG_STMT_MODE_AND_ROW_ENGINE is no longer an error, but rather, we temporarily force ROW logging: @@ -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(); + } } Tests are updated that relied on this behavior.

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(). The option BINLOG_STATE_FILTER only applies to filtering the current database. The name seems over-reaching, and would likely lead to confusion. Renamed to BINLOG_STATE_FILTER_DB to be clear the option only applies to the database.

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 force-pushed the main-MDEV-38796 branch from 1ffea61 to e983ad2 Compare May 28, 2026 20:16

bnestere force-pushed the main-MDEV-38796 branch from e983ad2 to cc5acda Compare June 5, 2026 21:14

bnestere force-pushed the main-MDEV-38796 branch from cc5acda to 9c78108 Compare July 21, 2026 20:26

bnestere added 5 commits July 22, 2026 15:47

MDEV-38796 Remove *_binlog_local_stmt_filter()

7fe6738

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

53c847f

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 4c713f0 to f7a90e8 Compare July 22, 2026 21:56

MDEV-38796: Fix Windows compiler error

423acf7

bnestere marked this pull request as ready for review July 23, 2026 16:22

This comment was marked as spam.

Sign in to view

bnestere requested a review from montywi July 23, 2026 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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 6 commits into
gtt-montyfrom
main-MDEV-38796

bnestere commented May 27, 2026 •

edited

Loading

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!

ParadoxV5 commented Jul 21, 2026

Uh oh!

bnestere commented Jul 21, 2026

Uh oh!

This comment was marked as spam.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Commit walkthrough

Notes for review

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!

ParadoxV5 commented Jul 21, 2026

Uh oh!

bnestere commented Jul 21, 2026

Uh oh!

This comment was marked as spam.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

bnestere commented May 27, 2026 •

edited

Loading

CLAassistant commented May 27, 2026 •

edited

Loading