Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
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.

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));
Expand Down Expand Up @@ -2530,6 +2539,13 @@ static int proposer_task(task_arg arg) {
goto retry_new;
}

/* 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);
}

assert(ep->p);
if (ep->client_msg->p->force_delivery)
ep->p->force_delivery = ep->client_msg->p->force_delivery;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions unittest/gunit/libmysqlgcs/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
96 changes: 96 additions & 0 deletions unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/* 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 <xcom/xcom_profile.h>
#include <xcom_vp.h>
#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 {
/* The view in force when the synode is reserved: this member is node 1. */
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);
}

/* 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);
}

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));
}

/* 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, 0};
ASSERT_FALSE(reservation_is_stale(other_group));
}

} // namespace xcom_stale_reservation_unittest
Loading