public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: Andres Freund <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: New WAL record to detect the checkpoint redo location
Date: Wed, 20 Sep 2023 16:20:08 -0400
Message-ID: <CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoYy-Vc6G9QKcAKNksCa29cv__czr+N9X_QCxEfQVpp_8w@mail.gmail.com>
References: <CAFiTN-s-K=mVA=HPr_VoU-5bvyLQpNeuzjq1ebPJMEfCJZKFsg@mail.gmail.com>
<[email protected]>
<CA+TgmoYy-Vc6G9QKcAKNksCa29cv__czr+N9X_QCxEfQVpp_8w@mail.gmail.com>
On Mon, Sep 18, 2023 at 2:57 PM Robert Haas <[email protected]> wrote:
> I've been brainstorming about this today, trying to figure out some
> ideas to make it work.
Here are some patches.
0001 refactors XLogInsertRecord to unify a couple of separate tests of
isLogSwitch, hopefully making it cleaner and cheaper to add more
special cases.
0002 is a very minimal patch to add XLOG_CHECKPOINT_REDO without using
it for anything.
0003 modifies CreateCheckPoint() to insert an XLOG_CHECKPOINT_REDO
record for any non-shutdown checkpoint, and modifies
XLogInsertRecord() to treat that as a new special case, wherein after
inserting the record the redo pointer is reset while still holding the
WAL insertion locks.
I've tested this to the extent of running the regression tests, and I
also did one (1) manual test where it looked like the right thing was
happening, but that's it, so this might be buggy or perform like
garbage for all I know. But my hope is that it isn't buggy and
performs adequately. If there's any chance of getting some comments on
the basic design choices before I spend time testing and polishing it,
that would be very helpful.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v6-0001-Unify-two-isLogSwitch-tests-in-XLogInsertRecord.patch (4.7K, ../CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.com/2-v6-0001-Unify-two-isLogSwitch-tests-in-XLogInsertRecord.patch)
download | inline diff:
From 474f96734c8c86a3d01979b6a3f8c293eee4e927 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Wed, 20 Sep 2023 13:48:19 -0400
Subject: [PATCH v6 1/3] Unify two isLogSwitch tests in XLogInsertRecord.
An upcoming patch wants to introduce an additional special case in this
function. To keep that as cheap as possible, minimize the amount of branching
that we do based on whether this is an XLOG_SWITCH record.
---
src/backend/access/transam/xlog.c | 97 ++++++++++++++++++-------------
1 file changed, 56 insertions(+), 41 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f6f8adc72a..348cbebe3a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -792,57 +792,72 @@ XLogInsertRecord(XLogRecData *rdata,
*----------
*/
START_CRIT_SECTION();
+
if (isLogSwitch)
+ {
+ /*
+ * In order to insert an XLOG_SWITCH record, we need to hold all of
+ * the WAL insertion locks, not just one, so that no one else can
+ * begin inserting a record until we've figured out how much space
+ * remains in the current WAL segment and claimed all of it.
+ *
+ * Nonetheless, this case is simpler than the normal cases handled
+ * below, which must check for changes in doPageWrites and RedoRecPtr.
+ * Those checks are only needed for records that can contain
+ * full-pages images, and an XLOG_SWITCH record never does.
+ */
+ Assert(fpw_lsn == InvalidXLogRecPtr);
WALInsertLockAcquireExclusive();
+ inserted = ReserveXLogSwitch(&StartPos, &EndPos, &rechdr->xl_prev);
+ }
else
- WALInsertLockAcquire();
-
- /*
- * Check to see if my copy of RedoRecPtr is out of date. If so, may have
- * to go back and have the caller recompute everything. This can only
- * happen just after a checkpoint, so it's better to be slow in this case
- * and fast otherwise.
- *
- * Also check to see if fullPageWrites was just turned on or there's a
- * running backup (which forces full-page writes); if we weren't already
- * doing full-page writes then go back and recompute.
- *
- * If we aren't doing full-page writes then RedoRecPtr doesn't actually
- * affect the contents of the XLOG record, so we'll update our local copy
- * but not force a recomputation. (If doPageWrites was just turned off,
- * we could recompute the record without full pages, but we choose not to
- * bother.)
- */
- if (RedoRecPtr != Insert->RedoRecPtr)
{
- Assert(RedoRecPtr < Insert->RedoRecPtr);
- RedoRecPtr = Insert->RedoRecPtr;
- }
- doPageWrites = (Insert->fullPageWrites || Insert->runningBackups > 0);
+ WALInsertLockAcquire();
- if (doPageWrites &&
- (!prevDoPageWrites ||
- (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr)))
- {
/*
- * Oops, some buffer now needs to be backed up that the caller didn't
- * back up. Start over.
+ * Check to see if my copy of RedoRecPtr is out of date. If so, may
+ * have to go back and have the caller recompute everything. This can
+ * only happen just after a checkpoint, so it's better to be slow in
+ * this case and fast otherwise.
+ *
+ * Also check to see if fullPageWrites was just turned on or there's a
+ * running backup (which forces full-page writes); if we weren't
+ * already doing full-page writes then go back and recompute.
+ *
+ * If we aren't doing full-page writes then RedoRecPtr doesn't
+ * actually affect the contents of the XLOG record, so we'll update
+ * our local copy but not force a recomputation. (If doPageWrites was
+ * just turned off, we could recompute the record without full pages,
+ * but we choose not to bother.)
*/
- WALInsertLockRelease();
- END_CRIT_SECTION();
- return InvalidXLogRecPtr;
- }
+ if (RedoRecPtr != Insert->RedoRecPtr)
+ {
+ Assert(RedoRecPtr < Insert->RedoRecPtr);
+ RedoRecPtr = Insert->RedoRecPtr;
+ }
+ doPageWrites = (Insert->fullPageWrites || Insert->runningBackups > 0);
- /*
- * Reserve space for the record in the WAL. This also sets the xl_prev
- * pointer.
- */
- if (isLogSwitch)
- inserted = ReserveXLogSwitch(&StartPos, &EndPos, &rechdr->xl_prev);
- else
- {
+ if (doPageWrites &&
+ (!prevDoPageWrites ||
+ (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr)))
+ {
+ /*
+ * Oops, some buffer now needs to be backed up that the caller
+ * didn't back up. Start over.
+ */
+ WALInsertLockRelease();
+ END_CRIT_SECTION();
+ return InvalidXLogRecPtr;
+ }
+
+ /*
+ * Reserve space for the record in the WAL. This also sets the xl_prev
+ * pointer.
+ */
ReserveXLogInsertLocation(rechdr->xl_tot_len, &StartPos, &EndPos,
&rechdr->xl_prev);
+
+ /* Normal records are always inserted. */
inserted = true;
}
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v6-0002-Minimally-add-XLOG_CHECKPOINT_REDO.patch (2.5K, ../CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.com/3-v6-0002-Minimally-add-XLOG_CHECKPOINT_REDO.patch)
download | inline diff:
From af6b150041ff399fda83cf9c43da880e8d850921 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Wed, 20 Sep 2023 13:55:28 -0400
Subject: [PATCH v6 2/3] Minimally add XLOG_CHECKPOINT_REDO.
Not to be committed separately.
Based on a previous patch by Dilip Kumar.
---
src/backend/access/rmgrdesc/xlogdesc.c | 7 +++++++
src/backend/access/transam/xlog.c | 4 ++++
src/backend/replication/logical/decode.c | 1 +
src/include/catalog/pg_control.h | 1 +
4 files changed, 13 insertions(+)
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index f390c177e4..37f59bda7e 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -148,6 +148,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
LSN_FORMAT_ARGS(xlrec.overwritten_lsn),
timestamptz_to_str(xlrec.overwrite_time));
}
+ else if (info == XLOG_CHECKPOINT_REDO)
+ {
+ /* No details to write out */
+ }
}
const char *
@@ -196,6 +200,9 @@ xlog_identify(uint8 info)
case XLOG_FPI_FOR_HINT:
id = "FPI_FOR_HINT";
break;
+ case XLOG_CHECKPOINT_REDO:
+ id = "CHECKPOINT_REDO";
+ break;
}
return id;
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 348cbebe3a..026e2fc9da 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8102,6 +8102,10 @@ xlog_redo(XLogReaderState *record)
/* Keep track of full_page_writes */
lastFullPageWrites = fpw;
}
+ else if (info == XLOG_CHECKPOINT_REDO)
+ {
+ /* nothing to do here, just for informational purposes */
+ }
}
/*
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 730061c9da..24b712aa66 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -190,6 +190,7 @@ xlog_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
case XLOG_FPI_FOR_HINT:
case XLOG_FPI:
case XLOG_OVERWRITE_CONTRECORD:
+ case XLOG_CHECKPOINT_REDO:
break;
default:
elog(ERROR, "unexpected RM_XLOG_ID record type: %u", info);
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index dc953977c5..1136613259 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -78,6 +78,7 @@ typedef struct CheckPoint
#define XLOG_FPI 0xB0
/* 0xC0 is used in Postgres 9.5-11 */
#define XLOG_OVERWRITE_CONTRECORD 0xD0
+#define XLOG_CHECKPOINT_REDO 0xE0
/*
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v6-0003-WIP-Insert-XLOG_CHECKPOINT_REDO-at-the-redo-point.patch (7.9K, ../CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.com/4-v6-0003-WIP-Insert-XLOG_CHECKPOINT_REDO-at-the-redo-point.patch)
download | inline diff:
From 04e712d73b32b7148fa8fcf019367ea3230050f3 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Wed, 20 Sep 2023 16:11:31 -0400
Subject: [PATCH v6 3/3] WIP: Insert XLOG_CHECKPOINT_REDO at the redo point.
Merge this into previous commit.
---
src/backend/access/transam/xlog.c | 129 ++++++++++++++++++++++--------
1 file changed, 95 insertions(+), 34 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 026e2fc9da..292646d1cf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -559,6 +559,16 @@ typedef struct XLogCtlData
slock_t info_lck; /* locks shared variables shown above */
} XLogCtlData;
+/*
+ * Classification of XLogRecordInsert operations.
+ */
+typedef enum
+{
+ WALINSERT_NORMAL,
+ WALINSERT_SPECIAL_SWITCH,
+ WALINSERT_SPECIAL_CHECKPOINT
+} WalInsertClass;
+
static XLogCtlData *XLogCtl = NULL;
/* a private copy of XLogCtl->Insert.WALInsertLocks, for convenience */
@@ -739,13 +749,21 @@ XLogInsertRecord(XLogRecData *rdata,
bool inserted;
XLogRecord *rechdr = (XLogRecord *) rdata->data;
uint8 info = rechdr->xl_info & ~XLR_INFO_MASK;
- bool isLogSwitch = (rechdr->xl_rmid == RM_XLOG_ID &&
- info == XLOG_SWITCH);
+ WalInsertClass class = WALINSERT_NORMAL;
XLogRecPtr StartPos;
XLogRecPtr EndPos;
bool prevDoPageWrites = doPageWrites;
TimeLineID insertTLI;
+ /* Does this record type require special handling? */
+ if (rechdr->xl_rmid == RM_XLOG_ID)
+ {
+ if (info == XLOG_SWITCH)
+ class = WALINSERT_SPECIAL_SWITCH;
+ else if (XLOG_CHECKPOINT_REDO)
+ class = WALINSERT_SPECIAL_CHECKPOINT;
+ }
+
/* we assume that all of the record header is in the first chunk */
Assert(rdata->len >= SizeOfXLogRecord);
@@ -793,7 +811,7 @@ XLogInsertRecord(XLogRecData *rdata,
*/
START_CRIT_SECTION();
- if (isLogSwitch)
+ if (class == WALINSERT_SPECIAL_SWITCH)
{
/*
* In order to insert an XLOG_SWITCH record, we need to hold all of
@@ -804,12 +822,27 @@ XLogInsertRecord(XLogRecData *rdata,
* Nonetheless, this case is simpler than the normal cases handled
* below, which must check for changes in doPageWrites and RedoRecPtr.
* Those checks are only needed for records that can contain
- * full-pages images, and an XLOG_SWITCH record never does.
+ * buffer references, and an XLOG_SWITCH record never does.
*/
Assert(fpw_lsn == InvalidXLogRecPtr);
WALInsertLockAcquireExclusive();
inserted = ReserveXLogSwitch(&StartPos, &EndPos, &rechdr->xl_prev);
}
+ else if (class == WALINSERT_SPECIAL_CHECKPOINT)
+ {
+ /*
+ * We need to update both the local and shared copies of RedoRecPtr, which
+ * means that we need to hold all the WAL insertion locks. However, there
+ * can't be any buffer references, so as above, we need not check RedoRecPtr
+ * before inserting the record; we just need to update it afterwards.
+ */
+ Assert(fpw_lsn == InvalidXLogRecPtr);
+ WALInsertLockAcquireExclusive();
+ ReserveXLogInsertLocation(rechdr->xl_tot_len, &StartPos, &EndPos,
+ &rechdr->xl_prev);
+ RedoRecPtr = Insert->RedoRecPtr = StartPos;
+ inserted = true;
+ }
else
{
WALInsertLockAcquire();
@@ -876,7 +909,8 @@ XLogInsertRecord(XLogRecData *rdata,
* All the record data, including the header, is now ready to be
* inserted. Copy the record in the space reserved.
*/
- CopyXLogRecordToWAL(rechdr->xl_tot_len, isLogSwitch, rdata,
+ CopyXLogRecordToWAL(rechdr->xl_tot_len,
+ class == WALINSERT_SPECIAL_SWITCH, rdata,
StartPos, EndPos, insertTLI);
/*
@@ -935,7 +969,7 @@ XLogInsertRecord(XLogRecData *rdata,
* padding space that fills the rest of the segment, and perform
* end-of-segment actions (eg, notifying archiver).
*/
- if (isLogSwitch)
+ if (class == WALINSERT_SPECIAL_SWITCH)
{
TRACE_POSTGRESQL_WAL_SWITCH();
XLogFlush(EndPos);
@@ -6487,6 +6521,8 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset)
* All of this mechanism allows us to continue working while we checkpoint.
* As a result, timing of actions is critical here and be careful to note that
* this function will likely take minutes to execute on a busy system.
+ *
+ * XXX FIX ABOVE COMMENTS
*/
void
CreateCheckPoint(int flags)
@@ -6609,36 +6645,37 @@ CreateCheckPoint(int flags)
checkPoint.fullPageWrites = Insert->fullPageWrites;
- /*
- * Compute new REDO record ptr = location of next XLOG record.
- *
- * NB: this is NOT necessarily where the checkpoint record itself will be,
- * since other backends may insert more XLOG records while we're off doing
- * the buffer flush work. Those XLOG records are logically after the
- * checkpoint, even though physically before it. Got that?
- */
- freespace = INSERT_FREESPACE(curInsert);
- if (freespace == 0)
+ if (shutdown)
{
- if (XLogSegmentOffset(curInsert, wal_segment_size) == 0)
- curInsert += SizeOfXLogLongPHD;
- else
- curInsert += SizeOfXLogShortPHD;
- }
- checkPoint.redo = curInsert;
+ /*
+ * Compute new REDO record ptr = location of next XLOG record.
+ *
+ * Since this is a shutdown checkpoint, there can't be any concurrent WAL
+ * insertion.
+ */
+ freespace = INSERT_FREESPACE(curInsert);
+ if (freespace == 0)
+ {
+ if (XLogSegmentOffset(curInsert, wal_segment_size) == 0)
+ curInsert += SizeOfXLogLongPHD;
+ else
+ curInsert += SizeOfXLogShortPHD;
+ }
+ checkPoint.redo = curInsert;
- /*
- * Here we update the shared RedoRecPtr for future XLogInsert calls; this
- * must be done while holding all the insertion locks.
- *
- * Note: if we fail to complete the checkpoint, RedoRecPtr will be left
- * pointing past where it really needs to point. This is okay; the only
- * consequence is that XLogInsert might back up whole buffers that it
- * didn't really need to. We can't postpone advancing RedoRecPtr because
- * XLogInserts that happen while we are dumping buffers must assume that
- * their buffer changes are not included in the checkpoint.
- */
- RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
+ /*
+ * Here we update the shared RedoRecPtr for future XLogInsert calls; this
+ * must be done while holding all the insertion locks.
+ *
+ * Note: if we fail to complete the checkpoint, RedoRecPtr will be left
+ * pointing past where it really needs to point. This is okay; the only
+ * consequence is that XLogInsert might back up whole buffers that it
+ * didn't really need to. We can't postpone advancing RedoRecPtr because
+ * XLogInserts that happen while we are dumping buffers must assume that
+ * their buffer changes are not included in the checkpoint.
+ */
+ RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
+ }
/*
* Now we can release the WAL insertion locks, allowing other xacts to
@@ -6646,6 +6683,30 @@ CreateCheckPoint(int flags)
*/
WALInsertLockRelease();
+ /*
+ * If this is an online checkpoint, we have not yet determined the redo
+ * point. We do so now by inserting the special XLOG_CHECKPOINT_REDO record;
+ * the LSN at which it starts becomes the new redo pointer. We don't do this
+ * for a shutdown checkpoint, because in that case no WAL can be written
+ * between the redo point and the insertion of the checkpoint record itself,
+ * so the checkpoint record itself services to mark the redo point.
+ */
+ if (!shutdown)
+ {
+ int dummy = 0;
+
+ /* Record must have payload to avoid assertion failure. */
+ XLogBeginInsert();
+ XLogRegisterData((char *) &dummy, sizeof(dummy));
+ (void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
+
+ /*
+ * XLogInsertRecord will have updated RedoRecPtr, but we need to copy that
+ * into the record that will be inserted when the checkpoint is complete.
+ */
+ checkPoint.redo = RedoRecPtr;
+ }
+
/* Update the info_lck-protected copy of RedoRecPtr as well */
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->RedoRecPtr = checkPoint.redo;
--
2.37.1 (Apple Git-137.1)
view thread (2+ messages)
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]
Subject: Re: New WAL record to detect the checkpoint redo location
In-Reply-To: <CA+TgmoZ_C1DKuNT0w13_b_6J8JgSupHFSLBCKgQdhJf3FannbA@mail.gmail.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