On Sat, Oct 16, 2021 at 9:13 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> While double-checking this stuff, I have noticed something that's
> wrong in the patch when a command that follows a
> CREATE_REPLICATION_SLOT query resets SnapBuildClearExportedSnapshot().
> Once the slot is created, the WAL sender is in a TRANS_INPROGRESS
> state, meaning that AbortCurrentTransaction() would call
> AbortTransaction(), hence calling ResetSnapBuildExportSnapshotState()
> and resetting SavedResourceOwnerDuringExport to NULL before we store a
> NULL into CurrentResourceOwner :)
Right, good catch!
> One solution would be as simple as saving
> SavedResourceOwnerDuringExport into a temporary variable before
> calling AbortCurrentTransaction(), and save it back into
> CurrentResourceOwner once we are done in
> SnapBuildClearExportedSnapshot() as we need to rely on
> AbortTransaction() to do the static state cleanup if an error happens
> until the command after the replslot creation command shows up.
Yeah, this idea looks fine to me. I have modified the patch. In
addition to that I have removed calling
ResetSnapBuildExportSnapshotState from the
SnapBuildClearExportedSnapshot because that is anyway being called
from the AbortTransaction.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 062a84dcd821fa5e41998c8714f8ec16befcf983 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Mon, 11 Oct 2021 20:38:58 +0530
Subject: [PATCH v2] Reset snapshot export state during abort
While exporting a snapshot we set a temporary states which get
cleaned up only while executing the next replication command.
But if the snapshot exporting transaction gets aborted then those
states are not cleaned. This patch fix this by cleaning it up
during AbortTransaction.
---
src/backend/access/transam/xact.c | 4 ++++
src/backend/replication/logical/snapbuild.c | 18 +++++++++++++++++-
src/include/replication/snapbuild.h | 1 +
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0..d3f7440 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -46,6 +46,7 @@
#include "replication/logical.h"
#include "replication/logicallauncher.h"
#include "replication/origin.h"
+#include "replication/snapbuild.h"
#include "replication/syncrep.h"
#include "replication/walsender.h"
#include "storage/condition_variable.h"
@@ -2698,6 +2699,9 @@ AbortTransaction(void)
/* Reset logical streaming state. */
ResetLogicalStreamingState();
+ /* Reset snapshot export state. */
+ ResetSnapBuildExportSnapshotState();
+
/* If in parallel mode, clean up workers and exit parallel mode. */
if (IsInParallelMode())
{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a533334..d889c22 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -682,6 +682,8 @@ SnapBuildGetOrBuildSnapshot(SnapBuild *builder, TransactionId xid)
void
SnapBuildClearExportedSnapshot(void)
{
+ ResourceOwner tmpResOwner;
+
/* nothing exported, that is the usual case */
if (!ExportInProgress)
return;
@@ -689,10 +691,24 @@ SnapBuildClearExportedSnapshot(void)
if (!IsTransactionState())
elog(ERROR, "clearing exported snapshot in wrong transaction state");
+ /*
+ * Abort transaction will reset the SavedResourceOwnerDuringExport so
+ * remember this in a local variable.
+ */
+ tmpResOwner = SavedResourceOwnerDuringExport;
+
/* make sure nothing could have ever happened */
AbortCurrentTransaction();
- CurrentResourceOwner = SavedResourceOwnerDuringExport;
+ CurrentResourceOwner = tmpResOwner;
+}
+
+/*
+ * Reset snapshot export state on the transaction abort.
+ */
+void
+ResetSnapBuildExportSnapshotState(void)
+{
SavedResourceOwnerDuringExport = NULL;
ExportInProgress = false;
}
diff --git a/src/include/replication/snapbuild.h b/src/include/replication/snapbuild.h
index de72124..6a1082b 100644
--- a/src/include/replication/snapbuild.h
+++ b/src/include/replication/snapbuild.h
@@ -70,6 +70,7 @@ extern void SnapBuildSnapDecRefcount(Snapshot snap);
extern Snapshot SnapBuildInitialSnapshot(SnapBuild *builder);
extern const char *SnapBuildExportSnapshot(SnapBuild *snapstate);
extern void SnapBuildClearExportedSnapshot(void);
+extern void ResetSnapBuildExportSnapshotState(void);
extern SnapBuildState SnapBuildCurrentState(SnapBuild *snapstate);
extern Snapshot SnapBuildGetOrBuildSnapshot(SnapBuild *builder,
--
1.8.3.1
