From: Callahan, Drew <callaan@amazon.com>
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Cc: sawada.mshk@gmail.com <sawada.mshk@gmail.com>
Subject: Potential data loss due to race condition during logical replication slot creation
Date: Thu, 1 Feb 2024 22:28:44 +0000
Message-ID: <29273AF3-9684-4069-9257-D05090B03B99@amazon.com> (raw)
Hello,
We discovered a race condition during logical replication slot creation that can result in the changes for transactions running at the time of the slot creation to only be partially replicated. We found the cause was due to the slot transitioning from an inconsistent or partially consistent state to a fully consistent state when restoring a snapshot that had been persisted to disk by a different logical slot. We provide a simple reproduction of this issue below:
Session 1:
SELECT pg_create_logical_replication_slot('slot1', 'test_decoding');
CREATE TABLE test (a int);
BEGIN;
INSERT INTO test VALUES (1);
Session 2:
SELECT pg_create_logical_replication_slot('slot2', 'test_decoding');
<query hangs>
Session 3:
CHECKPOINT;
select pg_logical_slot_get_changes('slot1', NULL, NULL);
<should return nothing of interest>
Session 1:
INSERT INTO test VALUES (2);
COMMIT;
<Session 2 query no longer hangs and successfully creates the slot2>
Session 2:
select pg_logical_slot_get_changes('slot1', NULL, NULL);
select pg_logical_slot_get_changes('slot2', NULL, NULL);
<expected: no rows of the txn are returned for slot2>
<actual: The 2nd row of the txn is returned for slot2>
Newly created logical replication slots initialize their restart LSN to the current insert position within the WAL and also force a checkpoint to get the current state of the running transactions on the system. This create process will then wait for all of the transactions within that running xact record to complete before being able to transition to the next snapbuild state. During this time period, if another running xact record is written and then a different logical replication process decodes this running xact record, a globally accessible snapshot will be persisted to disk.
Once all of the transactions from the initial running xact have finished, the process performing the slot creation will become unblocked and will then consume the new running xact record. The process will see a valid snapshot, restore that snapshot from disk, and then transition immediately to the consistent state. The slot will then set the confirmed flush LSN of the slot to the start of the next record after that running xact.
We now have a logical replication slot that has a restart LSN after the changes of transactions that will commit after our confirmed flushed LSN in any case where the two running xact records are not fully disjointed. Once those transactions commit, we will then partially stream their changes.
The attached fix addresses the issue by providing the snapshot builder with enough context to understand whether it is building a snapshot for a slot for the first time or if this is a previously existing slot. This is similar to the “need_full_snapshot” parameter which is already set by the caller to control when and how the snapbuilder is allowed to become consistent.
With this context, the snapshot builder can always skip performing snapshot restore in order to become fully consistent. Since this issue only occurs when the the logical replication slot consumes a persisted snapshot to become fully consistent we can prevent the issue by disallowing this behavior.
Thanks,
Drew
Attachments:
[application/octet-stream] skip_snap_restore.patch (3.3K, ../29273AF3-9684-4069-9257-D05090B03B99@amazon.com/3-skip_snap_restore.patch)
download | inline diff:diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.cindex 41243d0187a..4e4e0ff42cb 100644--- a/src/backend/replication/logical/logical.c+++ b/src/backend/replication/logical/logical.c@@ -441,6 +441,8 @@ CreateInitDecodingContext(const char *plugin,
xl_routine, prepare_write, do_write,
update_progress);
+ SetSnapBuildCreateMode(ctx->context);+
/* call output plugin initialization callback */
old_context = MemoryContextSwitchTo(ctx->context);
if (ctx->callbacks.startup_cb != NULL)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.cindex 7a7aba33e16..372f1934677 100644--- a/src/backend/replication/logical/snapbuild.c+++ b/src/backend/replication/logical/snapbuild.c@@ -278,6 +278,15 @@ struct SnapBuild
static ResourceOwner SavedResourceOwnerDuringExport = NULL;
static bool ExportInProgress = false;
+/*+ * Used to mark the current snapbuild instance in a create mode.+ * This create mode is required to prevent the snapbuilder from+ * restoring a snapshot from disk to become consistent as this can+ * cause the restart and confirmed flush LSNs of the slot to become+ * initialized to unsafe values.+ */+static bool InCreate = false;+
/* ->committed and ->catchange manipulation */
static void SnapBuildPurgeOlderTxn(SnapBuild *builder);
@@ -443,6 +452,25 @@ SnapBuildSnapIncRefcount(Snapshot snap)
snap->active_count++;
}
+/* Memory context reset callback for clearing the create mode flag */+static void+SnapBuildResetCreateModeCallback(void *arg)+{+ InCreate = false;+}++void+SetSnapBuildCreateMode(MemoryContext context)+{+ MemoryContextCallback *mcallback;++ mcallback = palloc0(sizeof(MemoryContextCallback));+ mcallback->func = SnapBuildResetCreateModeCallback;+ MemoryContextRegisterResetCallback(context, mcallback);++ InCreate = true;+}+
/*
* Decrease refcount of a snapshot and free if the refcount reaches zero.
*
@@ -1391,8 +1419,9 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
return false;
}
- /* b) valid on disk state and not building full snapshot */+ /* b) valid on disk state, we are an exisiting slot, and not building full snapshot */
else if (!builder->building_full_snapshot &&
+ !InCreate &&
SnapBuildRestore(builder, lsn))
{
/* there won't be any state to cleanup */
diff --git a/src/include/replication/snapbuild.h b/src/include/replication/snapbuild.hindex f49b941b53e..3aff45dced1 100644--- a/src/include/replication/snapbuild.h+++ b/src/include/replication/snapbuild.h@@ -64,6 +64,7 @@ extern SnapBuild *AllocateSnapshotBuilder(struct ReorderBuffer *reorder,
bool need_full_snapshot,
XLogRecPtr two_phase_at);
extern void FreeSnapshotBuilder(SnapBuild *builder);
+extern void SetSnapBuildCreateMode(MemoryContext context);
extern void SnapBuildSnapDecRefcount(Snapshot snap);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: callaan@amazon.com, pgsql-bugs@lists.postgresql.org, sawada.mshk@gmail.com
Subject: Re: Potential data loss due to race condition during logical replication slot creation
In-Reply-To: <29273AF3-9684-4069-9257-D05090B03B99@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox