Skip to content

newsql_get_snapshot: fall back to stored snapshot on ha-retry#6079

Open
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:newsql-snapshot-ha-retry
Open

newsql_get_snapshot: fall back to stored snapshot on ha-retry#6079
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:newsql-snapshot-ha-retry

Conversation

@markhannum

Copy link
Copy Markdown
Contributor

Problem

On an HA retry (is_hasql_retry=1), the client replays the transaction
starting from the snapshot LSN that was established during the original
BEGIN. That LSN is stored in clnt->snapshot_file/offset by
is_snap_uid_retry at BEGIN time.

Statements after BEGIN (INSERT, SELECT, COMMIT) carry no
snapshot_info in their CDB2SqlQuery protobuf because only the BEGIN
message carries it from the client. newsql_get_snapshot was not
checking clnt->snapshot_file as a fallback, so it returned file=0
for all post-BEGIN statements on a retry path.

initialize_shadow_trans passes that file/offset into
trans_start_serializable. When file=0 and is_ha_retry=1,
the assertion at bdb_osqltrn.c:278 fires and kills the node.
Because the assertion is in common code reached by every thread, all
three nodes crash simultaneously.

Root cause

newsql_get_snapshot (plugins/newsql/newsql.c) only copied
snapshot_info from the current sqlquery. It had no fallback for
the retry case, unlike fill_snapinfo which already handled this by
checking clnt->snapshot_file when is_hasql_retry is set.

Fix

Add the same fallback to newsql_get_snapshot: when *file is still 0
after checking sqlquery->snapshot_info, and clnt->is_hasql_retry is
set, use clnt->snapshot_file/offset instead. This mirrors the
existing logic in fill_snapinfo and makes the two paths consistent.

Testing

Reproduced with a 3-node cluster under election / network-partition
stress (jepsen-style nemesis testing). The assert(file) crash at
bdb_osqltrn.c:278 no longer fires after this change.

Files changed

  • plugins/newsql/newsql.c — 7-line addition in newsql_get_snapshot

On an ha-retry, is_snap_uid_retry sets clnt->is_hasql_retry=1 and
stores the retry snapshot LSN in clnt->snapshot_file/offset during
BEGIN handling.  Statements after BEGIN (INSERT, SELECT, COMMIT) carry
no snapshot_info in their sqlquery because only the BEGIN carries it
from the client.

initialize_shadow_trans calls newsql_get_snapshot to obtain the
snapshot LSN.  Without a fallback, newsql_get_snapshot returns file=0
for these in-transaction queries, causing trans_start_serializable to
be called with file=0 and is_ha_retry=1 — which triggers the assertion
at bdb_osqltrn.c:278 and crashes the node.

fill_snapinfo already handles this correctly by checking
clnt->snapshot_file when is_hasql_retry is set.  Add the same fallback
to newsql_get_snapshot so that initialize_shadow_trans always uses the
correct retry snapshot.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Mark Hannum <mhannum@bloomberg.net>

@roborivers roborivers 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.

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants