From: Anton A. Melnikov <a.melnikov@postgrespro.ru>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: daniel@yesql.se
Cc: peter@eisentraut.org
Cc: tgl@sss.pgh.pa.us
Cc: robertmhaas@gmail.com
Cc: michael@paquier.xyz
Cc: pgsql-hackers@postgresql.org
Subject: Re: Use XLOG_CONTROL_FILE macro everywhere?
Date: Tue, 3 Sep 2024 12:02:26 +0300
Message-ID: <fc33823e-dbdf-4f31-b9aa-bf7f758019b9@postgrespro.ru> (raw)
In-Reply-To: <20240903.143756.562059416508617821.horikyota.ntt@gmail.com>
References: <30207.1714164677@sss.pgh.pa.us>
<769dcaa2-acb7-49e5-a0b8-f257b91d1aed@eisentraut.org>
<54B16BF2-6979-4BB0-BE39-118D32B1BC58@yesql.se>
<20240903.143756.562059416508617821.horikyota.ntt@gmail.com>
On 03.09.2024 08:37, Kyotaro Horiguchi wrote:
> At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson <daniel@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
Attachments:v2-0001-Use-XLOG_CONTROL_FILE-macro-everywhere-in-C-code.patch (text/x-patch, 9.1K) download
From 0b1435492e293ee5553a69c2b3c560b93addc82e Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <a.melnikov@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.cindex 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.cindex 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.cindex 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.cindex 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 <olly@lfix.co.uk>, 2001;
* license: BSD
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
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: pgsql-hackers@postgresql.org
Cc: a.melnikov@postgrespro.ru, horikyota.ntt@gmail.com, daniel@yesql.se, peter@eisentraut.org, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, michael@paquier.xyz
Subject: Re: Use XLOG_CONTROL_FILE macro everywhere?
In-Reply-To: <fc33823e-dbdf-4f31-b9aa-bf7f758019b9@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox