Hi hackers,
while working on a replication slot tool (idea is to put it in contrib, not
shared yet), I realized that "pg_replslot" is being used > 25 times in
.c files.
I think it would make sense to replace those occurrences with a $SUBJECT,
attached
a patch doing so.
There is 2 places where it is not done:
src/bin/initdb/initdb.c
src/bin/pg_rewind/filemap.c
for consistency with the existing PG_STAT_TMP_DIR define.
Out of curiosity, checking the sizes of affected files (O2, no debug):
with patch:
text data bss dec hex filename
20315 224 17 20556 504c src/backend/backup/basebackup.o
30304 0 8 30312 7668
src/backend/replication/logical/reorderbuffer.o
23812 36 40 23888 5d50 src/backend/replication/slot.o
6367 0 0 6367 18df src/backend/utils/adt/genfile.o
40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
without patch:
text data bss dec hex filename
20315 224 17 20556 504c src/backend/backup/basebackup.o
30286 0 8 30294 7656
src/backend/replication/logical/reorderbuffer.o
23766 36 40 23842 5d22 src/backend/replication/slot.o
6363 0 0 6363 18db src/backend/utils/adt/genfile.o
40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
Also, I think we could do the same for:
pg_notify
pg_serial
pg_subtrans
pg_wal
pg_multixact
pg_tblspc
pg_logical
And I volunteer to do so if you think that makes sense.
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From ab65e1a4a18d624d17841990baa63a56dc1d0747 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Wed, 14 Aug 2024 09:16:21 +0000
Subject: [PATCH v1] Define PG_REPLSLOT_DIR
Replace most of the places where "pg_replslot" is used in .c files with a new
PG_REPLSLOT_DIR define. The places where it is not done is for consistency with
the existing PG_STAT_TMP_DIR define.
---
src/backend/backup/basebackup.c | 3 +-
.../replication/logical/reorderbuffer.c | 15 +++++----
src/backend/replication/slot.c | 32 +++++++++----------
src/backend/utils/adt/genfile.c | 5 +--
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_rewind/filemap.c | 2 +-
src/include/replication/slot.h | 2 ++
7 files changed, 33 insertions(+), 28 deletions(-)
25.9% src/backend/replication/logical/
56.5% src/backend/replication/
9.4% src/backend/utils/adt/
4.4% src/bin/
3.5% src/
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 01b35e26bd..de16afac74 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -36,6 +36,7 @@
#include "port.h"
#include "postmaster/syslogger.h"
#include "postmaster/walsummarizer.h"
+#include "replication/slot.h"
#include "replication/walsender.h"
#include "replication/walsender_private.h"
#include "storage/bufpage.h"
@@ -161,7 +162,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ PG_REPLSLOT_DIR,
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
PG_DYNSHMEM_DIR,
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..6f98115d1e 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4573,7 +4573,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
- sprintf(path, "pg_replslot/%s", slotname);
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, slotname);
/* we're only handling directories here, skip if it's not ours */
if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
@@ -4586,14 +4586,14 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
if (strncmp(spill_de->d_name, "xid", 3) == 0)
{
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
if (unlink(path) != 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ PG_REPLSLOT_DIR, path, slotname)));
}
}
FreeDir(spill_dir);
@@ -4612,7 +4612,8 @@ ReorderBufferSerializedPath(char *path, ReplicationSlot *slot, TransactionId xid
XLogSegNoOffsetToRecPtr(segno, 0, wal_segment_size, recptr);
- snprintf(path, MAXPGPATH, "pg_replslot/%s/xid-%u-lsn-%X-%X.spill",
+ snprintf(path, MAXPGPATH, "%s/%s/xid-%u-lsn-%X-%X.spill",
+ PG_REPLSLOT_DIR,
NameStr(MyReplicationSlot->data.name),
xid, LSN_FORMAT_ARGS(recptr));
}
@@ -4627,8 +4628,8 @@ StartupReorderBuffer(void)
DIR *logical_dir;
struct dirent *logical_de;
- logical_dir = AllocateDir("pg_replslot");
- while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
+ logical_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((logical_de = ReadDir(logical_dir, PG_REPLSLOT_DIR)) != NULL)
{
if (strcmp(logical_de->d_name, ".") == 0 ||
strcmp(logical_de->d_name, "..") == 0)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c290339af5..2e7366ef5f 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -20,7 +20,7 @@
* on standbys (to support cascading setups). The requirement that slots be
* usable on standbys precludes storing them in the system catalogs.
*
- * Each replication slot gets its own directory inside the $PGDATA/pg_replslot
+ * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
* directory. Inside that directory the state file will contain the slot's
* own data. Additional data can be stored alongside that file if required.
* While the server is running, the state data is also cached in memory for
@@ -916,8 +916,8 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
LWLockAcquire(ReplicationSlotAllocationLock, LW_EXCLUSIVE);
/* Generate pathnames. */
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* Rename the slot directory on disk, so that we'll no longer recognize
@@ -938,7 +938,7 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
*/
START_CRIT_SECTION();
fsync_fname(tmppath, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
else
@@ -1016,7 +1016,7 @@ ReplicationSlotSave(void)
Assert(MyReplicationSlot != NULL);
- sprintf(path, "pg_replslot/%s", NameStr(MyReplicationSlot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(MyReplicationSlot->data.name));
SaveSlotToPath(MyReplicationSlot, path, ERROR);
}
@@ -1881,7 +1881,7 @@ CheckPointReplicationSlots(bool is_shutdown)
continue;
/* save the slot to disk, locking is handled in SaveSlotToPath() */
- sprintf(path, "pg_replslot/%s", NameStr(s->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(s->data.name));
/*
* Slot's data is not flushed each time the confirmed_flush LSN is
@@ -1922,8 +1922,8 @@ StartupReplicationSlots(void)
elog(DEBUG1, "starting up replication slots");
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
char path[MAXPGPATH + 12];
PGFileType de_type;
@@ -1932,7 +1932,7 @@ StartupReplicationSlots(void)
strcmp(replication_de->d_name, "..") == 0)
continue;
- snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, replication_de->d_name);
de_type = get_dirent_type(path, replication_de, false, DEBUG1);
/* we're only creating directories here, skip if it's not our's */
@@ -1949,7 +1949,7 @@ StartupReplicationSlots(void)
path)));
continue;
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
continue;
}
@@ -1987,8 +1987,8 @@ CreateSlotOnDisk(ReplicationSlot *slot)
* takes out the lock, if we'd take the lock here, we'd deadlock.
*/
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* It's just barely possible that some previous effort to create or drop a
@@ -2027,7 +2027,7 @@ CreateSlotOnDisk(ReplicationSlot *slot)
START_CRIT_SECTION();
fsync_fname(path, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
@@ -2170,7 +2170,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
fsync_fname(path, false);
fsync_fname(dir, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
@@ -2205,7 +2205,7 @@ RestoreSlotFromDisk(const char *name)
/* no need to lock here, no concurrent access allowed yet */
/* delete temp file if it exists */
- sprintf(slotdir, "pg_replslot/%s", name);
+ sprintf(slotdir, "%s/%s", PG_REPLSLOT_DIR, name);
sprintf(path, "%s/state.tmp", slotdir);
if (unlink(path) < 0 && errno != ENOENT)
ereport(PANIC,
@@ -2332,7 +2332,7 @@ RestoreSlotFromDisk(const char *name)
(errmsg("could not remove directory \"%s\"",
slotdir)));
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
return;
}
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 0d82557417..e645e6b85a 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -708,7 +708,7 @@ pg_ls_logicalmapdir(PG_FUNCTION_ARGS)
}
/*
- * Function to return the list of files in the pg_replslot/<replication_slot>
+ * Function to return the list of files in the PG_REPLSLOT_DIR/<replication_slot>
* directory.
*/
Datum
@@ -728,6 +728,7 @@ pg_ls_replslotdir(PG_FUNCTION_ARGS)
errmsg("replication slot \"%s\" does not exist",
slotname)));
- snprintf(path, sizeof(path), "pg_replslot/%s", slotname);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, slotname);
+
return pg_ls_dir_files(fcinfo, path, false);
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f00718a015..2ee55eced0 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -242,7 +242,7 @@ static const char *const subdirs[] = {
"pg_multixact/offsets",
"base",
"base/1",
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
"pg_tblspc",
"pg_stat",
"pg_stat_tmp",
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4458324c9d..00e644d988 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -97,7 +97,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
"pg_dynshmem", /* defined as PG_DYNSHMEM_DIR */
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index c2ee149fd6..9665faac1c 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -17,6 +17,8 @@
#include "storage/spin.h"
#include "replication/walreceiver.h"
+#define PG_REPLSLOT_DIR "pg_replslot"
+
/*
* Behaviour of replication slots, upon release or crash.
*
--
2.34.1
