Re: Use XLOG_CONTROL_FILE macro everywhere?
On 04.09.2024 11:09, Kyotaro Horiguchi wrote: Instead, I'd like to propose separating the file and path-related definitions from xlog_internal.h, as shown in the attached first patch. This change would allow some modules to include files without unnecessary details. The second file is your patch, adjusted based on the first patch. I’d appreciate hearing from others on whether they find the first patch worthwhile. If it’s not considered worthwhile, then I believe having postmaster include xlog_internal.h would be the best approach. I really liked the idea of extracting only the necessary and logically complete part from xlog_internal.h. But now the presence of macros related to the segment sizes in the xlogfilepaths.h seems does not correspond to its name. So i suggest further extract size-related definition and macros from xlogfilepaths.h to xlogfilesize.h. Here is a patch that tries to do this based on the your first patch. Would be glad to hear your opinion. With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres CompanyFrom 4b6a12f6eaac98ee22dd63d64431968357905d77 Mon Sep 17 00:00:00 2001 From: "Anton A. Melnikov" Date: Sat, 7 Sep 2024 10:18:51 +0300 Subject: [PATCH] Extract size-related macros from xlogfilepaths.h to xlogfilesize.h --- src/include/access/xlogfilepaths.h | 65 +-- src/include/access/xlogfilesize.h | 83 ++ 2 files changed, 85 insertions(+), 63 deletions(-) create mode 100644 src/include/access/xlogfilesize.h diff --git a/src/include/access/xlogfilepaths.h b/src/include/access/xlogfilepaths.h index cdde2ccae4..ed28cf975e 100644 --- a/src/include/access/xlogfilepaths.h +++ b/src/include/access/xlogfilepaths.h @@ -12,6 +12,7 @@ #define XLOG_FILEPATHS_H #include "access/xlogdefs.h" +#include "access/xlogfilesize.h" /* * The XLog directory and files (relative to $PGDATA) @@ -34,75 +35,13 @@ /* files to signal promotion to primary */ #define PROMOTE_SIGNAL_FILE "promote" -/* wal_segment_size can range from 1MB to 1GB */ -#define WalSegMinSize 1024 * 1024 -#define WalSegMaxSize 1024 * 1024 * 1024 -/* default number of min and max wal segments */ -#define DEFAULT_MIN_WAL_SEGS 5 -#define DEFAULT_MAX_WAL_SEGS 64 - - -/* - * These macros encapsulate knowledge about the exact layout of XLog file - * names, timeline history file names, and archive-status file names. - */ +/* Maximum length of XLog file name including possible suffix */ #define MAXFNAMELEN 64 /* Length of XLog file name */ #define XLOG_FNAME_LEN 24 -/* check that the given size is a valid wal_segment_size */ -#define IsPowerOf2(x) (x > 0 && ((x) & ((x)-1)) == 0) -#define IsValidWalSegSize(size) \ - (IsPowerOf2(size) && \ - ((size) >= WalSegMinSize && (size) <= WalSegMaxSize)) - -/* Number of segments in a logical XLOG file */ -#define XLogSegmentsPerXLogId(wal_segsz_bytes) \ - (UINT64CONST(0x1) / (wal_segsz_bytes)) - -/* - * Compute an XLogRecPtr from a segment number and offset. - */ -#define XLogSegNoOffsetToRecPtr(segno, offset, wal_segsz_bytes, dest) \ - (dest) = (segno) * (wal_segsz_bytes) + (offset) -/* - * Compute a segment number from an XLogRecPtr. - * - * For XLByteToSeg, do the computation at face value. For XLByteToPrevSeg, - * a boundary byte is taken to be in the previous segment. This is suitable - * for deciding which segment to write given a pointer to a record end, - * for example. - */ -#define XLByteToSeg(xlrp, logSegNo, wal_segsz_bytes) \ - logSegNo = (xlrp) / (wal_segsz_bytes) - -#define XLByteToPrevSeg(xlrp, logSegNo, wal_segsz_bytes) \ - logSegNo = ((xlrp) - 1) / (wal_segsz_bytes) - -/* Compute the in-segment offset from an XLogRecPtr. */ -#define XLogSegmentOffset(xlogptr, wal_segsz_bytes) \ - ((xlogptr) & ((wal_segsz_bytes) - 1)) - -/* - * Convert values of GUCs measured in megabytes to equiv. segment count. - * Rounds down. - */ -#define XLogMBVarToSegs(mbvar, wal_segsz_bytes) \ - ((mbvar) / ((wal_segsz_bytes) / (1024 * 1024))) - -/* - * Is an XLogRecPtr within a particular XLOG segment? - * - * For XLByteInSeg, do the computation at face value. For XLByteInPrevSeg, - * a boundary byte is taken to be in the previous segment. - */ -#define XLByteInSeg(xlrp, logSegNo, wal_segsz_bytes) \ - (((xlrp) / (wal_segsz_bytes)) == (logSegNo)) - -#define XLByteInPrevSeg(xlrp, logSegNo, wal_segsz_bytes) \ - xlrp) - 1) / (wal_segsz_bytes)) == (logSegNo)) /* * XLOG file name handling functions diff --git a/src/include/access/xlogfilesize.h b/src/include/access/xlogfilesize.h new file mode 100644 index 00..fef30e02a6 --- /dev/null +++ b/src/include/access/xlogfilesize.h @@ -0,0 +1,83 @@ +/* + * xlogfilesize.h + * + * Size definitions and handling macros for PostgreSQL write-ahead logs. + * + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + *
Re: Use XLOG_CONTROL_FILE macro everywhere?
* 1024)))
+
+/*
+ * Is an XLogRecPtr within a particular XLOG segment?
+ *
+ * For XLByteInSeg, do the computation at face value. For XLByteInPrevSeg,
+ * a boundary byte is taken to be in the previous segment.
+ */
+#define XLByteInSeg(xlrp, logSegNo, wal_segsz_bytes) \
+ (((xlrp) / (wal_segsz_bytes)) == (logSegNo))
+
+#define XLByteInPrevSeg(xlrp, logSegNo, wal_segsz_bytes) \
+ xlrp) - 1) / (wal_segsz_bytes)) == (logSegNo))
+
+/*
+ * XLOG file name handling functions
+ */
+static inline void
+StatusFilePath(char *path, const char *xlog, const char *suffix)
+{
+ snprintf(path, MAXPGPATH, XLOGDIR "/archive_status/%s%s", xlog, suffix);
+}
+
+static inline bool
+IsTLHistoryFileName(const char *fname)
+{
+ return (strlen(fname) == 8 + strlen(".history") &&
+ strspn(fname, "0123456789ABCDEF") == 8 &&
+ strcmp(fname + 8, ".history") == 0);
+}
+
+static inline void
+XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo *logSegNo, int wal_segsz_bytes)
+{
+ uint32 log;
+ uint32 seg;
+
+ sscanf(fname, "%08X%08X%08X", tli, &log, &seg);
+ *logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
+}
+
+static inline void
+XLogFilePath(char *path, TimeLineID tli, XLogSegNo logSegNo, int wal_segsz_bytes)
+{
+ snprintf(path, MAXPGPATH, XLOGDIR "/%08X%08X%08X", tli,
+ (uint32) (logSegNo / XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (logSegNo % XLogSegmentsPerXLogId(wal_segsz_bytes)));
+}
+
+/*
+ * Generate a WAL segment file name. Do not use this function in a helper
+ * function allocating the result generated.
+ */
+static inline void
+XLogFileName(char *fname, TimeLineID tli, XLogSegNo logSegNo, int wal_segsz_bytes)
+{
+ snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli,
+ (uint32) (logSegNo / XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (logSegNo % XLogSegmentsPerXLogId(wal_segsz_bytes)));
+}
+
+static inline void
+XLogFileNameById(char *fname, TimeLineID tli, uint32 log, uint32 seg)
+{
+ snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, log, seg);
+}
+
+static inline bool
+IsXLogFileName(const char *fname)
+{
+ return (strlen(fname) == XLOG_FNAME_LEN && \
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN);
+}
+
+/*
+ * XLOG segment with .partial suffix. Used by pg_receivewal and at end of
+ * archive recovery, when we want to archive a WAL segment but it might not
+ * be complete yet.
+ */
+static inline bool
+IsPartialXLogFileName(const char *fname)
+{
+ return (strlen(fname) == XLOG_FNAME_LEN + strlen(".partial") &&
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
+ strcmp(fname + XLOG_FNAME_LEN, ".partial") == 0);
+}
+
+static inline void
+TLHistoryFileName(char *fname, TimeLineID tli)
+{
+ snprintf(fname, MAXFNAMELEN, "%08X.history", tli);
+}
+
+static inline void
+TLHistoryFilePath(char *path, TimeLineID tli)
+{
+ snprintf(path, MAXPGPATH, XLOGDIR "/%08X.history", tli);
+}
+
+static inline void
+BackupHistoryFileName(char *fname, TimeLineID tli, XLogSegNo logSegNo, XLogRecPtr startpoint, int wal_segsz_bytes)
+{
+ snprintf(fname, MAXFNAMELEN, "%08X%08X%08X.%08X.backup", tli,
+ (uint32) (logSegNo / XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (logSegNo % XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (XLogSegmentOffset(startpoint, wal_segsz_bytes)));
+}
+
+static inline bool
+IsBackupHistoryFileName(const char *fname)
+{
+ return (strlen(fname) > XLOG_FNAME_LEN &&
+ strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
+ strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
+}
+
+static inline void
+BackupHistoryFilePath(char *path, TimeLineID tli, XLogSegNo logSegNo, XLogRecPtr startpoint, int wal_segsz_bytes)
+{
+ snprintf(path, MAXPGPATH, XLOGDIR "/%08X%08X%08X.%08X.backup", tli,
+ (uint32) (logSegNo / XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (logSegNo % XLogSegmentsPerXLogId(wal_segsz_bytes)),
+ (uint32) (XLogSegmentOffset((startpoint), wal_segsz_bytes)));
+}
+
+
+#endif /* XLOG_FILEPATHS_H */
--
2.43.5
>From 813c3ff42886ecff5d038ffd5de44db488143841 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov"
Date: Wed, 4 Sep 2024 16:23:23 +0900
Subject: [PATCH 2/2] 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 +-
9 files
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 03.09.2024 08:37, Kyotaro Horiguchi wrote:
At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson 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/
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 CompanyFrom 0b1435492e293ee5553a69c2b3c560b93addc82e Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov"
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
#include
+#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", prio
Re: Use XLOG_CONTROL_FILE macro everywhere?
At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson wrote in > Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE > consistently as per the original patch. > > A few comments on the patch though: > > - * reads the data from $PGDATA/global/pg_control > + * reads the data from $PGDATA/ > > I don't think this is an improvement, I'd leave that one as the filename > spelled out. > > - "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. 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. 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. 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. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Use XLOG_CONTROL_FILE macro everywhere?
> On 27 Apr 2024, at 11:12, Peter Eisentraut wrote: > > On 26.04.24 22:51, Tom Lane wrote: >> Robert Haas writes: >>> On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier wrote: Not sure that I would bother with a second one. But, well, why not if people want to rename it, as long as you keep compatibility. >>> I vote for just standardizing on XLOG_CONTROL_FILE. That name seems >>> sufficiently intuitive to me, and I'd rather have one identifier for >>> this than two. It's simpler that way. >> +1. Back when we did the great xlog-to-wal renaming, we explicitly >> agreed that we wouldn't change internal symbols referring to xlog. >> It might or might not be appropriate to revisit that decision, >> but I sure don't want to do it piecemeal, one symbol at a time. >> Also, if we did rename this one, the logical choice would be >> WAL_CONTROL_FILE not PG_CONTROL_FILE. > > My reasoning was mainly that I don't see pg_control as controlling just the > WAL. But I don't feel strongly about instigating a great renaming here or > something. Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE consistently as per the original patch. A few comments on the patch though: - * reads the data from $PGDATA/global/pg_control + * reads the data from $PGDATA/ I don't think this is an improvement, I'd leave that one as the filename spelled out. - "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. -- Daniel Gustafsson
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 26.04.24 22:51, Tom Lane wrote: Robert Haas writes: On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier wrote: Not sure that I would bother with a second one. But, well, why not if people want to rename it, as long as you keep compatibility. I vote for just standardizing on XLOG_CONTROL_FILE. That name seems sufficiently intuitive to me, and I'd rather have one identifier for this than two. It's simpler that way. +1. Back when we did the great xlog-to-wal renaming, we explicitly agreed that we wouldn't change internal symbols referring to xlog. It might or might not be appropriate to revisit that decision, but I sure don't want to do it piecemeal, one symbol at a time. Also, if we did rename this one, the logical choice would be WAL_CONTROL_FILE not PG_CONTROL_FILE. My reasoning was mainly that I don't see pg_control as controlling just the WAL. But I don't feel strongly about instigating a great renaming here or something.
Re: Use XLOG_CONTROL_FILE macro everywhere?
Robert Haas writes: > On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier wrote: >> Not sure that I would bother with a second one. But, well, why not if >> people want to rename it, as long as you keep compatibility. > I vote for just standardizing on XLOG_CONTROL_FILE. That name seems > sufficiently intuitive to me, and I'd rather have one identifier for > this than two. It's simpler that way. +1. Back when we did the great xlog-to-wal renaming, we explicitly agreed that we wouldn't change internal symbols referring to xlog. It might or might not be appropriate to revisit that decision, but I sure don't want to do it piecemeal, one symbol at a time. Also, if we did rename this one, the logical choice would be WAL_CONTROL_FILE not PG_CONTROL_FILE. regards, tom lane
Re: Use XLOG_CONTROL_FILE macro everywhere?
On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier wrote: > On Wed, Apr 24, 2024 at 12:32:46PM +0300, Anton A. Melnikov wrote: > > To ensure backward compatibility we can save the old macro like this: > > > > #define XLOG_CONTROL_FILE "global/pg_control" > > #define PG_CONTROL_FILE XLOG_CONTROL_FILE > > > > With the best wishes, > > Not sure that I would bother with a second one. But, well, why not if > people want to rename it, as long as you keep compatibility. I vote for just standardizing on XLOG_CONTROL_FILE. That name seems sufficiently intuitive to me, and I'd rather have one identifier for this than two. It's simpler that way. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Use XLOG_CONTROL_FILE macro everywhere?
On Wed, Apr 24, 2024 at 12:32:46PM +0300, Anton A. Melnikov wrote: > To ensure backward compatibility we can save the old macro like this: > > #define XLOG_CONTROL_FILE "global/pg_control" > #define PG_CONTROL_FILE XLOG_CONTROL_FILE > > With the best wishes, Not sure that I would bother with a second one. But, well, why not if people want to rename it, as long as you keep compatibility. -- Michael signature.asc Description: PGP signature
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 24.04.2024 12:19, Daniel Gustafsson wrote: On 24 Apr 2024, at 11:13, Anton A. Melnikov wrote: On 24.04.2024 12:02, Peter Eisentraut wrote: On 19.04.24 05:50, Anton A. Melnikov wrote: May be better use this macro everywhere in C code? I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control". Maybe, but inconsistent usage is also unintuitive. Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE? The PG_CONTROL_FILE_SIZE macro is already in the code. With the best regards, XLOG_CONTROL_FILE is close to two decades old so there might be extensions using it (though the risk might be slim), perhaps using offering it as as well as backwards-compatability is warranted should we introduce a new name? To ensure backward compatibility we can save the old macro like this: #define XLOG_CONTROL_FILE "global/pg_control" #define PG_CONTROL_FILE XLOG_CONTROL_FILE With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Use XLOG_CONTROL_FILE macro everywhere?
> On 24 Apr 2024, at 11:13, Anton A. Melnikov wrote: > > On 24.04.2024 12:02, Peter Eisentraut wrote: >> On 19.04.24 05:50, Anton A. Melnikov wrote: >>> >>> May be better use this macro everywhere in C code? >> I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy >> for "pg_control". Maybe, but inconsistent usage is also unintuitive. > Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE? > > The PG_CONTROL_FILE_SIZE macro is already in the code. > With the best regards, XLOG_CONTROL_FILE is close to two decades old so there might be extensions using it (though the risk might be slim), perhaps using offering it as as well as backwards-compatability is warranted should we introduce a new name? -- Daniel Gustafsson
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 24.04.2024 12:02, Peter Eisentraut wrote: On 19.04.24 05:50, Anton A. Melnikov wrote: May be better use this macro everywhere in C code? I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control". Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE? The PG_CONTROL_FILE_SIZE macro is already in the code. With the best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 19.04.24 05:50, Anton A. Melnikov wrote: There is a macro XLOG_CONTROL_FILE for control file name defined in access/xlog_internal.h And there are some places in code where this macro is used like here https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/bin/pg_resetwal/pg_resetwal.c#L588 or here https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/common/controldata_utils.c#L214 But there are some other places where the control file name is used as text string directly. May be better use this macro everywhere in C code? I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control".
Re: Use XLOG_CONTROL_FILE macro everywhere?
On 20.04.2024 09:36, Daniel Gustafsson wrote: Anton: please register this patch in the Open commitfest to ensure it's not forgotten about. Done. Daniel and Michael thank you! -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Use XLOG_CONTROL_FILE macro everywhere?
> On 20 Apr 2024, at 01:23, Michael Paquier wrote: > > On Fri, Apr 19, 2024 at 10:12:14AM +0200, Daniel Gustafsson wrote: >> Off the cuff that seems to make sense, it does make it easier to grep for >> uses >> of the control file. > > +1 for switching to the macro where we can. Still, I don't see a > point in rushing and would just switch that once v18 opens up for > business. Absolutely, this is not fixing a defect so it's v18 material. Anton: please register this patch in the Open commitfest to ensure it's not forgotten about. -- Daniel Gustafsson
Re: Use XLOG_CONTROL_FILE macro everywhere?
On Fri, Apr 19, 2024 at 10:12:14AM +0200, Daniel Gustafsson wrote: > Off the cuff that seems to make sense, it does make it easier to grep for uses > of the control file. +1 for switching to the macro where we can. Still, I don't see a point in rushing and would just switch that once v18 opens up for business. -- Michael signature.asc Description: PGP signature
Re: Use XLOG_CONTROL_FILE macro everywhere?
> On 19 Apr 2024, at 05:50, Anton A. Melnikov wrote: > May be better use this macro everywhere in C code? Off the cuff that seems to make sense, it does make it easier to grep for uses of the control file. -- Daniel Gustafsson
Use XLOG_CONTROL_FILE macro everywhere?
Hello!
There is a macro XLOG_CONTROL_FILE for control file name
defined in access/xlog_internal.h
And there are some places in code where this macro is used
like here
https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/bin/pg_resetwal/pg_resetwal.c#L588
or here
https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/common/controldata_utils.c#L214
But there are some other places where the control file
name is used as text string directly.
May be better use this macro everywhere in C code?
The patch attached tries to do this.
Would be glad if take a look on it.
With the best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres CompanyFrom 2692683142c98572114c32f696b55c6a642dd3e9 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov"
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 | 3 ++-
src/bin/pg_combinebackup/pg_combinebackup.c | 5 +++--
src/bin/pg_controldata/pg_controldata.c | 2 +-
src/bin/pg_rewind/filemap.c | 3 ++-
src/bin/pg_rewind/pg_rewind.c | 8
src/bin/pg_upgrade/controldata.c| 11 ++-
src/bin/pg_verifybackup/pg_verifybackup.c | 3 ++-
src/common/controldata_utils.c | 2 +-
9 files changed, 22 insertions(+), 17 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..02b36caea7 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -90,6 +90,7 @@
#endif
#include "access/xlog.h"
+#include "access/xlog_internal.h"
#include "access/xlogrecovery.h"
#include "common/file_perm.h"
#include "common/file_utils.h"
@@ -1489,7 +1490,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
#include
+#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..1fe6618e7f 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -1,7 +1,7 @@
/*
* pg_controldata
*
- * reads the data from $PGDATA/global/pg_control
+ * reads the data from $PGDATA/
*
* copyright (c) Oliver Elphick , 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
#include
+#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(
