From fcbb965e6ba7a444c9616bb0162913fda6e1cde3 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 18 Oct 2021 10:23:12 +0530
Subject: [PATCH v3] Replace XLOG_INCLUDE_XID flag with a more localized flag.

Commit 0bead9af484c introduced XLOG_INCLUDE_XID flag to indicate that the
WAL record contains subXID-to-topXID association. It uses that flag later
to mark in CurrentTransactionState that top-xid is logged so that we
should not try to log it again with the next WAL record in the current
subtransaction. However, we can use a localized variable to pass that
information.

In passing, change the related function and variable names to make them
consistent with what the code is actually doing.

This also reverts the changes made by commit ade24dab97 where we have
improved the documentation for XLOG_INCLUDE_XID flag.

Author: Dilip Kumar
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/E1mSoYz-0007Fh-D9@gemulon.postgresql.org
---
 src/backend/access/transam/xact.c       | 25 +++++++++++++------------
 src/backend/access/transam/xlog.c       | 13 ++++++++++++-
 src/backend/access/transam/xloginsert.c | 23 ++++++++++-------------
 src/include/access/xact.h               |  4 ++--
 src/include/access/xlog.h               |  4 ++--
 src/include/access/xlogrecord.h         |  5 ++---
 6 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d96881c0de..8f9884a69b 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -205,7 +205,7 @@ typedef struct TransactionStateData
 	bool		didLogXid;		/* has xid been included in WAL record? */
 	int			parallelModeLevel;	/* Enter/ExitParallelMode counter */
 	bool		chain;			/* start a new block after this one */
-	bool		assigned;		/* assigned to top-level XID */
+	bool		topXidLogged;	/* top-level XID is logged? */
 	struct TransactionStateData *parent;	/* back link to parent */
 } TransactionStateData;
 
@@ -238,7 +238,7 @@ typedef struct SerializedTransactionState
 static TransactionStateData TopTransactionStateData = {
 	.state = TRANS_DEFAULT,
 	.blockState = TBLOCK_DEFAULT,
-	.assigned = false,
+	.topXidLogged = false,
 };
 
 /*
@@ -5168,7 +5168,7 @@ PushTransaction(void)
 	GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
 	s->prevXactReadOnly = XactReadOnly;
 	s->parallelModeLevel = 0;
-	s->assigned = false;
+	s->topXidLogged = false;
 
 	CurrentTransactionState = s;
 
@@ -6102,7 +6102,7 @@ xact_redo(XLogReaderState *record)
 }
 
 /*
- * IsSubTransactionAssignmentPending
+ * IsTopTransactionIdLogged
  *
  * This is used to decide whether we need to WAL log the top-level XID for
  * operation in a subtransaction.  We require that for logical decoding, see
@@ -6113,7 +6113,7 @@ xact_redo(XLogReaderState *record)
  * record.
  */
 bool
-IsSubTransactionAssignmentPending(void)
+IsTopTransactionIdLogged(void)
 {
 	/* wal_level has to be logical */
 	if (!XLogLogicalInfoActive())
@@ -6131,19 +6131,20 @@ IsSubTransactionAssignmentPending(void)
 	if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny()))
 		return false;
 
-	/* and it should not be already 'assigned' */
-	return !CurrentTransactionState->assigned;
+	/* and it should not be already 'logged' */
+	return !CurrentTransactionState->topXidLogged;
 }
 
 /*
- * MarkSubTransactionAssigned
+ * MarkTopTransactionIdLogged
  *
- * Mark the subtransaction assignment as completed.
+ * Remember that the top transaction id for the current subtransaction is WAL
+ * logged now.
  */
 void
-MarkSubTransactionAssigned(void)
+MarkTopTransactionIdLogged(void)
 {
-	Assert(IsSubTransactionAssignmentPending());
+	Assert(IsTopTransactionIdLogged());
 
-	CurrentTransactionState->assigned = true;
+	CurrentTransactionState->topXidLogged = true;
 }
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 62862255fc..c899bd88d8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -998,6 +998,9 @@ static void WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt);
  * 'flags' gives more in-depth control on the record being inserted. See
  * XLogSetRecordFlags() for details.
  *
+ * 'topxid_included' tells whether the top-transaction id is logged along with
+ * current subtransaction. See XLogRecordAssemble().
+ *
  * The first XLogRecData in the chain must be for the record header, and its
  * data must be MAXALIGNed.  XLogInsertRecord fills in the xl_prev and
  * xl_crc fields in the header, the rest of the header must already be filled
