Skip to content

MDEV-37602: Phase 1 & 2 - Session-scoped rgi and Master_info registration in index#5159

Open
bodyhedia44 wants to merge 1 commit into
MariaDB:mainfrom
bodyhedia44:MDEV-37602-binlog-replay-rework
Open

MDEV-37602: Phase 1 & 2 - Session-scoped rgi and Master_info registration in index#5159
bodyhedia44 wants to merge 1 commit into
MariaDB:mainfrom
bodyhedia44:MDEV-37602-binlog-replay-rework

Conversation

@bodyhedia44
Copy link
Copy Markdown
Contributor

Phase 1: Persist rpl_group_info per connection instead of per BINLOG statement
Phase 2: Create session Master_info and register in master_info_index with unique names

…tion in index

Phase 1: Persist rpl_group_info per connection instead of per BINLOG statement
Phase 2: Create session Master_info and register in master_info_index with unique names

This enables proper cross-event context and visibility in SHOW SLAVE STATUS.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a session-scoped Master_info (mi_fake) for BINLOG replay statements to eliminate the overhead of creating and deleting throwaway Master_info instances per query event. The session-scoped instance is registered in master_info_index and cleaned up during connection teardown. Feedback highlights two critical issues: first, master_info_index can be NULL on standalone servers and its modification requires holding the LOCK_active_mi mutex to avoid race conditions; second, the allocation of rpl_group_info lacks a NULL check, which could lead to a server crash if memory is exhausted.

Comment thread sql/sql_binlog.cc
Comment on lines +265 to +270
if (master_info_index->add_master_info(mi, FALSE))
{
delete mi;
my_error(ER_OUTOFMEMORY, MYF(ME_FATAL), 1);
goto end;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The global master_info_index can be NULL on standalone servers where replication is not initialized or disabled. Dereferencing it directly will cause a server crash when executing BINLOG statements. Additionally, modifying master_info_index requires holding the LOCK_active_mi mutex to prevent race conditions with other threads (e.g., concurrent SHOW STATUS or administrative commands).

Please add a NULL check for master_info_index and wrap the registration in LOCK_active_mi.

    if (master_info_index)
    {
      mysql_mutex_lock(&LOCK_active_mi);
      if (master_info_index->add_master_info(mi, FALSE))
      {
        mysql_mutex_unlock(&LOCK_active_mi);
        delete mi;
        my_error(ER_OUTOFMEMORY, MYF(ME_FATAL), 1);
        goto end;
      }
      mysql_mutex_unlock(&LOCK_active_mi);
    }

Comment thread sql/sql_binlog.cc
Comment on lines +275 to +277
if (!rgi)
rgi= thd->rgi_fake= new rpl_group_info(rli);
rgi->thd= thd;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If new rpl_group_info(rli) fails and returns NULL due to an out-of-memory condition, dereferencing rgi on the next line (rgi->thd = thd;) will cause a server crash. We should check if rgi is NULL and handle the error gracefully.

  if (!rgi)
  {
    if (!(rgi= thd->rgi_fake= new rpl_group_info(rli)))
    {
      my_error(ER_OUTOFMEMORY, MYF(ME_FATAL), 1);
      goto end;
    }
  }
  rgi->thd= thd;

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 2, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! This is a preliminary review.

According to https://github.com/MariaDB/server/blob/10.11/COMMUNITY_CONTRIBUTIONS.md

A contribution should have BOTH a design document in Jira and a pull request in GitHub.

In the normal case the process usually starts with filing a Jira for the bug or the feature proposed. Then a pull request gets created to implement the feature or fix the bug. It could be the other way around too. But it's important to always have both prior to review.

I cannot match "phase 1 and 2" to anything into the MDEV.

I would suggest to either open a sub-task inside MDEV-37602 and do a proper description there or file a completely independent jira and do proper design description into it.

Ideally it should follow the guidelines in https://mariadb.com/docs/general-resources/community/community/bug-tracking/reporting-bugs, more specifically:

Any background information you can provide (stack trace, tables, table definitions (show-create-table SHOW CREATE TABLE {tablename}), data dumps, query logs).
..
If the bug is about server producing wrong query results: the actual result (what you are getting), the expected result (what you think should be produced instead), and, unless it is obvious, the reason why you think the current result is wrong.
...
A test case or some other way to repeat the bug. This should preferably be in plain SQL or in mysqltest format. See mysqltest/README for information about this.
If it's impossible to do a test case, then providing us with a backtrace information would be of great help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

2 participants