Hello Aleksander!
Hello Álvaro!
Thank you for your attention to the patch.
Since BootStrapSlruPage() is the only caller of ZeroPage() it seems to
me that it merely wastes space in SlruCtlData. On top of that I'm not
100% sure if all the supported platforms have C99 compilers with
designated initializers support.
Wouldn't it be simpler to pass the callback straight to
BootStrapSlruPage()?
I agree with your proposals. Moreover, similar solution is also implemented and
exploited at
Tantor's versions of the xid64 patch
(https://www.postgresql.org/message-id/5ca56aed-dc69-4c3e-968f-a822ae0937b5%40gmail.com).
The corrected patch version implementing these proposals is attached.
Best regards,
Evgeny Voropaev,
Tantor Labs LLC.
From a5a29cdbb37834bfcd23cd22db90a78a7db43d18 Mon Sep 17 00:00:00 2001
From: Evgeny Voropaev <evo...@gmail.com>
Date: Fri, 14 Feb 2025 14:03:01 +0800
Subject: [PATCH v2] Elimination of the repetitive code at the SLRU bootstrap
functions.
The functions bootstrapping SLRU pages used to have a lot of repetitive code. The new
realized function BootStrapSlruPage has moved duplicating code into the
single place and eliminated code repetitions.
Author: Evgeny Voropaev <evgeny.vorop...@tantorlabs.com> <evo...@gmail.com>
---
src/backend/access/transam/clog.c | 27 +------------
src/backend/access/transam/commit_ts.c | 24 +-----------
src/backend/access/transam/multixact.c | 54 ++------------------------
src/backend/access/transam/slru.c | 35 +++++++++++++++++
src/backend/access/transam/subtrans.c | 28 ++++++-------
src/include/access/slru.h | 1 +
6 files changed, 55 insertions(+), 114 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 0d556c00b8c..5ae684da6af 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -832,19 +832,7 @@ check_transaction_buffers(int *newval, void **extra, GucSource source)
void
BootStrapCLOG(void)
{
- int slotno;
- LWLock *lock = SimpleLruGetBankLock(XactCtl, 0);
-
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- /* Create and zero the first page of the commit log */
- slotno = ZeroCLOGPage(0, false);
-
- /* Make sure it's written out */
- SimpleLruWritePage(XactCtl, slotno);
- Assert(!XactCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(XactCtl, 0, ZeroCLOGPage);
}
/*
@@ -1114,19 +1102,8 @@ clog_redo(XLogReaderState *record)
if (info == CLOG_ZEROPAGE)
{
int64 pageno;
- int slotno;
- LWLock *lock;
-
memcpy(&pageno, XLogRecGetData(record), sizeof(pageno));
-
- lock = SimpleLruGetBankLock(XactCtl, pageno);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- slotno = ZeroCLOGPage(pageno, false);
- SimpleLruWritePage(XactCtl, slotno);
- Assert(!XactCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(XactCtl, pageno, ZeroCLOGPage);
}
else if (info == CLOG_TRUNCATE)
{
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 95049acd0b5..54600f1b69c 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -747,16 +747,7 @@ ActivateCommitTs(void)
/* Create the current segment file, if necessary */
if (!SimpleLruDoesPhysicalPageExist(CommitTsCtl, pageno))
- {
- LWLock *lock = SimpleLruGetBankLock(CommitTsCtl, pageno);
- int slotno;
-
- LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = ZeroCommitTsPage(pageno, false);
- SimpleLruWritePage(CommitTsCtl, slotno);
- Assert(!CommitTsCtl->shared->page_dirty[slotno]);
- LWLockRelease(lock);
- }
+ BootStrapSlruPage(CommitTsCtl, pageno, ZeroCommitTsPage);
/* Change the activation status in shared memory. */
LWLockAcquire(CommitTsLock, LW_EXCLUSIVE);
@@ -1023,19 +1014,8 @@ commit_ts_redo(XLogReaderState *record)
if (info == COMMIT_TS_ZEROPAGE)
{
int64 pageno;
- int slotno;
- LWLock *lock;
-
memcpy(&pageno, XLogRecGetData(record), sizeof(pageno));
-
- lock = SimpleLruGetBankLock(CommitTsCtl, pageno);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- slotno = ZeroCommitTsPage(pageno, false);
- SimpleLruWritePage(CommitTsCtl, slotno);
- Assert(!CommitTsCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(CommitTsCtl, pageno, ZeroCommitTsPage);
}
else if (info == COMMIT_TS_TRUNCATE)
{
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 27ccdf9500f..30cc47021f7 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -2033,32 +2033,8 @@ check_multixact_member_buffers(int *newval, void **extra, GucSource source)
void
BootStrapMultiXact(void)
{
- int slotno;
- LWLock *lock;
-
- lock = SimpleLruGetBankLock(MultiXactOffsetCtl, 0);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- /* Create and zero the first page of the offsets log */
- slotno = ZeroMultiXactOffsetPage(0, false);
-
- /* Make sure it's written out */
- SimpleLruWritePage(MultiXactOffsetCtl, slotno);
- Assert(!MultiXactOffsetCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
-
- lock = SimpleLruGetBankLock(MultiXactMemberCtl, 0);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- /* Create and zero the first page of the members log */
- slotno = ZeroMultiXactMemberPage(0, false);
-
- /* Make sure it's written out */
- SimpleLruWritePage(MultiXactMemberCtl, slotno);
- Assert(!MultiXactMemberCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(MultiXactOffsetCtl, 0, ZeroMultiXactOffsetPage);
+ BootStrapSlruPage(MultiXactMemberCtl, 0, ZeroMultiXactMemberPage);
}
/*
@@ -3401,36 +3377,14 @@ multixact_redo(XLogReaderState *record)
if (info == XLOG_MULTIXACT_ZERO_OFF_PAGE)
{
int64 pageno;
- int slotno;
- LWLock *lock;
-
memcpy(&pageno, XLogRecGetData(record), sizeof(pageno));
-
- lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- slotno = ZeroMultiXactOffsetPage(pageno, false);
- SimpleLruWritePage(MultiXactOffsetCtl, slotno);
- Assert(!MultiXactOffsetCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(MultiXactOffsetCtl, pageno, ZeroMultiXactOffsetPage);
}
else if (info == XLOG_MULTIXACT_ZERO_MEM_PAGE)
{
int64 pageno;
- int slotno;
- LWLock *lock;
-
memcpy(&pageno, XLogRecGetData(record), sizeof(pageno));
-
- lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno);
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- slotno = ZeroMultiXactMemberPage(pageno, false);
- SimpleLruWritePage(MultiXactMemberCtl, slotno);
- Assert(!MultiXactMemberCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(MultiXactMemberCtl, pageno, ZeroMultiXactMemberPage);
}
else if (info == XLOG_MULTIXACT_CREATE_ID)
{
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 9ce628e62a5..fd332fd4c64 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -1850,3 +1850,38 @@ SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path)
errno = save_errno;
return result;
}
+
+/*
+ * BootStrapSlruPage is a workhorse for functions bootstraping
+ * SLRU pages, including:
+ * BootStrapMultiXact,
+ * BootStrapCLOG,
+ * ActivateCommitTs,
+ * multixact_redo,
+ * commit_ts_redo.
+ *
+ * It performs all of the rut such as acquiring a lock, zeroing,
+ * ensuring that a bootstrapped page is written out, and releasing the lock.
+ *
+ * A caller has to provide a proper zerofunc appropriate for the type of
+ * the page.
+ *
+ */
+void
+BootStrapSlruPage(SlruCtl ctl, int64 pageno, int(*zerofunc)(int64, bool) )
+{
+ int slotno;
+ LWLock *lock;
+
+ lock = SimpleLruGetBankLock(ctl, pageno);
+ LWLockAcquire(lock, LW_EXCLUSIVE);
+
+ /* Create and zero the page*/
+ slotno = (*zerofunc)(pageno, false);
+
+ /* Make sure it's written out */
+ SimpleLruWritePage(ctl, slotno);
+ Assert(!ctl->shared->page_dirty[slotno]);
+
+ LWLockRelease(lock);
+}
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index 15153618fad..ec63c823d66 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -74,7 +74,7 @@ static SlruCtlData SubTransCtlData;
#define SubTransCtl (&SubTransCtlData)
-static int ZeroSUBTRANSPage(int64 pageno);
+static int ZeroSUBTRANSPage(int64 pageno, bool unused);
static bool SubTransPagePrecedes(int64 page1, int64 page2);
@@ -269,19 +269,7 @@ check_subtrans_buffers(int *newval, void **extra, GucSource source)
void
BootStrapSUBTRANS(void)
{
- int slotno;
- LWLock *lock = SimpleLruGetBankLock(SubTransCtl, 0);
-
- LWLockAcquire(lock, LW_EXCLUSIVE);
-
- /* Create and zero the first page of the subtrans log */
- slotno = ZeroSUBTRANSPage(0);
-
- /* Make sure it's written out */
- SimpleLruWritePage(SubTransCtl, slotno);
- Assert(!SubTransCtl->shared->page_dirty[slotno]);
-
- LWLockRelease(lock);
+ BootStrapSlruPage(SubTransCtl, 0, ZeroSUBTRANSPage);
}
/*
@@ -291,10 +279,16 @@ BootStrapSUBTRANS(void)
* The slot number of the new page is returned.
*
* Control lock must be held at entry, and will be held at exit.
+ *
+ * "bool unused" is a parameter required to provide matching of the
+ * ZeroSUBTRANSPage function to the zerofunc function prototype
+ * used by BootStrapSlruPage.
*/
static int
-ZeroSUBTRANSPage(int64 pageno)
+ZeroSUBTRANSPage(int64 pageno, bool unused)
{
+ (void)unused; /* Supress compiler warning */
+
return SimpleLruZeroPage(SubTransCtl, pageno);
}
@@ -335,7 +329,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
prevlock = lock;
}
- (void) ZeroSUBTRANSPage(startPage);
+ (void) ZeroSUBTRANSPage(startPage, false);
if (startPage == endPage)
break;
@@ -395,7 +389,7 @@ ExtendSUBTRANS(TransactionId newestXact)
LWLockAcquire(lock, LW_EXCLUSIVE);
/* Zero the page */
- ZeroSUBTRANSPage(pageno);
+ ZeroSUBTRANSPage(pageno, false);
LWLockRelease(lock);
}
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index e142800aab2..515986b4506 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -214,5 +214,6 @@ extern bool SlruScanDirCbReportPresence(SlruCtl ctl, char *filename,
extern bool SlruScanDirCbDeleteAll(SlruCtl ctl, char *filename, int64 segpage,
void *data);
extern bool check_slru_buffers(const char *name, int *newval);
+extern void BootStrapSlruPage( SlruCtl ctl, int64 pageno, int(*zerofunc)(int64, bool) );
#endif /* SLRU_H */
--
2.47.1