@@ -1013,7 +1016,8 @@ XLogRecPtr
 XLogInsertRecord(XLogRecData *rdata,
 				 XLogRecPtr fpw_lsn,
 				 uint8 flags,
-				 int num_fpi)
+				 int num_fpi,
+				 bool topxid_included)
 {
 	XLogCtlInsert *Insert = &XLogCtl->Insert;
 	pg_crc32c	rdata_crc;
@@ -1166,6 +1170,13 @@ XLogInsertRecord(XLogRecData *rdata,
 
 	MarkCurrentTransactionIdLoggedIfAny();
 
+	/*
+	 * Mark top transaction id is logged (if needed) so that we should not try
+	 * to log it again with the next WAL record in the current subtransaction.
+	 */
+	if (topxid_included)
+		MarkTopTransactionIdLogged();
+
 	END_CRIT_SECTION();
 
 	/*
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index b492c656d7..8b76ca0286 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -123,7 +123,8 @@ static MemoryContext xloginsert_cxt;
 
 static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
 									   XLogRecPtr RedoRecPtr, bool doPageWrites,
-									   XLogRecPtr *fpw_lsn, int *num_fpi);
+									   XLogRecPtr *fpw_lsn, int *num_fpi,
+									   bool *topxid_included);
 static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
 									uint16 hole_length, char *dest, uint16 *dlen);
 
@@ -209,10 +210,6 @@ XLogResetInsertion(void)
 {
 	int			i;
 
-	/* reset the subxact assignment flag (if needed) */
-	if (curinsert_flags & XLOG_INCLUDE_XID)
-		MarkSubTransactionAssigned();
-
 	for (i = 0; i < max_registered_block_id; i++)
 		registered_buffers[i].in_use = false;
 
@@ -409,8 +406,6 @@ XLogRegisterBufData(uint8 block_id, char *data, int len)
  * - XLOG_MARK_UNIMPORTANT, to signal that the record is not important for
  *	 durability, which allows to avoid triggering WAL archiving and other
  *	 background activity.
- * - XLOG_INCLUDE_XID, a message-passing hack between XLogRecordAssemble
- *	 and XLogResetInsertion.
  */
 void
 XLogSetRecordFlags(uint8 flags)
@@ -465,6 +460,7 @@ XLogInsert(RmgrId rmid, uint8 info)
 	{
 		XLogRecPtr	RedoRecPtr;
 		bool		doPageWrites;
+		bool		topxid_included = false;
 		XLogRecPtr	fpw_lsn;
 		XLogRecData *rdt;
 		int			num_fpi = 0;
@@ -477,9 +473,10 @@ XLogInsert(RmgrId rmid, uint8 info)
 		GetFullPageWriteInfo(&RedoRecPtr, &doPageWrites);
 
 		rdt = XLogRecordAssemble(rmid, info, RedoRecPtr, doPageWrites,
-								 &fpw_lsn, &num_fpi);
+								 &fpw_lsn, &num_fpi, &topxid_included);
 
-		EndPos = XLogInsertRecord(rdt, fpw_lsn, curinsert_flags, num_fpi);
+		EndPos = XLogInsertRecord(rdt, fpw_lsn, curinsert_flags, num_fpi,
+								  topxid_included);
 	} while (EndPos == InvalidXLogRecPtr);
 
 	XLogResetInsertion();
@@ -502,7 +499,7 @@ XLogInsert(RmgrId rmid, uint8 info)
 static XLogRecData *
 XLogRecordAssemble(RmgrId rmid, uint8 info,
 				   XLogRecPtr RedoRecPtr, bool doPageWrites,
-				   XLogRecPtr *fpw_lsn, int *num_fpi)
+				   XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included)
 {
 	XLogRecData *rdt;
 	uint32		total_len = 0;
@@ -788,12 +785,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	}
 
 	/* followed by toplevel XID, if not already included in previous record */
-	if (IsSubTransactionAssignmentPending())
+	if (IsTopTransactionIdLogged())
 	{
 		TransactionId xid = GetTopTransactionIdIfAny();
 
-		/* update the flag (later used by XLogResetInsertion) */
-		XLogSetRecordFlags(XLOG_INCLUDE_XID);
+		/* Set the flag that the top xid is included in the WAL */
+		*topxid_included = true;
 
 		*(scratch++) = (char) XLR_BLOCK_ID_TOPLEVEL_XID;
 		memcpy(scratch, &xid, sizeof(TransactionId));
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 134f6862da..e39915a86e 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -433,8 +433,8 @@ extern void UnregisterXactCallback(XactCallback callback, void *arg);
 extern void RegisterSubXactCallback(SubXactCallback callback, void *arg);
 extern void UnregisterSubXactCallback(SubXactCallback callback, void *arg);
 
-extern bool IsSubTransactionAssignmentPending(void);
-extern void MarkSubTransactionAssigned(void);
+extern bool IsTopTransactionIdLogged(void);
+extern void MarkTopTransactionIdLogged(void);
 
 extern int	xactGetCommittedChildren(TransactionId **ptr);
 
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 5e2c94a05f..c0a560204b 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -212,7 +212,6 @@ extern bool XLOG_DEBUG;
  */
 #define XLOG_INCLUDE_ORIGIN		0x01	/* include the replication origin */
 #define XLOG_MARK_UNIMPORTANT	0x02	/* record not important for durability */
-#define XLOG_INCLUDE_XID		0x04	/* WAL-internal message-passing hack */
 
 
 /* Checkpoint statistics */
@@ -258,7 +257,8 @@ struct XLogRecData;
 extern XLogRecPtr XLogInsertRecord(struct XLogRecData *rdata,
 								   XLogRecPtr fpw_lsn,
 								   uint8 flags,
-								   int num_fpi);
+								   int num_fpi,
+								   bool topxid_included);
 extern void XLogFlush(XLogRecPtr RecPtr);
 extern bool XLogBackgroundFlush(void);
 extern bool XLogNeedsFlush(XLogRecPtr RecPtr);
diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index 8d1305eae8..e06ee92a5e 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -215,9 +215,8 @@ typedef struct XLogRecordDataHeaderLong
  * Block IDs used to distinguish different kinds of record fragments. Block
  * references are numbered from 0 to XLR_MAX_BLOCK_ID. A rmgr is free to use
  * any ID number in that range (although you should stick to small numbers,
- * because the WAL machinery is optimized for that case). A few ID
- * numbers are reserved to denote the "main" data portion of the record,
- * as well as replication-supporting transaction metadata.
+ * because the WAL machinery is optimized for that case). A couple of ID
+ * numbers are reserved to denote the "main" data portion of the record.
  *
  * The maximum is currently set at 32, quite arbitrarily. Most records only
  * need a handful of block references, but there are a few exceptions that
-- 
2.28.0.windows.1

