Skip to content

Bug#121063 Members of one group assign different GNOs to the same transaction - #736

Closed
matias-sanchez wants to merge 4 commits into
mysql:trunkfrom
matias-sanchez:bug121063-stale-synode-reservation
Closed

matias-sanchez wants to merge 4 commits into
mysql:trunkfrom
matias-sanchez:bug121063-stale-synode-reservation

Conversation

@matias-sanchez

Copy link
Copy Markdown

Bug#121063: https://bugs.mysql.com/bug.php?id=121063

What happens

Two members of the same group assign different GNOs to the same transaction, in multi-primary mode, when a member leaves and rejoins during a rolling restart under write load. The group splits into internally consistent sets that disagree on the GTID of the same transaction. It can surface on the group_replication_applier channel as Error_code 1032 or 1062, or stay silent with every member ONLINE and the disagreement present only in the binary logs.

Reproduced on 8.4.8, 8.4.11 and 9.7.2. A self contained reproducer is attached to the bug.

Where it starts

A reserved synode is only ours while we still hold the node index it was reserved under.

local_synode_allocator stamps the member's current index into the synode, synode.node = my_nodeno. Still inside reserve_synode_number, the task yields in the while (too_far(*msgno)) loop at TIMED_TASK_WAIT. During that yield, site_install_action reassigns site->nodeno. The reservation still carries the old index, so proposer_task brands and proposes into a slot that now belongs to another node.

The header comment of xcom_base.cc states the rule at line 107: only node N may propose a value for synode {X N}. With two proposers on the same slot at cnt=0, acceptor.promise is never raised, since it is assigned in exactly one place, inside handle_simple_prepare, which is the phase 1 decision. Both proposals are accepted, both are learned, and handle_learn keeps whichever LEARN arrived first because of the /* Avoid re-learn */ guard. Members that heard different values first deliver different payloads.

A bpftrace trace of one such slot followed from ACCEPT through LEARN to DELIVER is in the bug, posted 28 Aug 2026.

The change

Before proposing, verify the reservation still carries the member's own node index. If it does not, drop it through retry_new and take a new one.

This is the same check incr_msgno (xcom_base.cc:668) already makes whenever it advances, with the comment In case site and node number has changed. The client transaction is not lost, it goes out in a slot that does belong to the member.

Testing

In the lab the anomaly goes to zero: 0 stale proposals out of 15,523,048, against 993 predicted from the unpatched rate, and 0 divergences out of 5,667,504 transactions, against 17.1 expected. No run carrying the patch has reproduced the divergence, on two hosts.

Performance: 12 runs per arm across two hosts, load only, no restarts, no emulated latency, comparing the patched plugin against the same source built without the patch and against the stock image. Ratio of median successful operations per 10 s window, bootstrap over runs: patched over unpatched [0.9998, 1.0004], patched over stock [0.9994, 1.0003]. A positive control with a known throttle was detected at [0.9944, 0.9953], so the method resolves differences of about 0.5%. The workload is rate limited, so this measures behaviour under a production-like load and not peak capacity.

I do not have an MTR test for this. The reproduction needs seven members, rolling restarts and sustained write load, which does not fit the standard framework. The reproducer attached to the bug builds that environment and decides the verdict by decoding every member's binary log and comparing all 21 pairs.

@matias-sanchez
matias-sanchez requested a review from a team August 28, 2026 20:57
@oracle-contributor-agreement oracle-contributor-agreement Bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 28, 2026
@github-actions github-actions Bot added Replication Changes touching replication or binlog code Pluggable Changes touching plugins or components Review Requested Review requested from code owners labels Aug 28, 2026
@gopshank
gopshank requested review from nacarvalho and removed request for gopshank and seemasundara August 29, 2026 01:21
@nacarvalho
nacarvalho requested review from jujose-1 and tiagoportelajorge and removed request for nacarvalho August 31, 2026 09:09
Comment thread plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc Outdated

@jujose-1 jujose-1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Matias,
Thank you for the contribution and for working on this fix. I’ve left a few review comments for your consideration. Please take a look when you have a chance.

Regards,
Justin Jose

@matias-sanchez
matias-sanchez force-pushed the bug121063-stale-synode-reservation branch from 288df77 to 267fed8 Compare September 2, 2026 23:41
@matias-sanchez

Copy link
Copy Markdown
Author

