On 03.09.2024 08:37, Kyotaro Horiguchi wrote:
At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson <dan...@yesql.se> wrote in
Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE
consistently as per the original patch.
1)
A few comments on the patch though:
- * reads the data from $PGDATA/global/pg_control
+ * reads the data from $PGDATA/<control file>
I don't think this is an improvement, I'd leave that one as the filename
spelled out.
I agree with the first point. In fact, I think it might be even better
to just write something like "reads the data from the control file" in
plain language rather than using the actual file name.
Thanks for remarks! Agreed with both. Tried to fix in v2 attached.
2)
- "the \".old\" suffix from %s/global/pg_control.old.\n"
+ "the \".old\" suffix from %s/%s.old.\n"
Same with that change, not sure I think that makes reading the errormessage
code any easier.
As for the
second point, I'm fine either way, but if the main goal is to ensure
resilience against future changes to the value of XLOG_CONTROL_FILE,
then changing it makes sense. On the other hand, I don't see any
strong reasons not to change it. That said, I don't really expect the
value to change in the first place.
In v2 removed XLOG_CONTROL_FILE from args and used it directly in the string.
IMHO this makes the code more readable and will output the correct
text if the macro changes.
3)
The following point caught my attention.
+++ b/src/backend/postmaster/postmaster.c
...
+#include "access/xlog_internal.h"
The name xlog_internal suggests that the file should only be included
by modules dealing with XLOG details, and postmaster.c doesn't seem to
fit that category. If the macro is used more broadly, it might be
better to move it to a more public location. However, following the
current discussion, if we decide to keep the macro's name as it is, it
would make more sense to keep it in its current location.
Maybe include/access/xlogdefs.h would be a good place for this?
In v2 moved some definitions from xlog_internal to xlogdefs
and removed including xlog_internal.h from the postmaster.c.
Please, take a look on it.
With the best wishes,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From 0b1435492e293ee5553a69c2b3c560b93addc82e Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <a.melni...@postgrespro.ru>
Date: Fri, 19 Apr 2024 06:12:44 +0300
Subject: [PATCH] Use XLOG_CONTROL_FILE macro everywhere in C code
---
src/backend/backup/basebackup.c | 2 +-
src/backend/postmaster/postmaster.c | 2 +-
src/bin/pg_combinebackup/pg_combinebackup.c | 5 +++--
src/bin/pg_controldata/pg_controldata.c | 3 ++-
src/bin/pg_rewind/filemap.c | 3 ++-
src/bin/pg_rewind/pg_rewind.c | 8 ++++----
src/bin/pg_upgrade/controldata.c | 9 +++++----
src/bin/pg_verifybackup/pg_verifybackup.c | 3 ++-
src/common/controldata_utils.c | 2 +-
src/include/access/xlogdefs.h | 15 +++++++++++++++
10 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 9a2bf59e84..c5a1e18104 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -1347,7 +1347,7 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
snprintf(pathbuf, sizeof(pathbuf), "%s/%s", path, de->d_name);
/* Skip pg_control here to back up it last */
- if (strcmp(pathbuf, "./global/pg_control") == 0)
+ if (strcmp(pathbuf, "./" XLOG_CONTROL_FILE) == 0)
continue;
if (lstat(pathbuf, &statbuf) != 0)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7f3170a8f0..1a34ed1cab 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1489,7 +1489,7 @@ checkControlFile(void)
char path[MAXPGPATH];
FILE *fp;
- snprintf(path, sizeof(path), "%s/global/pg_control", DataDir);
+ snprintf(path, sizeof(path), "%s/%s", DataDir, XLOG_CONTROL_FILE);
fp = AllocateFile(path, PG_BINARY_R);
if (fp == NULL)
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index b26c532445..16448e78b8 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -16,6 +16,7 @@
#include <fcntl.h>
#include <limits.h>
+#include "access/xlog_internal.h"
#include "backup_label.h"
#include "common/blkreftable.h"
#include "common/checksum_helper.h"
@@ -284,7 +285,7 @@ main(int argc, char *argv[])
{
char *controlpath;
- controlpath = psprintf("%s/%s", prior_backup_dirs[i], "global/pg_control");
+ controlpath = psprintf("%s/%s", prior_backup_dirs[i], XLOG_CONTROL_FILE);
pg_fatal("%s: manifest system identifier is %llu, but control file has %llu",
controlpath,
@@ -591,7 +592,7 @@ check_control_files(int n_backups, char **backup_dirs)
bool crc_ok;
char *controlpath;
- controlpath = psprintf("%s/%s", backup_dirs[i], "global/pg_control");
+ controlpath = psprintf("%s/%s", backup_dirs[i], XLOG_CONTROL_FILE);
pg_log_debug("reading \"%s\"", controlpath);
control_file = get_controlfile_by_exact_path(controlpath, &crc_ok);
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 93e0837947..ba3e18c918 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -1,7 +1,8 @@
/*
* pg_controldata
*
- * reads the data from $PGDATA/global/pg_control
+ * reads the data from reads the data from the control file
+ * which is located at $PGDATA/XLOG_CONTROL_FILE
*
* copyright (c) Oliver Elphick <o...@lfix.co.uk>, 2001;
* license: BSD
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4458324c9d..6224e94d09 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -26,6 +26,7 @@
#include <sys/stat.h>
#include <unistd.h>
+#include "access/xlog_internal.h"
#include "catalog/pg_tablespace_d.h"
#include "common/file_utils.h"
#include "common/hashfn_unstable.h"
@@ -643,7 +644,7 @@ decide_file_action(file_entry_t *entry)
* Don't touch the control file. It is handled specially, after copying
* all the other files.
*/
- if (strcmp(path, "global/pg_control") == 0)
+ if (strcmp(path, XLOG_CONTROL_FILE) == 0)
return FILE_ACTION_NONE;
/* Skip macOS system files */
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 8449ae78ef..213a8b22cd 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -328,7 +328,7 @@ main(int argc, char **argv)
* need to make sure by themselves that the target cluster is in a clean
* state.
*/
- buffer = slurpFile(datadir_target, "global/pg_control", &size);
+ buffer = slurpFile(datadir_target, XLOG_CONTROL_FILE, &size);
digestControlFile(&ControlFile_target, buffer, size);
pg_free(buffer);
@@ -338,12 +338,12 @@ main(int argc, char **argv)
{
ensureCleanShutdown(argv[0]);
- buffer = slurpFile(datadir_target, "global/pg_control", &size);
+ buffer = slurpFile(datadir_target, XLOG_CONTROL_FILE, &size);
digestControlFile(&ControlFile_target, buffer, size);
pg_free(buffer);
}
- buffer = source->fetch_file(source, "global/pg_control", &size);
+ buffer = source->fetch_file(source, XLOG_CONTROL_FILE, &size);
digestControlFile(&ControlFile_source, buffer, size);
pg_free(buffer);
@@ -631,7 +631,7 @@ perform_rewind(filemap_t *filemap, rewind_source *source,
* Fetch the control file from the source last. This ensures that the
* minRecoveryPoint is up-to-date.
*/
- buffer = source->fetch_file(source, "global/pg_control", &size);
+ buffer = source->fetch_file(source, XLOG_CONTROL_FILE, &size);
digestControlFile(&ControlFile_source_after, buffer, size);
pg_free(buffer);
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 1f0ccea3ed..0527ae76d8 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -11,6 +11,7 @@
#include <ctype.h>
+#include "access/xlog_internal.h"
#include "common/string.h"
#include "pg_upgrade.h"
@@ -714,10 +715,10 @@ disable_old_cluster(void)
new_path[MAXPGPATH];
/* rename pg_control so old server cannot be accidentally started */
- prep_status("Adding \".old\" suffix to old global/pg_control");
+ prep_status("Adding \".old\" suffix to old " XLOG_CONTROL_FILE);
- snprintf(old_path, sizeof(old_path), "%s/global/pg_control", old_cluster.pgdata);
- snprintf(new_path, sizeof(new_path), "%s/global/pg_control.old", old_cluster.pgdata);
+ snprintf(old_path, sizeof(old_path), "%s/%s", old_cluster.pgdata, XLOG_CONTROL_FILE);
+ snprintf(new_path, sizeof(new_path), "%s/%s.old", old_cluster.pgdata, XLOG_CONTROL_FILE);
if (pg_mv_file(old_path, new_path) != 0)
pg_fatal("could not rename file \"%s\" to \"%s\": %m",
old_path, new_path);
@@ -725,7 +726,7 @@ disable_old_cluster(void)
pg_log(PG_REPORT, "\n"
"If you want to start the old cluster, you will need to remove\n"
- "the \".old\" suffix from %s/global/pg_control.old.\n"
+ "the \".old\" suffix from %s/" XLOG_CONTROL_FILE ".old.\n"
"Because \"link\" mode was used, the old cluster cannot be safely\n"
"started once the new cluster has been started.",
old_cluster.pgdata);
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index fd610c20a6..53338df397 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -18,6 +18,7 @@
#include <sys/stat.h>
#include <time.h>
+#include "access/xlog_internal.h"
#include "common/controldata_utils.h"
#include "common/hashfn_unstable.h"
#include "common/logging.h"
@@ -735,7 +736,7 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath)
* version 1.
*/
if (context->manifest->version != 1 &&
- strcmp(relpath, "global/pg_control") == 0)
+ strcmp(relpath, XLOG_CONTROL_FILE) == 0)
verify_control_file(fullpath, context->manifest->system_identifier);
/* Update statistics for progress report, if necessary */
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 82309b2510..411139eeb0 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -53,7 +53,7 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
{
char ControlFilePath[MAXPGPATH];
- snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
+ snprintf(ControlFilePath, MAXPGPATH, "%s/%s", DataDir, XLOG_CONTROL_FILE);
return get_controlfile_by_exact_path(ControlFilePath, crc_ok_p);
}
diff --git a/src/include/access/xlogdefs.h b/src/include/access/xlogdefs.h
index 3009798952..9ee9ffad70 100644
--- a/src/include/access/xlogdefs.h
+++ b/src/include/access/xlogdefs.h
@@ -28,6 +28,21 @@ typedef uint64 XLogRecPtr;
#define InvalidXLogRecPtr 0
#define XLogRecPtrIsInvalid(r) ((r) == InvalidXLogRecPtr)
+/*
+ * The XLog directory and control file (relative to $PGDATA)
+ */
+#define XLOGDIR "pg_wal"
+#define XLOG_CONTROL_FILE "global/pg_control"
+
+/*
+ * These macros encapsulate knowledge about the exact layout of XLog file
+ * names, timeline history file names, and archive-status file names.
+ */
+#define MAXFNAMELEN 64
+
+/* Length of XLog file name */
+#define XLOG_FNAME_LEN 24
+
/*
* First LSN to use for "fake" LSNs.
*
--
2.46.0
