public inbox for [email protected]
help / color / mirror / Atom feedFrom: Aleksander Alekseev <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: [PATCH] Refactor SLRU to always use long file names
Date: Wed, 11 Sep 2024 16:07:06 +0300
Message-ID: <CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz=mjXHDb94GORtM_Eyg@mail.gmail.com> (raw)
Hi,
Commit 4ed8f0913bfd introduced long SLRU file names. The proposed
patch removes SlruCtl->long_segment_names flag and makes SLRU always
use long file names. This simplifies both the code and the API.
Corresponding changes to pg_upgrade are included.
One drawback I see is that technically SLRU is an exposed API and
changing it may affect third-party code. I'm not sure if we should
seriously worry about this. Firstly, the change is trivial and
secondly, it's not clear whether such third-party code even exists (we
broke this API just recently in 4ed8f0913bfd and no one complained).
I didn't include any tests for the new pg_upgrade code. To my
knowledge we test it manually, with buildfarm members and during
alpha- and beta-testing periods. Please let me know if you think there
should be a corresponding TAP test.
Thoughts?
--
Best regards,
Aleksander Alekseev
Attachments:
[application/octet-stream] v1-0001-Always-use-long-SLRU-segment-file-names.patch (14.8K, 2-v1-0001-Always-use-long-SLRU-segment-file-names.patch)
download | inline diff:
From c297163e9b69d6f978301eeb629834b73e81e2d4 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 11 Sep 2024 13:17:33 +0300
Subject: [PATCH v1] Always use long SLRU segment file names
PG17 introduced long SLRU segment file names (commit 4ed8f0913bfd). We used
short or long file names depending on SlruCtl->long_segment_names. This commit
refactors SLRU to always use long file names in order to simplify the code.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
src/backend/access/transam/clog.c | 2 +-
src/backend/access/transam/commit_ts.c | 3 +-
src/backend/access/transam/multixact.c | 6 +-
src/backend/access/transam/slru.c | 73 ++++---------------
src/backend/access/transam/subtrans.c | 2 +-
src/backend/commands/async.c | 2 +-
src/backend/storage/lmgr/predicate.c | 2 +-
src/bin/pg_upgrade/pg_upgrade.c | 78 +++++++++++++++++++++
src/bin/pg_upgrade/pg_upgrade.h | 6 ++
src/bin/pg_verifybackup/t/003_corruption.pl | 2 +-
src/include/access/slru.h | 10 +--
src/test/modules/test_slru/test_slru.c | 8 +--
12 files changed, 108 insertions(+), 86 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index e6f79320e9..71fa291c10 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -810,7 +810,7 @@ CLOGShmemInit(void)
XactCtl->PagePrecedes = CLOGPagePrecedes;
SimpleLruInit(XactCtl, "transaction", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE,
"pg_xact", LWTRANCHE_XACT_BUFFER,
- LWTRANCHE_XACT_SLRU, SYNC_HANDLER_CLOG, false);
+ LWTRANCHE_XACT_SLRU, SYNC_HANDLER_CLOG);
SlruPagePrecedesUnitTests(XactCtl, CLOG_XACTS_PER_PAGE);
}
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 77e1899d7a..44de962a28 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -556,8 +556,7 @@ CommitTsShmemInit(void)
SimpleLruInit(CommitTsCtl, "commit_timestamp", CommitTsShmemBuffers(), 0,
"pg_commit_ts", LWTRANCHE_COMMITTS_BUFFER,
LWTRANCHE_COMMITTS_SLRU,
- SYNC_HANDLER_COMMIT_TS,
- false);
+ SYNC_HANDLER_COMMIT_TS);
SlruPagePrecedesUnitTests(CommitTsCtl, COMMIT_TS_XACTS_PER_PAGE);
commitTsShared = ShmemInitStruct("CommitTs shared",
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 8c37d7eba7..4072a78f3c 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1974,15 +1974,13 @@ MultiXactShmemInit(void)
"multixact_offset", multixact_offset_buffers, 0,
"pg_multixact/offsets", LWTRANCHE_MULTIXACTOFFSET_BUFFER,
LWTRANCHE_MULTIXACTOFFSET_SLRU,
- SYNC_HANDLER_MULTIXACT_OFFSET,
- false);
+ SYNC_HANDLER_MULTIXACT_OFFSET);
SlruPagePrecedesUnitTests(MultiXactOffsetCtl, MULTIXACT_OFFSETS_PER_PAGE);
SimpleLruInit(MultiXactMemberCtl,
"multixact_member", multixact_member_buffers, 0,
"pg_multixact/members", LWTRANCHE_MULTIXACTMEMBER_BUFFER,
LWTRANCHE_MULTIXACTMEMBER_SLRU,
- SYNC_HANDLER_MULTIXACT_MEMBER,
- false);
+ SYNC_HANDLER_MULTIXACT_MEMBER);
/* doesn't call SimpleLruTruncate() or meet criteria for unit tests */
/* Initialize our shared state struct */
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index e7f73bf427..d5cc5fc6e2 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -77,42 +77,22 @@
*
* "path" should point to a buffer at least MAXPGPATH characters long.
*
- * If ctl->long_segment_names is true, segno can be in the range [0, 2^60-1].
- * The resulting file name is made of 15 characters, e.g. dir/123456789ABCDEF.
- *
- * If ctl->long_segment_names is false, segno can be in the range [0, 2^24-1].
- * The resulting file name is made of 4 to 6 characters, as of:
- *
- * dir/1234 for [0, 2^16-1]
- * dir/12345 for [2^16, 2^20-1]
- * dir/123456 for [2^20, 2^24-1]
+ * segno can be in the range [0, 2^60-1]. The resulting file name is made
+ * of 15 characters, e.g. dir/123456789ABCDEF.
*/
static inline int
SlruFileName(SlruCtl ctl, char *path, int64 segno)
{
- if (ctl->long_segment_names)
- {
- /*
- * We could use 16 characters here but the disadvantage would be that
- * the SLRU segments will be hard to distinguish from WAL segments.
- *
- * For this reason we use 15 characters. It is enough but also means
- * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
- */
- Assert(segno >= 0 && segno <= INT64CONST(0xFFFFFFFFFFFFFFF));
- return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
- (long long) segno);
- }
- else
- {
- /*
- * Despite the fact that %04X format string is used up to 24 bit
- * integers are allowed. See SlruCorrectSegmentFilenameLength()
- */
- Assert(segno >= 0 && segno <= INT64CONST(0xFFFFFF));
- return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
- (unsigned int) segno);
- }
+ /*
+ * We could use 16 characters here but the disadvantage would be that
+ * the SLRU segments will be hard to distinguish from WAL segments.
+ *
+ * For this reason we use 15 characters. It is enough but also means
+ * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
+ */
+ Assert(segno >= 0 && segno <= INT64CONST(0xFFFFFFFFFFFFFFF));
+ return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
+ (long long) segno);
}
/*
@@ -251,7 +231,7 @@ SimpleLruAutotuneBuffers(int divisor, int max)
void
SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
const char *subdir, int buffer_tranche_id, int bank_tranche_id,
- SyncRequestHandler sync_handler, bool long_segment_names)
+ SyncRequestHandler sync_handler)
{
SlruShared shared;
bool found;
@@ -342,7 +322,6 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
*/
ctl->shared = shared;
ctl->sync_handler = sync_handler;
- ctl->long_segment_names = long_segment_names;
ctl->bank_mask = (nslots / SLRU_BANK_SIZE) - 1;
strlcpy(ctl->Dir, subdir, sizeof(ctl->Dir));
}
@@ -1745,30 +1724,6 @@ SlruScanDirCbDeleteAll(SlruCtl ctl, char *filename, int64 segpage, void *data)
return false; /* keep going */
}
-/*
- * An internal function used by SlruScanDirectory().
- *
- * Returns true if a file with a name of a given length may be a correct
- * SLRU segment.
- */
-static inline bool
-SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
-{
- if (ctl->long_segment_names)
- return (len == 15); /* see SlruFileName() */
- else
-
- /*
- * Commit 638cf09e76d allowed 5-character lengths. Later commit
- * 73c986adde5 allowed 6-character length.
- *
- * Note: There is an ongoing plan to migrate all SLRUs to 64-bit page
- * numbers, and the corresponding 15-character file names, which may
- * eventually deprecate the support for 4, 5, and 6-character names.
- */
- return (len == 4 || len == 5 || len == 6);
-}
-
/*
* Scan the SimpleLru directory and apply a callback to each file found in it.
*
@@ -1800,7 +1755,7 @@ SlruScanDirectory(SlruCtl ctl, SlruScanCallback callback, void *data)
len = strlen(clde->d_name);
- if (SlruCorrectSegmentFilenameLength(ctl, len) &&
+ if ((len == 15) &&
strspn(clde->d_name, "0123456789ABCDEF") == len)
{
segno = strtoi64(clde->d_name, NULL, 16);
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index 50bb1d8cfc..b92d7d4e27 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -243,7 +243,7 @@ SUBTRANSShmemInit(void)
SubTransCtl->PagePrecedes = SubTransPagePrecedes;
SimpleLruInit(SubTransCtl, "subtransaction", SUBTRANSShmemBuffers(), 0,
"pg_subtrans", LWTRANCHE_SUBTRANS_BUFFER,
- LWTRANCHE_SUBTRANS_SLRU, SYNC_HANDLER_NONE, false);
+ LWTRANCHE_SUBTRANS_SLRU, SYNC_HANDLER_NONE);
SlruPagePrecedesUnitTests(SubTransCtl, SUBTRANS_XACTS_PER_PAGE);
}
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 8ed503e1c1..84e89e6f00 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -537,7 +537,7 @@ AsyncShmemInit(void)
NotifyCtl->PagePrecedes = asyncQueuePagePrecedes;
SimpleLruInit(NotifyCtl, "notify", notify_buffers, 0,
"pg_notify", LWTRANCHE_NOTIFY_BUFFER, LWTRANCHE_NOTIFY_SLRU,
- SYNC_HANDLER_NONE, true);
+ SYNC_HANDLER_NONE);
if (!found)
{
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index e24a0f2fdb..9e8a4c7851 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -814,7 +814,7 @@ SerialInit(void)
SimpleLruInit(SerialSlruCtl, "serializable",
serializable_buffers, 0, "pg_serial",
LWTRANCHE_SERIAL_BUFFER, LWTRANCHE_SERIAL_SLRU,
- SYNC_HANDLER_NONE, false);
+ SYNC_HANDLER_NONE);
#ifdef USE_ASSERT_CHECKING
SerialPagePrecedesLogicallyUnitTests();
#endif
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 663235816f..9a78ecc91a 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -38,6 +38,7 @@
#include "postgres_fe.h"
+#include <dirent.h>
#include <time.h>
#include "catalog/pg_class_d.h"
@@ -59,6 +60,8 @@ static void prepare_new_cluster(void);
static void prepare_new_globals(void);
static void create_new_objects(void);
static void copy_xact_xlog_xid(void);
+static void check_slru_segment_filenames(void);
+static void rename_slru_segments(const char *dirname);
static void set_frozenxids(bool minmxid_only);
static void make_outputdirs(char *pgdata);
static void setup(char *argv0);
@@ -154,6 +157,7 @@ main(int argc, char **argv)
*/
copy_xact_xlog_xid();
+ check_slru_segment_filenames();
/* New now using xids of the old system */
@@ -806,6 +810,80 @@ copy_xact_xlog_xid(void)
check_ok();
}
+static void
+rename_slru_segments(const char* dirname)
+{
+ DIR *dir;
+ struct dirent *de;
+ int len;
+ int64 segno;
+ char dir_path[MAXPGPATH];
+ char old_path[MAXPGPATH];
+ char new_path[MAXPGPATH];
+
+ prep_status("Renaming SLRU segments in %s", dirname);
+ snprintf(dir_path, sizeof(dir_path), "%s/%s", new_cluster.pgdata, dirname);
+
+ dir = opendir(dir_path);
+ if (dir == NULL)
+ pg_fatal("could not open directory \"%s\": %m", dir_path);
+
+ while (errno = 0, (de = readdir(dir)) != NULL)
+ {
+ /*
+ * ignore '.', '..' and everything else that doesn't look
+ * like an SLRU segment with a short file name
+ */
+
+ len = strlen(de->d_name);
+ if(len != 4 && len != 5 && len != 6)
+ continue;
+
+ if(strspn(de->d_name, "0123456789ABCDEF") != len)
+ continue;
+
+ segno = strtoi64(de->d_name, NULL, 16);
+ snprintf(new_path, MAXPGPATH, "%s/%015llX", dir_path,
+ (long long) segno);
+ snprintf(old_path, MAXPGPATH, "%s/%s", dir_path, de->d_name);
+
+ if (pg_mv_file(old_path, new_path) != 0)
+ pg_fatal("could not rename file \"%s\" to \"%s\": %m",
+ old_path, new_path);
+ }
+
+ if (errno)
+ pg_fatal("could not read directory \"%s\": %m", dir_path);
+
+ if (closedir(dir))
+ pg_fatal("could not close directory \"%s\": %m", dir_path);
+
+ check_ok();
+}
+
+static void
+check_slru_segment_filenames(void)
+{
+ int i;
+ static const char* dirs[] = {
+ "pg_xact",
+ "pg_commit_ts",
+ "pg_multixact/offsets",
+ "pg_multixact/members",
+ "pg_subtrans",
+ "pg_serial",
+ };
+
+ /*
+ TODO FIXME UNCOMMENT BEFORE COMMITTING
+ if(new_cluster.controldata.cat_ver < SLRU_SEG_FILENAMES_CHANGE_CAT_VER)
+ return;
+ */
+
+ for (i = 0; i < sizeof(dirs)/sizeof(dirs[0]); i++)
+ rename_slru_segments(dirs[i]);
+}
+
/*
* set_frozenxids()
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index cdb6e2b759..ba3ad0f490 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -125,6 +125,12 @@ extern char *output_files[];
*/
#define JSONB_FORMAT_CHANGE_CAT_VER 201409291
+/*
+ * change of SLRU segment filenames length in 18.0
+ * TODO FIXME CHANGE TO THE ACTUAL VALUE BEFORE COMMITTING
+ */
+#define SLRU_SEG_FILENAMES_CHANGE_CAT_VER 20241001
+
/*
* Each relation is represented by a relinfo structure.
diff --git a/src/bin/pg_verifybackup/t/003_corruption.pl b/src/bin/pg_verifybackup/t/003_corruption.pl
index ae91e04338..b4ea3a8bd9 100644
--- a/src/bin/pg_verifybackup/t/003_corruption.pl
+++ b/src/bin/pg_verifybackup/t/003_corruption.pl
@@ -182,7 +182,7 @@ sub mutilate_extra_tablespace_file
sub mutilate_missing_file
{
my ($backup_path) = @_;
- my $pathname = "$backup_path/pg_xact/0000";
+ my $pathname = "$backup_path/pg_xact/000000000000000";
unlink($pathname) || die "$pathname: $!";
return;
}
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index 97e612cd10..94c2e3712d 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -133,13 +133,6 @@ typedef struct SlruCtlData
*/
bits16 bank_mask;
- /*
- * If true, use long segment file names. Otherwise, use short file names.
- *
- * For details about the file name format, see SlruFileName().
- */
- bool long_segment_names;
-
/*
* Which sync handler function to use when handing sync requests over to
* the checkpointer. SYNC_HANDLER_NONE to disable fsync (eg pg_notify).
@@ -187,8 +180,7 @@ extern Size SimpleLruShmemSize(int nslots, int nlsns);
extern int SimpleLruAutotuneBuffers(int divisor, int max);
extern void SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
const char *subdir, int buffer_tranche_id,
- int bank_tranche_id, SyncRequestHandler sync_handler,
- bool long_segment_names);
+ int bank_tranche_id, SyncRequestHandler sync_handler);
extern int SimpleLruZeroPage(SlruCtl ctl, int64 pageno);
extern int SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
TransactionId xid);
diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
index d227b06703..de6a6bffd2 100644
--- a/src/test/modules/test_slru/test_slru.c
+++ b/src/test/modules/test_slru/test_slru.c
@@ -213,11 +213,6 @@ test_slru_page_precedes_logically(int64 page1, int64 page2)
static void
test_slru_shmem_startup(void)
{
- /*
- * Short segments names are well tested elsewhere so in this test we are
- * focusing on long names.
- */
- const bool long_segment_names = true;
const char slru_dir_name[] = "pg_test_slru";
int test_tranche_id;
int test_buffer_tranche_id;
@@ -241,8 +236,7 @@ test_slru_shmem_startup(void)
TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically;
SimpleLruInit(TestSlruCtl, "TestSLRU",
NUM_TEST_BUFFERS, 0, slru_dir_name,
- test_buffer_tranche_id, test_tranche_id, SYNC_HANDLER_NONE,
- long_segment_names);
+ test_buffer_tranche_id, test_tranche_id, SYNC_HANDLER_NONE);
}
void
--
2.46.0
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]
Subject: Re: [PATCH] Refactor SLRU to always use long file names
In-Reply-To: <CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz=mjXHDb94GORtM_Eyg@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