Hi @jujose-1, thanks for taking the time to review this and for the comments. Added the requested changes.

@jujose-1 jujose-1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @matias-sanchez ,

Thank you for updating the patch and addressing the earlier feedback. The production change looks good to me. I’ve left one additional comment regarding the unit-test setup.

I’ll also wait for @tiagoportelajorge 's review and any additional feedback he may have.

Thank you again for working on this.

Regards,
Justin Jose

Comment thread unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc Outdated
@kamil-holubicki

Copy link
Copy Markdown

My proposal is to add two debug asserts:

  1. Proposer side (offending node) in prepare_push_2p() after the ballot setup: assert(p->proposer.bal.node == p->synode.node). The node can propose only for its synode. Detect the case when the current node index mismatches the index at the synode reservation time.

  2. Acceptor side in handle_simple_accept(): assert(m->proposal.cnt != 0 || m->proposal.node == m->synode.node);. In fast path (cnt==0) the node can propose only for its synode.

@github-actions github-actions Bot added the MTR Failed MTR suite failed label Sep 3, 2026
@github-actions github-actions Bot removed the MTR Failed MTR suite failed label Sep 4, 2026
@matias-sanchez

matias-sanchez commented Sep 4, 2026 •

Copy link
Copy Markdown
Author

Thanks @kamil-holubicki . However, I'm not sure on how to safely add these asserts, because the issue covered is on multi primary and I'm not sure how to add that assert so that it does not fire when paxos_single_leader is ON, as in that mode the synode carries the leader's node index, not the proposer's. @jujose-1 would adding site->max_active_leaders != active_leaders_all be the proper way to validate this? Also, do you recommend adding this on this PR or would you prefer a follow up one?

@jujose-1

jujose-1 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Thanks, @matias-sanchez , for updating the unit test. The revised test now uses valid memberships and verifies the same reservation across the view change. This looks good to me.

Thanks also, @kamil-holubicki, for the suggestions.

@jujose-1 would adding site->max_active_leaders != active_leaders_all be the proper way to validate this? Also, do you recommend adding this on this PR or would you prefer a follow up one?

My understanding is that the proposed assertions would cover the scenario reported in this bug, but they may not hold for remotely allocated synodes, where the synode can legitimately carry the allocating leader’s node index rather than the proposer’s.

Given that the reported issue involves a locally allocated reservation, and the current fix scopes the ownership validation to that path, I suggest keeping this PR focused on that change. The acceptor side does not currently have the allocation provenance needed to apply the same invariant safely, so I would prefer not to add the proposed assertions here.

kamil-holubicki pushed a commit to kamil-holubicki/percona-server that referenced this pull request Sep 4, 2026
…t GNOs to the same transaction

https://perconadev.atlassian.net/browse/PS-11530

Upstream bug:
Bug#121063: https://bugs.mysql.com/bug.php?id=121063

Problem:
Two members of the same group assign different GNOs to the same
transaction, in multi-primary mode, when a member leaves and rejoins
during a rolling restart under write load. The group splits into
internally consistent sets that disagree on the GTID of the same
transaction. It can surface on the `group_replication_applier` channel
as Error_code 1032 or 1062, or stay silent with every member ONLINE and
the disagreement present only in the binary logs.

Cause:
A reserved synode is only ours while we still hold the node index it was
reserved under.

`local_synode_allocator` stamps the member's current index into the
synode, `synode.node = my_nodeno`. Still inside `reserve_synode_number`,
the task yields in the `while (too_far(*msgno))` loop at
`TIMED_TASK_WAIT`. During that yield, `site_install_action` reassigns
`site->nodeno`. The reservation still carries the old index, so
`proposer_task` brands and proposes into a slot that now belongs to
another node.

The header comment of `xcom_base.cc` states the rule: only node N may
propose a value for synode {X N}. With two proposers on the same slot
at `cnt=0`, `acceptor.promise` is never raised, since it is assigned in
exactly one place, inside `handle_simple_prepare`, which is the phase 1
decision. Both proposals are accepted, both are learned, and
`handle_learn` keeps whichever LEARN arrived first because of the
`/* Avoid re-learn */` guard. Members that heard different values first
deliver different payloads.

Solution:
Before proposing, verify the reservation still carries the member's own
node index. If it does not, drop it through `retry_new` and take
a new one.

