From 4276b1160247da646cb0cd12cc68d6e5a97e58a9 Mon Sep 17 00:00:00 2001 From: Matias Sanchez Date: Fri, 28 Aug 2026 17:17:00 -0300 Subject: [PATCH 1/4] Bug#121063 Members of one group assign different GNOs to the same transaction --- .../src/bindings/xcom/xcom/xcom_base.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc index 9fee175b8634..25b7f4bef2cd 100644 --- a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc +++ b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc @@ -1756,6 +1756,15 @@ static void push_msg_3p(site_def const *site, pax_machine *p, pax_msg *msg, STRLIT(pax_op_to_str(msg->op))); } +/* A reserved synode is only ours while we still hold the node index it was + reserved under. A view change that renumbers us hands that slot to another + node, which may reserve it as well. */ +static bool_t reservation_is_stale(synode_no msgno) { + site_def const *site = find_site_def(msgno); + node_no me = site ? get_nodeno(site) : VOID_NODE_NO; + return me != VOID_NODE_NO && me != msgno.node; +} + /* Brand client message with unique ID */ static void brand_client_msg(pax_msg *msg, synode_no msgno) { assert(!synode_eq(msgno, null_synode)); @@ -2520,6 +2529,13 @@ static int proposer_task(task_arg arg) { brand_client_msg(ep->client_msg->p, ep->msgno); + /* Only a locally allocated synode carries our own node index; a remote or + global allocation carries the allocating leader's, by design. */ + if (ep->synode_allocation == synode_allocation_type::local && + reservation_is_stale(ep->msgno)) { + GOTO(retry_new); + } + for (;;) { /* Loop until the client message has been learned */ /* Get a Paxos instance to send the client message */ From 267fed85a22d654de4d0211648a5571c803f75ed Mon Sep 17 00:00:00 2001 From: Matias Sanchez Date: Wed, 2 Sep 2026 20:19:47 -0300 Subject: [PATCH 2/4] Bug#121063 Revalidate the reservation after wait_for_cache, with a unit test --- .../src/bindings/xcom/xcom/xcom_base.cc | 9 +- .../src/bindings/xcom/xcom/xcom_base.h | 2 + unittest/gunit/libmysqlgcs/CMakeLists.txt | 1 + .../xcom/gcs_xcom_stale_reservation-t.cc | 100 ++++++++++++++++++ 4 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc diff --git a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc index 25b7f4bef2cd..94ebe57f9c7e 100644 --- a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc +++ b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc @@ -1759,7 +1759,7 @@ static void push_msg_3p(site_def const *site, pax_machine *p, pax_msg *msg, /* A reserved synode is only ours while we still hold the node index it was reserved under. A view change that renumbers us hands that slot to another node, which may reserve it as well. */ -static bool_t reservation_is_stale(synode_no msgno) { +bool_t reservation_is_stale(synode_no msgno) { site_def const *site = find_site_def(msgno); node_no me = site ? get_nodeno(site) : VOID_NODE_NO; return me != VOID_NODE_NO && me != msgno.node; @@ -2546,6 +2546,13 @@ static int proposer_task(task_arg arg) { goto retry_new; } + /* Checked again after wait_for_cache: that call can suspend, and a view + change during the wait leaves the reservation stale. */ + if (ep->synode_allocation == synode_allocation_type::local && + reservation_is_stale(ep->msgno)) { + GOTO(retry_new); + } + assert(ep->p); if (ep->client_msg->p->force_delivery) ep->p->force_delivery = ep->client_msg->p->force_delivery; diff --git a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.h b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.h index c83ac928b2b8..ebdd2cae95f3 100644 --- a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.h +++ b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.h @@ -70,6 +70,8 @@ void *xcom_thread_main(void *cp); synode_no incr_synode(synode_no synode); +bool_t reservation_is_stale(synode_no msgno); + synode_no decr_synode(synode_no synode); char *dbg_pax_msg(pax_msg const *p); diff --git a/unittest/gunit/libmysqlgcs/CMakeLists.txt b/unittest/gunit/libmysqlgcs/CMakeLists.txt index 27d9535d84c5..fbc87757e566 100644 --- a/unittest/gunit/libmysqlgcs/CMakeLists.txt +++ b/unittest/gunit/libmysqlgcs/CMakeLists.txt @@ -75,6 +75,7 @@ SET(GCS_XCOM_TESTS xcom/gcs_xcom_xcom_transport xcom/gcs_xcom_communication_protocol_changer xcom/gcs_xcom_xcom_cache + xcom/gcs_xcom_stale_reservation xcom/gcs_xcom_control_interface xcom/gcs_xcom_view_identifier xcom/gcs_message_stage_fragmentation diff --git a/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc b/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc new file mode 100644 index 000000000000..0a733c6ae58b --- /dev/null +++ b/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc @@ -0,0 +1,100 @@ +/* Copyright (c) 2026, Oracle and/or its affiliates. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License, version 2.0, + as published by the Free Software Foundation. + + This program is designed to work with certain software (including + but not limited to OpenSSL) that is licensed under separate terms, + as designated in a particular file or component or in included license + documentation. The authors of MySQL hereby grant you an additional + permission to link the program and your derivative works with the + separately licensed software that they have either included with + the program or referenced in the documentation. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License, version 2.0, for more details. + + 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 St, Fifth Floor, Boston, MA 02110-1301 USA */ + +#include +#include +#include "gcs_base_test.h" + +#include "site_def.h" +#include "xcom_base.h" + +namespace xcom_stale_reservation_unittest { + +/* Bug#121063: a synode reserved under one node index must be recognised as no + longer ours once a view change gives this member a different index. */ +class XcomStaleReservation : public GcsBaseTest { + protected: + void SetUp() override { + m_addr = new std::string("127.0.0.1:12345"); + char const *names[]{m_addr->c_str()}; + m_na = new_node_address(1, names); + + /* The view in force when the synode is reserved: this member is node 1. */ + m_before = new_site_def(); + init_site_def(1, m_na, m_before); + m_before->start = m_synode_before; + m_before->nodeno = 1; + push_site_def(m_before); + + /* The view installed while the reservation is held: the member is now + node 0. */ + m_after = new_site_def(); + init_site_def(1, m_na, m_after); + m_after->start = m_synode_after; + m_after->nodeno = 0; + push_site_def(m_after); + } + + void TearDown() override { + push_site_def(nullptr); + free_site_defs(); + delete_node_address(1, m_na); + delete m_addr; + } + + std::string *m_addr{nullptr}; + node_address *m_na{nullptr}; + site_def *m_before{nullptr}; + site_def *m_after{nullptr}; + + /* A view is active from its start synode on, so a slot below 20 falls under + the old view and one at or above it under the new one. */ + synode_no const m_synode_before{1, 10, 0}; + synode_no const m_synode_after{1, 20, 0}; +}; + +/* Under the view it was taken in, the reservation is ours. */ +TEST_F(XcomStaleReservation, reservation_under_the_current_index_is_fresh) { + synode_no const reserved{1, 15, 1}; + ASSERT_FALSE(reservation_is_stale(reserved)); +} + +/* After the renumbering, the same index belongs to another node. */ +TEST_F(XcomStaleReservation, reservation_outliving_a_renumbering_is_stale) { + synode_no const reserved{1, 25, 1}; + ASSERT_TRUE(reservation_is_stale(reserved)); +} + +/* A slot that carries the index this member holds now is usable. */ +TEST_F(XcomStaleReservation, slot_matching_the_new_index_is_fresh) { + synode_no const reserved{1, 25, 0}; + ASSERT_FALSE(reservation_is_stale(reserved)); +} + +/* Without a site there is nothing to compare against. */ +TEST_F(XcomStaleReservation, unknown_site_is_not_reported_stale) { + synode_no const other_group{99, 25, 1}; + ASSERT_FALSE(reservation_is_stale(other_group)); +} + +} // namespace xcom_stale_reservation_unittest From 971d2b85bc014f02eb0d5110347281fd89e90593 Mon Sep 17 00:00:00 2001 From: Matias Sanchez Date: Thu, 3 Sep 2026 22:00:05 -0300 Subject: [PATCH 3/4] Bug#121063 Rework the unit test to use valid memberships and a real view change --- .../xcom/gcs_xcom_stale_reservation-t.cc | 60 +++++++++---------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc b/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc index 0a733c6ae58b..ca19847bf5b2 100644 --- a/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc +++ b/unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc @@ -35,46 +35,39 @@ namespace xcom_stale_reservation_unittest { class XcomStaleReservation : public GcsBaseTest { protected: void SetUp() override { - m_addr = new std::string("127.0.0.1:12345"); - char const *names[]{m_addr->c_str()}; - m_na = new_node_address(1, names); - /* The view in force when the synode is reserved: this member is node 1. */ - m_before = new_site_def(); - init_site_def(1, m_na, m_before); - m_before->start = m_synode_before; - m_before->nodeno = 1; - push_site_def(m_before); - - /* The view installed while the reservation is held: the member is now - node 0. */ - m_after = new_site_def(); - init_site_def(1, m_na, m_after); - m_after->start = m_synode_after; - m_after->nodeno = 0; - push_site_def(m_after); + char const *names[]{"127.0.0.1:12341", "127.0.0.1:12342", + "127.0.0.1:12343"}; + node_address *na = new_node_address(3, names); + + site_def *before = new_site_def(); + init_site_def(3, na, before); + before->start = synode_no{1, 10, 0}; + before->nodeno = 1; + push_site_def(before); + delete_node_address(3, na); } - void TearDown() override { - push_site_def(nullptr); - free_site_defs(); - delete_node_address(1, m_na); - delete m_addr; + /* The view installed while the reservation is held: the first member is + gone, so this one is now node 0. */ + void install_new_view() { + char const *names[]{"127.0.0.1:12342", "127.0.0.1:12343"}; + node_address *na = new_node_address(2, names); + + site_def *after = new_site_def(); + init_site_def(2, na, after); + after->start = synode_no{1, 20, 0}; + after->nodeno = 0; + push_site_def(after); + delete_node_address(2, na); } - std::string *m_addr{nullptr}; - node_address *m_na{nullptr}; - site_def *m_before{nullptr}; - site_def *m_after{nullptr}; - - /* A view is active from its start synode on, so a slot below 20 falls under - the old view and one at or above it under the new one. */ - synode_no const m_synode_before{1, 10, 0}; - synode_no const m_synode_after{1, 20, 0}; + void TearDown() override { free_site_defs(); } }; /* Under the view it was taken in, the reservation is ours. */ TEST_F(XcomStaleReservation, reservation_under_the_current_index_is_fresh) { + install_new_view(); synode_no const reserved{1, 15, 1}; ASSERT_FALSE(reservation_is_stale(reserved)); } @@ -82,18 +75,21 @@ TEST_F(XcomStaleReservation, reservation_under_the_current_index_is_fresh) { /* After the renumbering, the same index belongs to another node. */ TEST_F(XcomStaleReservation, reservation_outliving_a_renumbering_is_stale) { synode_no const reserved{1, 25, 1}; + ASSERT_FALSE(reservation_is_stale(reserved)); + install_new_view(); ASSERT_TRUE(reservation_is_stale(reserved)); } /* A slot that carries the index this member holds now is usable. */ TEST_F(XcomStaleReservation, slot_matching_the_new_index_is_fresh) { + install_new_view(); synode_no const reserved{1, 25, 0}; ASSERT_FALSE(reservation_is_stale(reserved)); } /* Without a site there is nothing to compare against. */ TEST_F(XcomStaleReservation, unknown_site_is_not_reported_stale) { - synode_no const other_group{99, 25, 1}; + synode_no const other_group{99, 25, 0}; ASSERT_FALSE(reservation_is_stale(other_group)); } From 549d04d1fb78cc2a5ff42f6366fa86779c680142 Mon Sep 17 00:00:00 2001 From: Matias Sanchez Date: Mon, 7 Sep 2026 18:34:39 -0300 Subject: [PATCH 4/4] Bug#121063 Keep only the reservation check after wait_for_cache --- .../libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc index 94ebe57f9c7e..f6d77e38bcaf 100644 --- a/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc +++ b/plugin/group_replication/libmysqlgcs/src/bindings/xcom/xcom/xcom_base.cc @@ -2529,13 +2529,6 @@ static int proposer_task(task_arg arg) { brand_client_msg(ep->client_msg->p, ep->msgno); - /* Only a locally allocated synode carries our own node index; a remote or - global allocation carries the allocating leader's, by design. */ - if (ep->synode_allocation == synode_allocation_type::local && - reservation_is_stale(ep->msgno)) { - GOTO(retry_new); - } - for (;;) { /* Loop until the client message has been learned */ /* Get a Paxos instance to send the client message */ @@ -2546,8 +2539,8 @@ static int proposer_task(task_arg arg) { goto retry_new; } - /* Checked again after wait_for_cache: that call can suspend, and a view - change during the wait leaves the reservation stale. */ + /* wait_for_cache can suspend, and a view change during the wait leaves + the reservation stale. */ if (ep->synode_allocation == synode_allocation_type::local && reservation_is_stale(ep->msgno)) { GOTO(retry_new);