On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
>
> On 2021-Oct-01, Amit Kapila wrote:
> I think a straight standalone variable (probably a static boolean in
> xloginsert.c) might be less confusing.
I have written two patches, Approach1 is as you described using a
static boolean and Approach2 as a local variable to XLogAssembleRecord
as described by Amit, attached both of them for your reference.
IMHO, either of these approaches looks cleaner.
>
> ... so, reading the xact.c code again, TransactionState->assigned really
> means "whether the subXID-to-topXID association has been wal-logged",
> which is a completely different meaning from what the term 'assigned'
> means in all other comments in xact.c ... and I think the subroutine
> name MarkSubTransactionAssigned() is not a great choice either.
I have also renamed the variable and functions as per the actual usage.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 3d4fb94fdeef2ae46511291f144b6305c9eea9f7 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Sat, 2 Oct 2021 15:21:52 +0530
Subject: [PATCH] Refactor code for logging the top transaction id in WAL
---
src/backend/access/transam/xact.c | 22 +++++++++++-----------
src/backend/access/transam/xlog.c | 7 ++++++-
src/backend/access/transam/xloginsert.c | 23 ++++++++++-------------
src/include/access/xact.h | 4 ++--
src/include/access/xlog.h | 4 ++--
5 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0..893147649 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -204,7 +204,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;
@@ -237,7 +237,7 @@ typedef struct SerializedTransactionState
static TransactionStateData TopTransactionStateData = {
.state = TRANS_DEFAULT,
.blockState = TBLOCK_DEFAULT,
- .assigned = false,
+ .topXidLogged = false,
};
/*
@@ -5159,7 +5159,7 @@ PushTransaction(void)
GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
s->prevXactReadOnly = XactReadOnly;
s->parallelModeLevel = 0;
- s->assigned = false;
+ s->topXidLogged = false;
CurrentTransactionState = s;
@@ -6093,7 +6093,7 @@ xact_redo(XLogReaderState *record)
}
/*
- * IsSubTransactionAssignmentPending
+ * IsLogTopTransactionIdPending
*
* 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
@@ -6104,7 +6104,7 @@ xact_redo(XLogReaderState *record)
* record.
*/
bool
-IsSubTransactionAssignmentPending(void)
+IsLogTopTransactionIdPending(void)
{
/* wal_level has to be logical */
if (!XLogLogicalInfoActive())
@@ -6123,18 +6123,18 @@ IsSubTransactionAssignmentPending(void)
return false;
/* and it should not be already 'assigned' */
- return !CurrentTransactionState->assigned;
+ return !CurrentTransactionState->topXidLogged;
}
/*
- * MarkSubTransactionAssigned
+ * MarkTopTransactionIdLogged
*
- * Mark the subtransaction assignment as completed.
+ * Mark top transaction id is WAL logged for the current subtransaction.
*/
void
-MarkSubTransactionAssigned(void)
+MarkTopTransactionIdLogged(void)
{
- Assert(IsSubTransactionAssignmentPending());
+ Assert(IsLogTopTransactionIdPending());
- CurrentTransactionState->assigned = true;
+ CurrentTransactionState->topXidLogged = true;
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f8c714b..78232aa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1010,7 +1010,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;
@@ -1163,6 +1164,10 @@ XLogInsertRecord(XLogRecData *rdata,
MarkCurrentTransactionIdLoggedIfAny();
+ /* Mark top transaction id is logged (if needed) */
+ if (topxid_included)
+ MarkTopTransactionIdLogged();
+
END_CRIT_SECTION();
/*
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index b492c65..6b5925e 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 (IsLogTopTransactionIdPending())
{
TransactionId xid = GetTopTransactionIdIfAny();
- /* update the flag (later used by XLogResetInsertion) */
- XLogSetRecordFlags(XLOG_INCLUDE_XID);
+ /* Set update 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 134f686..e2dafe6 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 IsLogTopTransactionIdPending(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 5e2c94a..c0a5602 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);
--
1.8.3.1
From 55a6fe0256c314ed077bf8472d5f4b5dd87d2da8 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilipkumar@localhost.localdomain>
Date: Sat, 2 Oct 2021 16:08:57 +0530
Subject: [PATCH] Refactor code for logging the top transaction id in WAL
Instead of using the curinsert_flags, which is used for different
purpose, directly use the file level static variable. And also
change variable/function name which looks more appropriate.
---
src/backend/access/transam/xact.c | 22 +++++++++++-----------
src/backend/access/transam/xloginsert.c | 22 +++++++++++++++-------
src/include/access/xact.h | 4 ++--
src/include/access/xlog.h | 1 -
4 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0..893147649 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -204,7 +204,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;
@@ -237,7 +237,7 @@ typedef struct SerializedTransactionState
static TransactionStateData TopTransactionStateData = {
.state = TRANS_DEFAULT,
.blockState = TBLOCK_DEFAULT,
- .assigned = false,
+ .topXidLogged = false,
};
/*
@@ -5159,7 +5159,7 @@ PushTransaction(void)
GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
s->prevXactReadOnly = XactReadOnly;
s->parallelModeLevel = 0;
- s->assigned = false;
+ s->topXidLogged = false;
CurrentTransactionState = s;
@@ -6093,7 +6093,7 @@ xact_redo(XLogReaderState *record)
}
/*
- * IsSubTransactionAssignmentPending
+ * IsLogTopTransactionIdPending
*
* 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
@@ -6104,7 +6104,7 @@ xact_redo(XLogReaderState *record)
* record.
*/
bool
-IsSubTransactionAssignmentPending(void)
+IsLogTopTransactionIdPending(void)
{
/* wal_level has to be logical */
if (!XLogLogicalInfoActive())
@@ -6123,18 +6123,18 @@ IsSubTransactionAssignmentPending(void)
return false;
/* and it should not be already 'assigned' */
- return !CurrentTransactionState->assigned;
+ return !CurrentTransactionState->topXidLogged;
}
/*
- * MarkSubTransactionAssigned
+ * MarkTopTransactionIdLogged
*
- * Mark the subtransaction assignment as completed.
+ * Mark top transaction id is WAL logged for the current subtransaction.
*/
void
-MarkSubTransactionAssigned(void)
+MarkTopTransactionIdLogged(void)
{
- Assert(IsSubTransactionAssignmentPending());
+ Assert(IsLogTopTransactionIdPending());
- CurrentTransactionState->assigned = true;
+ CurrentTransactionState->topXidLogged = true;
}
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index b492c65..2e2344b 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -100,6 +100,15 @@ static uint8 curinsert_flags = 0;
static XLogRecData hdr_rdt;
static char *hdr_scratch = NULL;
+/*
+ * This variable is used for indicating that the top transaction xid is
+ * included in the current WAL record. This temporary flag is set by
+ * XLogRecordAssemble and used by XLogResetInsertion to mark the flag
+ * in CurrentTransactionState. For more detail about this, refer to comments
+ * atop IsLogTopTransactionIdPending().
+ */
+static bool topxid_included = false;
+
#define SizeOfXlogOrigin (sizeof(RepOriginId) + sizeof(char))
#define SizeOfXLogTransactionId (sizeof(TransactionId) + sizeof(char))
@@ -209,9 +218,9 @@ XLogResetInsertion(void)
{
int i;
- /* reset the subxact assignment flag (if needed) */
- if (curinsert_flags & XLOG_INCLUDE_XID)
- MarkSubTransactionAssigned();
+ /* Mark top transaction id is logged (if needed) */
+ if (topxid_included)
+ MarkTopTransactionIdLogged();
for (i = 0; i < max_registered_block_id; i++)
registered_buffers[i].in_use = false;
@@ -222,6 +231,7 @@ XLogResetInsertion(void)
mainrdata_last = (XLogRecData *) &mainrdata_head;
curinsert_flags = 0;
begininsert_called = false;
+ topxid_included = false;
}
/*
@@ -409,8 +419,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)
@@ -788,12 +796,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
}
/* followed by toplevel XID, if not already included in previous record */
- if (IsSubTransactionAssignmentPending())
+ if (IsLogTopTransactionIdPending())
{
TransactionId xid = GetTopTransactionIdIfAny();
/* update the flag (later used by XLogResetInsertion) */
- XLogSetRecordFlags(XLOG_INCLUDE_XID);
+ 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 134f686..e2dafe6 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 IsLogTopTransactionIdPending(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 5e2c94a..01a6a8b 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 */
--
1.8.3.1