This is the same check `incr_msgno` already makes whenever it advances,
with the comment `In case site and node number has changed`.
The client transaction is not lost, it goes out in a slot that does
belong to the member.

Ported from upstream MySQL PR
mysql/mysql-server#736
by Matias Sanchez.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
reserved under. A view change that renumbers us hands that slot to another
node, which may reserve it as well. */
bool_t reservation_is_stale(synode_no msgno) {
site_def const *site = find_site_def(msgno);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I need to think a bit more about this, because I don't know exactly how find_site_def behaves for synodes that were still in the proposal phase. It works well for synodes that are already in the stream and accepted by the group.

That synode might still happen, but now necessarily with the data you are proposing. That is why we have checks like if (match_my_msg(ep->p->learner.msg, ep->client_msg->p))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi Tiago. As far as I can tell find_site_def does not treat the two cases differently: it walks the site_defs list comparing the synode against each site's start, and has no notion of whether the synode was proposed, accepted or executed.

On match_my_msg, I think it catches a different scenario: it runs after finished(p), so the value is already learned and kept by handle_learn's re-learn guard by then.

Let me know if you think I might be missing something. It is tough source code i'm not entirely familiar with.

@tiagoportelajorge

Copy link
Copy Markdown
Member

HI @matias-sanchez , thank you for working on this!

I've reopened a conversation because I think that the code can be simplified. I've also dropped some food for thought, even for me to double-check the behaviour of find_site_def

@matias-sanchez

Copy link
Copy Markdown
Author

Thanks @jujose-1 and @tiagoportelajorge for the review. @tiagoportelajorge I will look deeper into the code on the points you raised and update soon, as at first glance I am still not sure how it could be simplified, so let me know any further thoughts or guidance you may have.

@tiagoportelajorge

tiagoportelajorge commented Sep 7, 2026 •

Copy link
Copy Markdown
Member

Thanks @jujose-1 and @tiagoportelajorge for the review. @tiagoportelajorge I will look deeper into the code on the points you raised and update soon, as at first glance I am still not sure how it could be simplified, so let me know any further thoughts or guidance you may have.

Hi @matias-sanchez,

Please check my comment in the reopened thread:

Hi!
Just to add a note here... the loop that follows is the main loop of the proposer, where each of the 8 proposers are waiting for the message to be accepted.
In short, I don't think that this check makes sense here, and maybe the one inside the loop is enough, because even with view changes, we keep the proposals active. They will end up being accepted in another configuration, but it will avoid the said problem.

Regards,

Tiago

@matias-sanchez

Copy link
Copy Markdown
Author

Hi @tiagoportelajorge , thanks for reposting your comment as I had missed that one. I tested you're proposal and you were right, that check was not needed, the one inside the loop is enough :) . Just pushed a commit removing it and leaving only the one inside the loop.

@github-actions github-actions Bot added Build Passed PR build passed MTR Failed MTR suite failed labels Sep 8, 2026
@matias-sanchez

Copy link
Copy Markdown
Author

Hi @tiagoportelajorge @jujose-1, only posting back to check with you if something is still pending at my side on this PR, or are we good?

@jujose-1

Copy link
Copy Markdown
Member

Hi @matias-sanchez, Thank you for checking in. There is nothing pending from your side at the moment. We are having an internal discussion on the process for integrating the change into trunk.

We’ll let you know if anything further is needed from your side. Thank you again for the contribution.

Regards,
Justin Jose

@matias-sanchez

Copy link
Copy Markdown
Author

Hi @jujose-1 thanks u so much for the reply! cheers

@github-actions github-actions Bot added the Integrate Advisory only: OCA verified and trusted approval observed; revalidate before merge label Sep 28, 2026
@RidhaOracle

Copy link
Copy Markdown
Member

/codex

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

✅ Codex PR Review completed successfully!

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • ab.chatgpt.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "ab.chatgpt.com"

See Network Configuration for more information.

Generated by Codex PR Review for #736

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the four changed files at 549d04d1. Found one Windows maintainer-build issue in the new test's header order (inline). No additional high-confidence defects found in the reservation guard or retry path. Static review only; no PR code or tests were executed, as required by this workflow. Prior inline review threads were unavailable under the read-access policy.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • ab.chatgpt.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "ab.chatgpt.com"

See Network Configuration for more information.

Generated by Codex PR Review for #736 · codex · gpt60 · 175.9 AIC · ⌖ 24.1 AIC · ⊞ 14K
Comment /codex to run again

Comment on lines +24 to +26
#include <xcom/xcom_profile.h>
#include <xcom_vp.h>
#include "gcs_base_test.h"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P2] Handle the Windows header-order warning in the new test

Including xcom_vp.h before gcs_base_test.h brings in the Windows headers through the bundled SunRPC headers before the standard integer headers, triggering the known UINT8_MAX macro redefinition (MSVC C4005). The existing gcs_xcom_site_def-t.cc uses this same include order and has a file-specific /wd4005 in unittest/gunit/libmysqlgcs/CMakeLists.txt; that suppression does not cover this new test, and mysqlgcs's suppression is PRIVATE. With MYSQL_MAINTAINER_MODE enabling /WX, the new test therefore breaks the Windows build. Include gcs_base_test.h first, as the other XCom tests do, or extend the existing Windows suppression to this source.

@mysql-oca-bot

Copy link
Copy Markdown

Hi, thank you for your contribution. Your code has been assigned to an internal queue. Please follow
bug http://bugs.mysql.com/bug.php?id=121387 for updates.
Thanks

@RidhaOracle RidhaOracle added the Merged Merged Pull Request. label Oct 1, 2026
inikep pushed a commit to Percona-Lab/percona-server-linear that referenced this pull request Oct 7, 2026
Blueprint
https://blueprints.launchpad.net/percona-server/+spec/audit-log-default-db

Add field `DB' for `MYSQL_AUDIT_GENERAL_STATUS' records.

==========================================

[mysql#786] Bug 1610242: Reading past the end of heap buffer on saving the default DB in audit p

Original title:
[mysql#786] Bug 1610242: Reading past the end of heap buffer on saving the default DB in audit plugin

memcpy in the call

memcpy(local->db, event_general->general_query, sizeof(local->db));

always copies `NAME_LEN' bytes, but the real length of the
`general_query' can be less than `NAME_LEN'

Fix is to copy `general_query_length' bytes.

==========================================

[#930] Bug 1613647: Sporadic audit_log_filter_users failures

Insert silent statements (which aren't are grepped away) before
connects to make sure the previous statement we which want to see in
the audit log actually gets there before the connect.

==========================================

[#941] Bug 1617833: audit_log_default_db failures

1. Insert silent statement after UNINSTALL PLUGIN to make sure that SHOW
   WARNINGS is logged before the statement from the next connection.

2. Insert silent statement before establishing the connection which does
   'INSTALL PLUGIN', make sure 'SELECT * FROM t' gets to the log.

3. Make reconnect in case of error optional for 'change_user' mysqltest
   command

==========================================

[#1682] Bug 1650294: Test main.audit_log_filter_users is unstable

The source of instability is the fact that event of creating new
connection and closing previous one are not synchronized. It makes
"Quit" record to appear after "Connect" one.

Fix is to replace `include/wait_until_disconnected.inc' which waits for
client connection to be closed with
`include/wait_until_count_sessions.inc' which waits for specific
`Threads_connected' value which is updated after
`MYSQL_AUDIT_NOTIFY_CONNECTION_DISCONNECT'.

==========================================

[#1762] Bug 1626559: Test `main.audit_log_default_db' is unstable

Sometimes "SHOW WARNINGS" appears in the audit log around UNINSTALL
PLUGIN / INSTALL PLUGIN. This is the warning which MySQL issue about
plugin being in use.

Normally warning should not make it to audit log because test case
truncates log after UNINSTALL PLUGIN. But sometimes mysql-test picks
this warning up after plugin has been reinstalled and it gets logged
into audit log.

This patch makes sure that test execution continues only when plugin
disappeared from the `mysql.plgins', which happens after `deinit' is
called and all plugin is fully shut down.

==========================================

[mysql#736] Allow to log only queries for specific DBs in audit log plugin

Blueprint:

[https://blueprints.launchpad.net/percona-server/+spec/audit-filer-db]

Add two global variables:

- `audit_log_include_databases' comma separated list of databases to
  include in audit logging
- `audit_log_exclude_databases' comma separated list of databases to
  exclude from audit logging

Allow escaped backticks in the `next_word' function. This change needs
to be backported to the 5.6.

Patch changes behaviour of user and database filters. User filter
becomes case sensitive for user name and case insensitive for host name.
Database filter becomes case insensitive.

[#931] Bug 1617828: audit_log_filter_db failures

The fix increases timeout for event shot wait to 10 minutes and adds
silent query at the end of the event.

============================================================

[mysql#826] Add filtering by `sql_command'

Blueprint:

https://blueprints.launchpad.net/percona-server/+spec/audit-filer-command

- add option `audit_log_include_command' to specify commands to log
- add option `audit_log_exclude_command' to specify commands to exclude
  from logging
- backport changes for `next_word' fixing the capture of default
  database with quoted backticks.
- backport case-sensitivity fix for account filters

==========================================

[#1381] Bug 1650321: Valgrind error on main.audit_log_filter_commands

Valgrind complained about `cmd' being used uninitialized. However, I
don't a path when it could happen, since the first command was to
initialize it unconditionally.

Still this error message pointed me to the fact that we don't need `cmd'
at all, we can simply use `name' and `length' provided as arguments.

==========================================

[#1444] Bug 1663251: ASan errors on main.audit_log_filter_commands

`my_hash_search' takes two arguments - key to search for and it's
length. When length is 0, `my_hash_search' considers that key has the
same length as hash length.

Since this behaviour may be relied on somewhere, the fix is to avoid
calling `my_hash_search' for empty strings.

==========================================

[#846] Bug 1614444: Assertion `local->stack.frames[local->stack.top].query == event_general->general_query.str' failed

It is a bug which can affects filtering by DB for triggers.

There is a trigger

    CREATE TRIGGER tr1 BEFORE INSERT ON t1 FOR EACH ROW SET @Aux=1;

and query

    INSERT INTO t1(c5,c6)VALUES (1,0);

Lets see which events are sent to the audit_log:

1. TABLE ACCESS event for "INSERT INTO t1(c5,c6)VALUES (1,0)"
2. STATUS event for "SET @Aux=1"
3. STATUS event for "INSERT INTO t1(c5,c6)VALUES (1,0)"

When (1) comes, plugin pushes "INSERT INTO t1(c5,c6)VALUES (1,0)" to
the top of the stack, so it can match it later with STATUS event.

When (2) comes, plugin is trying to match "SET @Aux=1" against the stack
top. But "SET @Aux=1" isn't on the stack since it doesn't access any
database. In debug mode we see the assertion failure. In the release
mode, "SET @Aux=1" will be attributed as accessing table "t1", which is
wrong.

The fix is to replace assert with if statement. Now the query the within
trigger which doesn't access any database will always be logged just
like any similar query executed outside of the trigger.

============================================================

[#1176] Bug 1641910: Trying to set audit_log_exclude_accounts crashes server

The root cause of this bug is that logic around filtering variables rely
heavily on the fact that CHECK and UPDATE functions are always called.
This is not true when variables passed via defaults file or set as
command line options.

The fix is add special handling for filtering variables on startup.

==========================================

[#1451] Bug 1666496: Server crashes on startup if plugin is unable to create file pointed by --audit_log_file

In order to expose this bug we need to set both
--audit-log-exclude-commands and --audit-log-file at start up.

Second one has to be incorrect, so that plugin would refuse to start
with error:

    mysqld: File './data-dir/mysql-audit.json' not found (Errcode: 2 - No such file or directory)
    2017-02-21T11:58:42.392280Z 0 [ERROR] Plugin audit_log reported: 'Cannot open file ./data-dir/mysql-audit.json.'
    2017-02-21T11:58:42.392350Z 0 [ERROR] Plugin audit_log reported: 'Error: No such file or directory'
    2017-02-21T11:58:42.392355Z 0 [ERROR] Plugin 'audit_log' init function returned error.
    2017-02-21T11:58:42.392360Z 0 [ERROR] Plugin 'audit_log' registration as a AUDIT failed.

The cause for the crash is common with bug 1641910. MEMALLOC'ed
variables are not properly initialized by server (UPDATE and CHECK
functions aren't invoked).

The fix is to make sure that initialization of MEMALLOC'ed variables is
the very first thing we do.

==========================================

[#2032] lp1716844: Escaping control characters in the audit log

Implemented escaping for the JSON output format, and updated the test suite.

The other output formats are left unchanged, as they do not have a well defined method for escaping these characters.
XML does support control characters in version 1.1, but most tools only understand 1.0, and our output is 1.0.

==========================================

[g5]PS-5363 (Merge MySQL 8.0.17): fixed audit_log.audit_log_default_db MTR test case

Similarly to what Oracle did in treir fix for Bug #29248047
"5.7 AUDIT PLUGIN DOES NOT LOG WHO UNINSTALLS THE AUDIT PLUGIN UNLIKE 5.6"
(commit mysql/mysql-server@6a893b8)
added '--source include/disconnect_connections.inc' to
'audit_log.audit_log_default_db' MTR test case after plugin uninstallation.
inikep pushed a commit to Percona-Lab/percona-server-linear that referenced this pull request Oct 7, 2026
Blueprint
https://blueprints.launchpad.net/percona-server/+spec/audit-log-default-db

Add field `DB' for `MYSQL_AUDIT_GENERAL_STATUS' records.

==========================================

[mysql#786] Bug 1610242: Reading past the end of heap buffer on saving the default DB in audit p

Original title:
[mysql#786] Bug 1610242: Reading past the end of heap buffer on saving the default DB in audit plugin

memcpy in the call

memcpy(local->db, event_general->general_query, sizeof(local->db));

always copies `NAME_LEN' bytes, but the real length of the
`general_query' can be less than `NAME_LEN'

Fix is to copy `general_query_length' bytes.

==========================================

[#930] Bug 1613647: Sporadic audit_log_filter_users failures

Insert silent statements (which aren't are grepped away) before
connects to make sure the previous statement we which want to see in
the audit log actually gets there before the connect.

==========================================

[#941] Bug 1617833: audit_log_default_db failures

1. Insert silent statement after UNINSTALL PLUGIN to make sure that SHOW
   WARNINGS is logged before the statement from the next connection.

2. Insert silent statement before establishing the connection which does
   'INSTALL PLUGIN', make sure 'SELECT * FROM t' gets to the log.

3. Make reconnect in case of error optional for 'change_user' mysqltest
   command

==========================================

[#1682] Bug 1650294: Test main.audit_log_filter_users is unstable

The source of instability is the fact that event of creating new
connection and closing previous one are not synchronized. It makes
"Quit" record to appear after "Connect" one.

Fix is to replace `include/wait_until_disconnected.inc' which waits for
client connection to be closed with
`include/wait_until_count_sessions.inc' which waits for specific
`Threads_connected' value which is updated after
`MYSQL_AUDIT_NOTIFY_CONNECTION_DISCONNECT'.

==========================================

[#1762] Bug 1626559: Test `main.audit_log_default_db' is unstable

Sometimes "SHOW WARNINGS" appears in the audit log around UNINSTALL
PLUGIN / INSTALL PLUGIN. This is the warning which MySQL issue about
plugin being in use.

Normally warning should not make it to audit log because test case
truncates log after UNINSTALL PLUGIN. But sometimes mysql-test picks
this warning up after plugin has been reinstalled and it gets logged
into audit log.

This patch makes sure that test execution continues only when plugin
disappeared from the `mysql.plgins', which happens after `deinit' is
called and all plugin is fully shut down.

==========================================

[mysql#736] Allow to log only queries for specific DBs in audit log plugin

Blueprint:

[https://blueprints.launchpad.net/percona-server/+spec/audit-filer-db]

Add two global variables:

- `audit_log_include_databases' comma separated list of databases to
  include in audit logging
- `audit_log_exclude_databases' comma separated list of databases to
  exclude from audit logging

Allow escaped backticks in the `next_word' function. This change needs
to be backported to the 5.6.

Patch changes behaviour of user and database filters. User filter
becomes case sensitive for user name and case insensitive for host name.
Database filter becomes case insensitive.

[#931] Bug 1617828: audit_log_filter_db failures

The fix increases timeout for event shot wait to 10 minutes and adds
silent query at the end of the event.

============================================================

[mysql#826] Add filtering by `sql_command'

Blueprint:

https://blueprints.launchpad.net/percona-server/+spec/audit-filer-command

- add option `audit_log_include_command' to specify commands to log
- add option `audit_log_exclude_command' to specify commands to exclude
  from logging
- backport changes for `next_word' fixing the capture of default
  database with quoted backticks.
- backport case-sensitivity fix for account filters

==========================================

[#1381] Bug 1650321: Valgrind error on main.audit_log_filter_commands

Valgrind complained about `cmd' being used uninitialized. However, I
don't a path when it could happen, since the first command was to
initialize it unconditionally.

Still this error message pointed me to the fact that we don't need `cmd'
at all, we can simply use `name' and `length' provided as arguments.

==========================================

[#1444] Bug 1663251: ASan errors on main.audit_log_filter_commands

`my_hash_search' takes two arguments - key to search for and it's
length. When length is 0, `my_hash_search' considers that key has the
same length as hash length.

Since this behaviour may be relied on somewhere, the fix is to avoid
calling `my_hash_search' for empty strings.

==========================================

[#846] Bug 1614444: Assertion `local->stack.frames[local->stack.top].query == event_general->general_query.str' failed

It is a bug which can affects filtering by DB for triggers.

There is a trigger

    CREATE TRIGGER tr1 BEFORE INSERT ON t1 FOR EACH ROW SET @Aux=1;

and query

    INSERT INTO t1(c5,c6)VALUES (1,0);

Lets see which events are sent to the audit_log:

1. TABLE ACCESS event for "INSERT INTO t1(c5,c6)VALUES (1,0)"
2. STATUS event for "SET @Aux=1"
3. STATUS event for "INSERT INTO t1(c5,c6)VALUES (1,0)"

When (1) comes, plugin pushes "INSERT INTO t1(c5,c6)VALUES (1,0)" to
the top of the stack, so it can match it later with STATUS event.

When (2) comes, plugin is trying to match "SET @Aux=1" against the stack
top. But "SET @Aux=1" isn't on the stack since it doesn't access any
database. In debug mode we see the assertion failure. In the release
mode, "SET @Aux=1" will be attributed as accessing table "t1", which is
wrong.

The fix is to replace assert with if statement. Now the query the within
trigger which doesn't access any database will always be logged just
like any similar query executed outside of the trigger.

============================================================

[#1176] Bug 1641910: Trying to set audit_log_exclude_accounts crashes server

The root cause of this bug is that logic around filtering variables rely
heavily on the fact that CHECK and UPDATE functions are always called.
This is not true when variables passed via defaults file or set as
command line options.

The fix is add special handling for filtering variables on startup.

==========================================

[#1451] Bug 1666496: Server crashes on startup if plugin is unable to create file pointed by --audit_log_file

In order to expose this bug we need to set both
--audit-log-exclude-commands and --audit-log-file at start up.

Second one has to be incorrect, so that plugin would refuse to start
with error:

    mysqld: File './data-dir/mysql-audit.json' not found (Errcode: 2 - No such file or directory)
    2017-02-21T11:58:42.392280Z 0 [ERROR] Plugin audit_log reported: 'Cannot open file ./data-dir/mysql-audit.json.'
    2017-02-21T11:58:42.392350Z 0 [ERROR] Plugin audit_log reported: 'Error: No such file or directory'
    2017-02-21T11:58:42.392355Z 0 [ERROR] Plugin 'audit_log' init function returned error.
    2017-02-21T11:58:42.392360Z 0 [ERROR] Plugin 'audit_log' registration as a AUDIT failed.

The cause for the crash is common with bug 1641910. MEMALLOC'ed
variables are not properly initialized by server (UPDATE and CHECK
functions aren't invoked).

The fix is to make sure that initialization of MEMALLOC'ed variables is
the very first thing we do.

==========================================

[#2032] lp1716844: Escaping control characters in the audit log

Implemented escaping for the JSON output format, and updated the test suite.

The other output formats are left unchanged, as they do not have a well defined method for escaping these characters.
XML does support control characters in version 1.1, but most tools only understand 1.0, and our output is 1.0.

==========================================

[g5]PS-5363 (Merge MySQL 8.0.17): fixed audit_log.audit_log_default_db MTR test case

Similarly to what Oracle did in treir fix for Bug #29248047
"5.7 AUDIT PLUGIN DOES NOT LOG WHO UNINSTALLS THE AUDIT PLUGIN UNLIKE 5.6"
(commit mysql/mysql-server@6a893b8)
added '--source include/disconnect_connections.inc' to
'audit_log.audit_log_default_db' MTR test case after plugin uninstallation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build Passed PR build passed Integrate Advisory only: OCA verified and trusted approval observed; revalidate before merge Merged Merged Pull Request. MTR Failed MTR suite failed OCA Verified All contributors have signed the Oracle Contributor Agreement. Pluggable Changes touching plugins or components Replication Changes touching replication or binlog code Review Requested Review requested from code owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants