public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: Andres Freund <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: shveta malik <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Jan Wieck <[email protected]>
Subject: RE: Commit Timestamp and LSN Inversion issue
Date: Fri, 8 Nov 2024 09:08:55 +0000
Message-ID: <OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <gkbrjxwyjbqckjbrkep3uy2mvrcq2mrfzprfmegbeiqxidtmbu@3cgs5iocjykk>
References: <CAJpy0uBxEJnabEp3JS=n9X19Vx2ZK3k5AR7N0h-cSMtOwYV3fA@mail.gmail.com>
<CAJ7c6TNZY3idBDoGOaoS_fF6vJuBMjnDhZC4i5V0J58s2SJ3AA@mail.gmail.com>
<CAA4eK1LOmtMpmJAxsmN0yXJb-Q9vi6KThDYXdpVGL_WZW5ZgvQ@mail.gmail.com>
<CAJ7c6TOcQPcydC7HW=xPEDN6_Ec7iSTP9j=1VTAeG=haguOQHw@mail.gmail.com>
<CAA4eK1KhAxe6=VTxa6xEbc22_VVBLsVuvRH8Wxws8i50gH=eqw@mail.gmail.com>
<[email protected]>
<gkbrjxwyjbqckjbrkep3uy2mvrcq2mrfzprfmegbeiqxidtmbu@3cgs5iocjykk>
On Friday, November 8, 2024 2:20 AM Andres Freund <[email protected]> wrote:
Hi,
> On 2024-11-05 08:58:36 -0500, Jan Wieck wrote:
> > The attached solution is minimally invasive because it doesn't move
> > the timestamp generation (clock_gettime() call) into the critical
> > section of
> > ReserveXLogInsertLocation() that is protected by a spinlock. Instead
> > it keeps track of the last commit-ts written to WAL in shared memory
> > and simply bumps that by one microsecond if the next one is below or
> > equal. There is one extra condition in that code section plus a
> > function call by pointer for every WAL record. In the unlikely case of
> > encountering a stalled or backwards running commit-ts, the expensive
> > part of recalculating the CRC of the altered commit WAL-record is done
> > later, after releasing the spinlock. I have not been able to measure
> > any performance impact on a machine with 2x Xeon-Silver (32 HT cores).
>
> I think it's *completely* unacceptable to call a hook inside
> ReserveXLogInsertLocation, with the spinlock held, no less. That's the most
> contended section of code in postgres.
I understand your concern and appreciate the feedback. I've made some
adjustments to the patch by directly placing the code to adjust the commit
timestamp within the spinlock, aiming to keep it as efficient as possible. The
changes have resulted in just a few extra lines. Would this approach be
acceptable to you ?
Additionally, we're also doing performance tests for it and will share the
results once they're available.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v2-0001-POC-Make-commit-timestamp-monotonic-increase.patch (9.7K, ../OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com/2-v2-0001-POC-Make-commit-timestamp-monotonic-increase.patch)
download | inline diff:
From 1edaedf40d85baad7c6e1d12adae4e5ccc72077e Mon Sep 17 00:00:00 2001
From: Hou Zhijie <[email protected]>
Date: Fri, 8 Nov 2024 15:10:48 +0800
Subject: [PATCH v4] Make commit timestamp monotonic increase
---
src/backend/access/transam/xact.c | 18 ++++-
src/backend/access/transam/xlog.c | 129 ++++++++++++++++++++++++++++--
src/include/access/xact.h | 2 +
3 files changed, 140 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b7ebcc2a55..0caaeae913 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -278,7 +278,7 @@ static bool currentCommandIdUsed;
*/
static TimestampTz xactStartTimestamp;
static TimestampTz stmtStartTimestamp;
-static TimestampTz xactStopTimestamp;
+TimestampTz xactStopTimestamp;
/*
* GID to be used for preparing the current transaction. This is also
@@ -325,6 +325,7 @@ typedef struct SubXactCallbackItem
static SubXactCallbackItem *SubXact_callbacks = NULL;
+xl_xact_commit *xlcommitrec = NULL;
/* local function prototypes */
static void AssignTransactionId(TransactionState s);
@@ -2214,6 +2215,9 @@ StartTransaction(void)
if (TransactionTimeout > 0)
enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+ /* Reset xlcommitrec */
+ xlcommitrec = NULL;
+
ShowTransactionState("StartTransaction");
}
@@ -5831,6 +5835,7 @@ XactLogCommitRecord(TimestampTz commit_time,
xl_xact_twophase xl_twophase;
xl_xact_origin xl_origin;
uint8 info;
+ XLogRecPtr result;
Assert(CritSectionCount > 0);
@@ -5974,7 +5979,16 @@ XactLogCommitRecord(TimestampTz commit_time,
/* we allow filtering by xacts */
XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
- return XLogInsert(RM_XACT_ID, info);
+ /*
+ * Save the commit xlrec so that we can modify the xactStopTimestamp and
+ * the xact_time of the xlrec while holding the lock that determines the
+ * commit-LSN to ensure the commit timestamps are monotonically increasing.
+ */
+ xlcommitrec = &xlrec;
+ result = XLogInsert(RM_XACT_ID, info);
+ xlcommitrec = NULL;
+
+ return result;
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6f58412bca..8efad83df3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -155,6 +155,12 @@ int wal_segment_size = DEFAULT_XLOG_SEG_SIZE;
*/
int CheckPointSegments;
+/*
+ * Whether the xact_time of xlrec is modified to ensure the commit timestamps
+ * are monotonically increasing.
+ */
+static bool XlogRecordModified = false;
+
/* Estimated distance between checkpoints, in bytes */
static double CheckPointDistanceEstimate = 0;
static double PrevCheckPointDistance = 0;
@@ -551,6 +557,14 @@ typedef struct XLogCtlData
XLogRecPtr lastFpwDisableRecPtr;
slock_t info_lck; /* locks shared variables shown above */
+
+ /*
+ * This is our shared, logical clock that we use to force
+ * commit timestamps to be monotonically increasing in
+ * commit-LSN order. This is protected by the Wal-insert
+ * spinlock.
+ */
+ TimestampTz lastXactStopTime;
} XLogCtlData;
/*
@@ -700,6 +714,7 @@ static void CopyXLogRecordToWAL(int write_len, bool isLogSwitch,
XLogRecData *rdata,
XLogRecPtr StartPos, XLogRecPtr EndPos,
TimeLineID tli);
+static void XLogRecordCorrectCRC(XLogRecData *rdata);
static void ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos,
XLogRecPtr *EndPos, XLogRecPtr *PrevPtr);
static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
@@ -778,6 +793,12 @@ XLogInsertRecord(XLogRecData *rdata,
if (!XLogInsertAllowed())
elog(ERROR, "cannot make new WAL entries during recovery");
+ /*
+ * Make sure the flag telling that ReserveXLog...() modified the record is
+ * false at this point.
+ */
+ XlogRecordModified = false;
+
/*
* Given that we're not in recovery, InsertTimeLineID is set and can't
* change, so we can read it without a lock.
@@ -906,6 +927,15 @@ XLogInsertRecord(XLogRecData *rdata,
if (inserted)
{
+ /*
+ * If modified the XLog Record, recalculate the CRC.
+ */
+ if (XlogRecordModified)
+ {
+ XLogRecordCorrectCRC(rdata);
+ XlogRecordModified = false;
+ }
+
/*
* Now that xl_prev has been filled in, calculate CRC of the record
* header.
@@ -1086,6 +1116,25 @@ XLogInsertRecord(XLogRecData *rdata,
return EndPos;
}
+/*
+ * Function to recalculate the WAL Record's CRC in case it was altered to
+ * ensure a monotonically increasing commit timestamp in LSN order.
+ */
+static void
+XLogRecordCorrectCRC(XLogRecData *rdata)
+{
+ XLogRecData *rdt;
+ XLogRecord *rechdr = (XLogRecord *) rdata->data;
+ pg_crc32c rdata_crc;
+
+ INIT_CRC32C(rdata_crc);
+ COMP_CRC32C(rdata_crc, rdata->data + SizeOfXLogRecord, rdata->len - SizeOfXLogRecord);
+ for (rdt = rdata->next; rdt != NULL; rdt = rdt->next)
+ COMP_CRC32C(rdata_crc, rdt->data, rdt->len);
+
+ rechdr->xl_crc = rdata_crc;
+}
+
/*
* Reserves the right amount of space for a record of given size from the WAL.
* *StartPos is set to the beginning of the reserved section, *EndPos to
@@ -1112,6 +1161,10 @@ ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
uint64 startbytepos;
uint64 endbytepos;
uint64 prevbytepos;
+ TimestampTz orgxacttime = 0;
+
+ if (xlcommitrec)
+ orgxacttime = xlcommitrec->xact_time;
size = MAXALIGN(size);
@@ -1127,21 +1180,56 @@ ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
* positions (XLogRecPtrs) can be done outside the locked region, and
* because the usable byte position doesn't include any headers, reserving
* X bytes from WAL is almost as simple as "CurrBytePos += X".
+ *
+ * Note that for the commit record, we need to ensure that the commit
+ * timestamp is monotonically increased in the commit-LSN order to avoid
+ * inconsistency between the two.
*/
- SpinLockAcquire(&Insert->insertpos_lck);
+ if (xlcommitrec)
+ {
+ SpinLockAcquire(&Insert->insertpos_lck);
- startbytepos = Insert->CurrBytePos;
- endbytepos = startbytepos + size;
- prevbytepos = Insert->PrevBytePos;
- Insert->CurrBytePos = endbytepos;
- Insert->PrevBytePos = startbytepos;
+ /*
+ * This is a local transaction. Make sure that the xact_time higher
+ * than any timestamp we have seen thus far.
+ */
+ if (unlikely(XLogCtl->lastXactStopTime >= xlcommitrec->xact_time))
+ {
+ XLogCtl->lastXactStopTime++;
+ xlcommitrec->xact_time = XLogCtl->lastXactStopTime;
+ xactStopTimestamp = XLogCtl->lastXactStopTime;
+ }
+ else
+ XLogCtl->lastXactStopTime = xlcommitrec->xact_time;
- SpinLockRelease(&Insert->insertpos_lck);
+ startbytepos = Insert->CurrBytePos;
+ endbytepos = startbytepos + size;
+ prevbytepos = Insert->PrevBytePos;
+ Insert->CurrBytePos = endbytepos;
+ Insert->PrevBytePos = startbytepos;
+
+ SpinLockRelease(&Insert->insertpos_lck);
+ }
+ else
+ {
+ SpinLockAcquire(&Insert->insertpos_lck);
+
+ startbytepos = Insert->CurrBytePos;
+ endbytepos = startbytepos + size;
+ prevbytepos = Insert->PrevBytePos;
+ Insert->CurrBytePos = endbytepos;
+ Insert->PrevBytePos = startbytepos;
+
+ SpinLockRelease(&Insert->insertpos_lck);
+ }
*StartPos = XLogBytePosToRecPtr(startbytepos);
*EndPos = XLogBytePosToEndRecPtr(endbytepos);
*PrevPtr = XLogBytePosToRecPtr(prevbytepos);
+ if (xlcommitrec && orgxacttime != xlcommitrec->xact_time)
+ XlogRecordModified = true;
+
/*
* Check that the conversions between "usable byte positions" and
* XLogRecPtrs work consistently in both directions.
@@ -1170,12 +1258,20 @@ ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
uint32 size = MAXALIGN(SizeOfXLogRecord);
XLogRecPtr ptr;
uint32 segleft;
+ TimestampTz orgxacttime = 0;
+
+ if (xlcommitrec)
+ orgxacttime = xlcommitrec->xact_time;
/*
* These calculations are a bit heavy-weight to be done while holding a
* spinlock, but since we're holding all the WAL insertion locks, there
* are no other inserters competing for it. GetXLogInsertRecPtr() does
* compete for it, but that's not called very frequently.
+ *
+ * Note that for the commit record, we need to ensure that the commit
+ * timestamp is monotonically increased in the commit-LSN order to avoid
+ * inconsistency between the two.
*/
SpinLockAcquire(&Insert->insertpos_lck);
@@ -1189,6 +1285,22 @@ ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
return false;
}
+ if (xlcommitrec)
+ {
+ /*
+ * This is a local transaction. Make sure that the xact_time higher
+ * than any timestamp we have seen thus far.
+ */
+ if (unlikely(XLogCtl->lastXactStopTime >= xlcommitrec->xact_time))
+ {
+ XLogCtl->lastXactStopTime++;
+ xlcommitrec->xact_time = XLogCtl->lastXactStopTime;
+ xactStopTimestamp = XLogCtl->lastXactStopTime;
+ }
+ else
+ XLogCtl->lastXactStopTime = xlcommitrec->xact_time;
+ }
+
endbytepos = startbytepos + size;
prevbytepos = Insert->PrevBytePos;
@@ -1209,6 +1321,9 @@ ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
*PrevPtr = XLogBytePosToRecPtr(prevbytepos);
+ if (xlcommitrec && orgxacttime != xlcommitrec->xact_time)
+ XlogRecordModified = true;
+
Assert(XLogSegmentOffset(*EndPos, wal_segment_size) == 0);
Assert(XLogRecPtrToBytePos(*EndPos) == endbytepos);
Assert(XLogRecPtrToBytePos(*StartPos) == startbytepos);
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index fb64d7413a..43e4b5aa18 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -431,6 +431,8 @@ typedef struct xl_xact_parsed_abort
TimestampTz origin_timestamp;
} xl_xact_parsed_abort;
+extern PGDLLIMPORT xl_xact_commit *xlcommitrec;
+extern PGDLLIMPORT TimestampTz xactStopTimestamp;
/* ----------------
* extern definitions
--
2.30.0.windows.2
view thread (33+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: Commit Timestamp and LSN Inversion issue
In-Reply-To: <OS0PR01MB57162A227EC357482FEB4470945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox