At Sun, 07 Mar 2021 20:09:33 -0500, Tom Lane <t...@sss.pgh.pa.us> wrote in
> Thomas Munro <thomas.mu...@gmail.com> writes:
> > Thanks! I'm afraid I wouldn't get around to it for a few weeks, so if
> > you have time, please do. (I'm not sure if it's strictly necessary to
> > log *this* xid, if a higher xid has already been logged, considering
> > that the goal is just to avoid getting confused about an xid that is
> > recycled after crash recovery, but coordinating that might be more
> > complicated; I don't know.)
>
> Yeah, ideally the patch wouldn't add any unnecessary WAL flush,
> if there's some cheap way to determine that our XID must already
> have been written out. But I'm not sure that it's worth adding
> any great amount of complexity to avoid that. For sure I would
> not advocate adding any new bookkeeping overhead in the mainline
> code paths to support it.
We need to *write* an additional record if the current transaction
haven't yet written one (EnsureTopTransactionIdLogged()). One
annoyance is the possibly most-common usage of calling
pg_current_xact_id() at the beginning of a transaction, which leads to
an additional 8 byte-long log of XLOG_XACT_ASSIGNMENT. We could also
avoid that by detecting any larger xid is already flushed out.
I haven't find a simple and clean way to tracking the maximum
flushed-out XID. The new cooperation between xlog.c and xact.c
related to XID and LSN happen on shared variable makes things
complex...
So the attached doesn't contain the max-flushed-xid tracking feature.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 2d84b2adcf1f4612948034817626ae4bfc0f57da Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Mon, 8 Mar 2021 15:32:30 +0900
Subject: [PATCH v1 1/2] Run 011_crash_recovery.pl with wal_level=minimal
The test doesn't need that feature and pg_current_xact_id() is better
exercised by turning off the feature.
---
src/test/recovery/t/011_crash_recovery.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 10cd98f70a..690655dda2 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -11,7 +11,7 @@ use Config;
plan tests => 3;
my $node = get_new_node('primary');
-$node->init(allows_streaming => 1);
+$node->init();
$node->start;
my ($stdin, $stdout, $stderr) = ('', '', '');
--
2.27.0
>From a3eee3b2bb28dc452d9a17d058d85c129a086af8 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Mon, 8 Mar 2021 15:43:01 +0900
Subject: [PATCH v1 2/2] Make sure published XIDs are persistent
pg_xact_status() premises that XIDs obtained by
pg_current_xact_id(_if_assigned)() are persistent beyond a crash. But
XIDs are not guaranteed to go beyond WAL buffers before commit and
thus XIDs may vanish if server crashes before commit. This patch
guarantees the XID shown by the functions to be flushed out to disk.
---
src/backend/access/transam/xact.c | 55 +++++++++++++++++++++++++------
src/backend/access/transam/xlog.c | 2 +-
src/backend/utils/adt/xid8funcs.c | 12 ++++++-
src/include/access/xact.h | 3 +-
4 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4e6a3df6b8..85b1c21ee0 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -201,7 +201,7 @@ typedef struct TransactionStateData
int prevSecContext; /* previous SecurityRestrictionContext */
bool prevXactReadOnly; /* entry-time xact r/o state */
bool startedInRecovery; /* did we start in recovery? */
- bool didLogXid; /* has xid been included in WAL record? */
+ XLogRecPtr minLSN; /* LSN needed to reach to record the xid */
int parallelModeLevel; /* Enter/ExitParallelMode counter */
bool chain; /* start a new block after this one */
bool assigned; /* assigned to top-level XID */
@@ -520,14 +520,46 @@ GetCurrentFullTransactionIdIfAny(void)
* MarkCurrentTransactionIdLoggedIfAny
*
* Remember that the current xid - if it is assigned - now has been wal logged.
+ *
+ * upto is the LSN up to which we need to flush WAL to ensure the current xid
+ * to be persistent. See EnsureCurrentTransactionIdLogged().
*/
void
-MarkCurrentTransactionIdLoggedIfAny(void)
+MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto)
{
- if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId))
- CurrentTransactionState->didLogXid = true;
+ if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId) &&
+ XLogRecPtrIsInvalid(CurrentTransactionState->minLSN))
+ CurrentTransactionState->minLSN = upto;
}
+/*
+ * EnsureCurrentTransactionIdLogged
+ *
+ * Make sure that the current top XID is WAL-logged.
+ */
+void
+EnsureTopTransactionIdLogged(void)
+{
+ /*
+ * We need at least one WAL record for the current top transaction to be
+ * flushed out. Write one if we don't have one yet.
+ */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ {
+ xl_xact_assignment xlrec;
+
+ xlrec.xtop = XidFromFullTransactionId(XactTopFullTransactionId);
+ Assert(TransactionIdIsValid(xlrec.xtop));
+ xlrec.nsubxacts = 0;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, MinSizeOfXactAssignment);
+ TopTransactionStateData.minLSN =
+ XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ }
+
+ XLogFlush(TopTransactionStateData.minLSN);
+}
/*
* GetStableLatestTransactionId
@@ -616,14 +648,14 @@ AssignTransactionId(TransactionState s)
* When wal_level=logical, guarantee that a subtransaction's xid can only
* be seen in the WAL stream if its toplevel xid has been logged before.
* If necessary we log an xact_assignment record with fewer than
- * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't set
+ * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if minLSN isn't set
* for a transaction even though it appears in a WAL record, we just might
* superfluously log something. That can happen when an xid is included
* somewhere inside a wal record, but not in XLogRecord->xl_xid, like in
* xl_standby_locks.
*/
if (isSubXact && XLogLogicalInfoActive() &&
- !TopTransactionStateData.didLogXid)
+ XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
log_unknown_top = true;
/*
@@ -693,6 +725,7 @@ AssignTransactionId(TransactionState s)
log_unknown_top)
{
xl_xact_assignment xlrec;
+ XLogRecPtr endptr;
/*
* xtop is always set by now because we recurse up transaction
@@ -707,11 +740,13 @@ AssignTransactionId(TransactionState s)
XLogRegisterData((char *) unreportedXids,
nUnreportedXids * sizeof(TransactionId));
- (void) XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ endptr = XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
nUnreportedXids = 0;
- /* mark top, not current xact as having been logged */
- TopTransactionStateData.didLogXid = true;
+
+ /* set minLSN of top, not of current xact if not yet */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ TopTransactionStateData.minLSN = endptr;
}
}
}
@@ -1996,7 +2031,7 @@ StartTransaction(void)
* initialize reported xid accounting
*/
nUnreportedXids = 0;
- s->didLogXid = false;
+ s->minLSN = InvalidXLogRecPtr;
/*
* must initialize resource-management stuff first
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 377afb8732..1a503c1447 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1159,7 +1159,7 @@ XLogInsertRecord(XLogRecData *rdata,
*/
WALInsertLockRelease();
- MarkCurrentTransactionIdLoggedIfAny();
+ MarkCurrentTransactionIdLoggedIfAny(EndPos);
END_CRIT_SECTION();
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index cc2b4ac797..992482f8c8 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -357,6 +357,8 @@ bad_format:
Datum
pg_current_xact_id(PG_FUNCTION_ARGS)
{
+ FullTransactionId xid;
+
/*
* Must prevent during recovery because if an xid is not assigned we try
* to assign one, which would fail. Programs already rely on this function
@@ -365,7 +367,12 @@ pg_current_xact_id(PG_FUNCTION_ARGS)
*/
PreventCommandDuringRecovery("pg_current_xact_id()");
- PG_RETURN_FULLTRANSACTIONID(GetTopFullTransactionId());
+ xid = GetTopFullTransactionId();
+
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
+ PG_RETURN_FULLTRANSACTIONID(xid);
}
/*
@@ -380,6 +387,9 @@ pg_current_xact_id_if_assigned(PG_FUNCTION_ARGS)
if (!FullTransactionIdIsValid(topfxid))
PG_RETURN_NULL();
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
PG_RETURN_FULLTRANSACTIONID(topfxid);
}
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index f49a57b35e..536a5e653b 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -386,7 +386,8 @@ extern FullTransactionId GetTopFullTransactionId(void);
extern FullTransactionId GetTopFullTransactionIdIfAny(void);
extern FullTransactionId GetCurrentFullTransactionId(void);
extern FullTransactionId GetCurrentFullTransactionIdIfAny(void);
-extern void MarkCurrentTransactionIdLoggedIfAny(void);
+extern void MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto);
+extern void EnsureTopTransactionIdLogged(void);
extern bool SubTransactionIsActive(SubTransactionId subxid);
extern CommandId GetCurrentCommandId(bool used);
extern void SetParallelStartTimestamps(TimestampTz xact_ts, TimestampTz stmt_ts);
--
2.27.0
