Use XLogFromFileName() in pg_resetwal to parse position from WAL file
Hi,
It looks like there's an opportunity to replace explicit WAL file
parsing code with XLogFromFileName() in pg_resetwal.c. This was not
done then (in PG 10) because the XLogFromFileName() wasn't accepting
file size as an input parameter (see [1]https://github.com/postgres/postgres/blob/REL_10_STABLE/src/include/access/xlog_internal.h) and pg_resetwal needed to
use WAL file size from the controlfile. Thanks to the commit
fc49e24fa69a15efacd5b8958115ed9c43c48f9a which added the
wal_segsz_bytes parameter to XLogFromFileName().
I'm attaching a small patch herewith. This removes using extra
variables in pg_resetwal.c and a bit of duplicate code too (5 LOC).
Thoughts?
[1]: https://github.com/postgres/postgres/blob/REL_10_STABLE/src/include/access/xlog_internal.h
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Use-XLogFromFileName-in-pg_resetwal-to-parse-posi.patchapplication/octet-stream; name=v1-0001-Use-XLogFromFileName-in-pg_resetwal-to-parse-posi.patchDownload
From 42948508910266685093c74d9fe61c3c5857c5ec Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 4 Oct 2022 05:04:40 +0000
Subject: [PATCH v1] Use XLogFromFileName() in pg_resetwal to parse position
from file
Replace explicit WAL file parsing code with XLogFromFileName() in
pg_resetwal.c. This was not done then (in PG 10) because the
XLogFromFileName() wasn't accepting file size as an input parameter
and pg_resetwal needed to use WAL file size from the controlfile.
Thanks to the commit fc49e24fa69a15efacd5b8958115ed9c43c48f9a which
added the wal_segsz_bytes parameter to XLogFromFileName().
This removes using extra variables in pg_resetwal.c and a bit of
duplicate code too.
---
src/bin/pg_resetwal/pg_resetwal.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index d4772a2965..bad886ddf9 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -901,7 +901,6 @@ FindEndOfXLOG(void)
{
DIR *xldir;
struct dirent *xlde;
- uint64 segs_per_xlogid;
uint64 xlogbytepos;
/*
@@ -909,7 +908,6 @@ FindEndOfXLOG(void)
* old pg_control. Note that for the moment we are working with segment
* numbering according to the old xlog seg size.
*/
- segs_per_xlogid = (UINT64CONST(0x0000000100000000) / ControlFile.xlog_seg_size);
newXlogSegNo = ControlFile.checkPointCopy.redo / ControlFile.xlog_seg_size;
/*
@@ -926,18 +924,15 @@ FindEndOfXLOG(void)
if (IsXLogFileName(xlde->d_name) ||
IsPartialXLogFileName(xlde->d_name))
{
- unsigned int tli,
- log,
- seg;
+ unsigned int tli;
XLogSegNo segno;
/*
- * Note: We don't use XLogFromFileName here, because we want to
- * use the segment size from the control file, not the size the
- * pg_resetwal binary was compiled with
+ * We use segment size from the control file here, not the size the
+ * pg_resetwal binary was compiled with.
*/
- sscanf(xlde->d_name, "%08X%08X%08X", &tli, &log, &seg);
- segno = ((uint64) log) * segs_per_xlogid + seg;
+ XLogFromFileName(xlde->d_name, &tli, &segno,
+ ControlFile.xlog_seg_size);
/*
* Note: we take the max of all files found, regardless of their
--
2.34.1
At Tue, 4 Oct 2022 11:06:15 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
It looks like there's an opportunity to replace explicit WAL file
parsing code with XLogFromFileName() in pg_resetwal.c. This was not
done then (in PG 10) because the XLogFromFileName() wasn't accepting
file size as an input parameter (see [1]) and pg_resetwal needed to
use WAL file size from the controlfile. Thanks to the commit
fc49e24fa69a15efacd5b8958115ed9c43c48f9a which added the
wal_segsz_bytes parameter to XLogFromFileName().
Nice finding. I found a few '%08X%08X's but they don't seem to fit
similar fix.
I'm attaching a small patch herewith. This removes using extra
variables in pg_resetwal.c and a bit of duplicate code too (5 LOC).Thoughts?
- segs_per_xlogid = (UINT64CONST(0x0000000100000000) / ControlFile.xlog_seg_size);
newXlogSegNo = ControlFile.checkPointCopy.redo / ControlFile.xlog_seg_size;
Couldn't we use XLByteToSeg() here?
Other than that, it looks good to me.
[1] https://github.com/postgres/postgres/blob/REL_10_STABLE/src/include/access/xlog_internal.h
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, Oct 4, 2022 at 11:47 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
- segs_per_xlogid = (UINT64CONST(0x0000000100000000) / ControlFile.xlog_seg_size);
newXlogSegNo = ControlFile.checkPointCopy.redo / ControlFile.xlog_seg_size;Couldn't we use XLByteToSeg() here?
Yes, we could.
Other than that, it looks good to me.
Thanks. There are a few more assorted WAL file related things I found,
I will be sending all of them in this thread itself in a while.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
At Tue, 04 Oct 2022 15:17:06 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
Other than that, it looks good to me.
Sorry I have another comment.
- unsigned int tli, - log, - seg; + unsigned int tli; XLogSegNo segno;
TLI should be of type TimeLineID.
This is not directly related to this patch, pg_resetwal.c has the
following line..
static uint32 minXlogTli = 0;
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, Oct 04, 2022 at 03:17:06PM +0900, Kyotaro Horiguchi wrote:
Nice finding. I found a few '%08X%08X's but they don't seem to fit
similar fix.
Nice cleanup.
Couldn't we use XLByteToSeg() here?
Other than that, it looks good to me.
Yep. It looks that you're right here.
--
Michael
At Tue, 04 Oct 2022 15:23:48 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
This is not directly related to this patch, pg_resetwal.c has the
following line..static uint32 minXlogTli = 0;
I have found other three instances of this in xlog.c and
pg_receivewal.c. Do they worth fixing?
(pg_upgarade.c has "uint32 tli/logid/segno but I'm not sure they need
to be "fixed". At least the segno is not a XLogSegNo.)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, Oct 4, 2022 at 12:11 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
static uint32 minXlogTli = 0;
I have found other three instances of this in xlog.c and
pg_receivewal.c. Do they worth fixing?(pg_upgarade.c has "uint32 tli/logid/segno but I'm not sure they need
to be "fixed". At least the segno is not a XLogSegNo.)
There are quite a number of places where data types need to be fixed,
see XLogFileNameById() callers. They are all being parsed as uint32
and then used. I'm not sure if we want to fix all of them.
I think I found that we can fix/refactor few WAL file related things:
1. 0001 replaces explicit WAL file parsing code with
XLogFromFileName() and uses XLByteToSeg() in pg_resetwal.c. This was
not done then (in PG 10) because the XLogFromFileName() wasn't
accepting file size as an input parameter and pg_resetwal needed to
use WAL file size from the controlfile. Thanks to the commit
fc49e24fa69a15efacd5b8958115ed9c43c48f9a which added the
wal_segsz_bytes parameter to XLogFromFileName(). This removes using
extra variables in pg_resetwal.c and a bit of duplicate code too. It
also replaces the explicit code with the XLByteToSeg() macro.
2. 0002 replaces MAXPGPATH with MAXFNAMELEN for WAL file names.
MAXFNAMELEN (64 bytes) is typically meant to be used for all WAL file
names across the code base. Because the WAL file names in postgres
can't be bigger than 64 bytes, in fact, not more than XLOG_FNAME_LEN
(24 bytes) but there are suffixes, timeline history files etc. To
accommodate all of that MAXFNAMELEN is introduced. There are some
places in the code base that still use MAXPGPATH (1024 bytes) for WAL
file names which is an unnecessary wastage of stack memory. This makes
code consistent across and saves a bit of space.
3. 0003 replaces WAL file name calculation with XLogFileNameById() in
pg_upgrade/controldata.c to be consistent across the code base. Note
that this requires us to change the nextxlogfile size from hard-coded
25 bytes to MAXFNAMELEN (64 bytes).
I'm attaching the v2 patch set.
Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Use-WAL-file-related-inline-functions-macros-in-p.patchapplication/octet-stream; name=v2-0001-Use-WAL-file-related-inline-functions-macros-in-p.patchDownload
From cd7d7d01f73b47f61b08e202c02167f469a800b3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 4 Oct 2022 06:45:07 +0000
Subject: [PATCH v2] Use WAL file related inline functions/macros in
pg_resetwal
Replace explicit WAL file parsing code with XLogFromFileName() in
pg_resetwal.c. This was not done then (in PG 10) because the
XLogFromFileName() wasn't accepting file size as an input parameter
and pg_resetwal needed to use WAL file size from the controlfile.
Thanks to the commit fc49e24fa69a15efacd5b8958115ed9c43c48f9a which
added the wal_segsz_bytes parameter to XLogFromFileName(). This
removes using extra variables in pg_resetwal.c and a bit of
duplicate code too.
Also, use XLByteToSeg() macro instead of explicit calculation of
segment number.
---
src/bin/pg_resetwal/pg_resetwal.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index d4772a2965..aaa4840920 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -901,7 +901,6 @@ FindEndOfXLOG(void)
{
DIR *xldir;
struct dirent *xlde;
- uint64 segs_per_xlogid;
uint64 xlogbytepos;
/*
@@ -909,8 +908,8 @@ FindEndOfXLOG(void)
* old pg_control. Note that for the moment we are working with segment
* numbering according to the old xlog seg size.
*/
- segs_per_xlogid = (UINT64CONST(0x0000000100000000) / ControlFile.xlog_seg_size);
- newXlogSegNo = ControlFile.checkPointCopy.redo / ControlFile.xlog_seg_size;
+ XLByteToSeg(ControlFile.checkPointCopy.redo, newXlogSegNo,
+ ControlFile.xlog_seg_size);
/*
* Scan the pg_wal directory to find existing WAL segment files. We assume
@@ -926,18 +925,15 @@ FindEndOfXLOG(void)
if (IsXLogFileName(xlde->d_name) ||
IsPartialXLogFileName(xlde->d_name))
{
- unsigned int tli,
- log,
- seg;
+ unsigned int tli;
XLogSegNo segno;
/*
- * Note: We don't use XLogFromFileName here, because we want to
- * use the segment size from the control file, not the size the
- * pg_resetwal binary was compiled with
+ * We use segment size from the control file here, not the size the
+ * pg_resetwal binary was compiled with.
*/
- sscanf(xlde->d_name, "%08X%08X%08X", &tli, &log, &seg);
- segno = ((uint64) log) * segs_per_xlogid + seg;
+ XLogFromFileName(xlde->d_name, &tli, &segno,
+ ControlFile.xlog_seg_size);
/*
* Note: we take the max of all files found, regardless of their
--
2.34.1
v2-0002-Use-MAXFNAMELEN-for-WAL-file-names-instead-of-MAX.patchapplication/octet-stream; name=v2-0002-Use-MAXFNAMELEN-for-WAL-file-names-instead-of-MAX.patchDownload
From abe765e81382bc0e56619cc2058d48db5a21459b Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 4 Oct 2022 06:35:01 +0000
Subject: [PATCH v2] Use MAXFNAMELEN for WAL file names instead of MAXPGPATH
MAXFNAMELEN (64 bytes) is typically meant to be used for all WAL
file names across the code base. Because the WAL file names in
postgres can't be bigger than 64 bytes, in fact, not more than
XLOG_FNAME_LEN (24 bytes) but there are suffixes, timeline history
files etc. To accommodate all of that MAXFNAMELEN is introduced.
There are some places in the code base that still use MAXPGPATH
(1024 bytes) for WAL file names which is an unnecessary wastage of
stack memory.
Replace MAXPGPATH with MAXFNAMELEN for WAL file names for
consistency across the code base and a bit of space savings.
---
src/backend/access/transam/xlogarchive.c | 4 ++--
src/bin/pg_basebackup/receivelog.c | 2 +-
src/bin/pg_waldump/pg_waldump.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index e2b7176f2f..2d499bb665 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -57,7 +57,7 @@ RestoreArchivedFile(char *path, const char *xlogfname,
{
char xlogpath[MAXPGPATH];
char *xlogRestoreCmd;
- char lastRestartPointFname[MAXPGPATH];
+ char lastRestartPointFname[MAXFNAMELEN];
int rc;
struct stat stat_buf;
XLogSegNo restartSegNo;
@@ -292,7 +292,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
bool failOnSignal, uint32 wait_event_info)
{
char xlogRecoveryCmd[MAXPGPATH];
- char lastRestartPointFname[MAXPGPATH];
+ char lastRestartPointFname[MAXFNAMELEN];
char *dp;
char *endp;
const char *sp;
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 9c71323d70..3e1561e4dd 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -93,7 +93,7 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
char *fn;
ssize_t size;
XLogSegNo segno;
- char walfile_name[MAXPGPATH];
+ char walfile_name[MAXFNAMELEN];
XLByteToSeg(startpoint, segno, WalSegSz);
XLogFileName(walfile_name, stream->timeline, segno, WalSegSz);
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 9993378ca5..079b3c155d 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -296,7 +296,7 @@ WALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
TimeLineID *tli_p)
{
TimeLineID tli = *tli_p;
- char fname[MAXPGPATH];
+ char fname[MAXFNAMELEN];
int tries;
XLogFileName(fname, tli, nextSegNo, state->segcxt.ws_segsize);
@@ -367,7 +367,7 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
&errinfo))
{
WALOpenSegment *seg = &errinfo.wre_seg;
- char fname[MAXPGPATH];
+ char fname[MAXFNAMELEN];
XLogFileName(fname, seg->ws_tli, seg->ws_segno,
state->segcxt.ws_segsize);
--
2.34.1
v2-0003-Use-XLogFileNameById-in-pg_upgrade-controldata.c.patchapplication/octet-stream; name=v2-0003-Use-XLogFileNameById-in-pg_upgrade-controldata.c.patchDownload
From 7f1aeb726409b451faae61e20f9164b225bee535 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 4 Oct 2022 07:06:02 +0000
Subject: [PATCH v2] Use XLogFileNameById() in pg_upgrade/controldata.c
Use XLogFileNameById() in pg_upgrade/controldata.c instead of
explicit code to be consistent across the code base. Note that
this requires us to change the nextxlogfile size from hard-coded
25 bytes to MAXFNAMELEN (64 bytes).
---
src/bin/pg_upgrade/controldata.c | 4 ++--
src/bin/pg_upgrade/pg_upgrade.h | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 018cd310f7..b0b9da85d4 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -549,8 +549,8 @@ get_control_data(ClusterInfo *cluster, bool live_check)
{
if (got_tli && got_log_id && got_log_seg)
{
- snprintf(cluster->controldata.nextxlogfile, 25, "%08X%08X%08X",
- tli, logid, segno);
+ XLogFileNameById(cluster->controldata.nextxlogfile, tli, logid,
+ segno);
got_nextxlogfile = true;
}
}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 31589b0fdc..69fa7f4b64 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -10,6 +10,7 @@
#include <sys/stat.h>
#include <sys/time.h>
+#include "access/xlog_internal.h"
#include "common/relpath.h"
#include "libpq-fe.h"
@@ -198,7 +199,7 @@ typedef struct
{
uint32 ctrl_ver;
uint32 cat_ver;
- char nextxlogfile[25];
+ char nextxlogfile[MAXFNAMELEN];
uint32 chkpnt_nxtxid;
uint32 chkpnt_nxtepoch;
uint32 chkpnt_nxtoid;
--
2.34.1
At Tue, 4 Oct 2022 13:20:54 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
On Tue, Oct 4, 2022 at 12:11 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:static uint32 minXlogTli = 0;
I have found other three instances of this in xlog.c and
pg_receivewal.c. Do they worth fixing?(pg_upgarade.c has "uint32 tli/logid/segno but I'm not sure they need
to be "fixed". At least the segno is not a XLogSegNo.)There are quite a number of places where data types need to be fixed,
see XLogFileNameById() callers. They are all being parsed as uint32
and then used. I'm not sure if we want to fix all of them.I think I found that we can fix/refactor few WAL file related things:
1. 0001 replaces explicit WAL file parsing code with
XLogFromFileName() and uses XLByteToSeg() in pg_resetwal.c. This was
not done then (in PG 10) because the XLogFromFileName() wasn't
accepting file size as an input parameter and pg_resetwal needed to
use WAL file size from the controlfile. Thanks to the commit
fc49e24fa69a15efacd5b8958115ed9c43c48f9a which added the
wal_segsz_bytes parameter to XLogFromFileName(). This removes using
extra variables in pg_resetwal.c and a bit of duplicate code too. It
also replaces the explicit code with the XLByteToSeg() macro.
Looks good to me.
2. 0002 replaces MAXPGPATH with MAXFNAMELEN for WAL file names.
MAXFNAMELEN (64 bytes) is typically meant to be used for all WAL file
names across the code base. Because the WAL file names in postgres
can't be bigger than 64 bytes, in fact, not more than XLOG_FNAME_LEN
(24 bytes) but there are suffixes, timeline history files etc. To
accommodate all of that MAXFNAMELEN is introduced. There are some
places in the code base that still use MAXPGPATH (1024 bytes) for WAL
file names which is an unnecessary wastage of stack memory. This makes
code consistent across and saves a bit of space.
Looks reasonable, too. I don't find other instances of the same mistake.
3. 0003 replaces WAL file name calculation with XLogFileNameById() in
pg_upgrade/controldata.c to be consistent across the code base. Note
that this requires us to change the nextxlogfile size from hard-coded
25 bytes to MAXFNAMELEN (64 bytes).
I'm not sure I like this. In other places where XLogFileNameById() is
used, the buffer is known to store longer strings so MAXFNAMELEN is
reasonable. But we don't need to add useless 39 bytes here.
Therefore, even if I wanted to change it, I would replace it with
"XLOG_FNAME_LEN + 1".
I'm attaching the v2 patch set.
Thoughts?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, Oct 4, 2022 at 2:01 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
1. 0001 replaces explicit WAL file parsing code with
Looks good to me.
2. 0002 replaces MAXPGPATH with MAXFNAMELEN for WAL file names.
Looks reasonable, too. I don't find other instances of the same mistake.
Thanks for reviewing.
3. 0003 replaces WAL file name calculation with XLogFileNameById() in
pg_upgrade/controldata.c to be consistent across the code base. Note
that this requires us to change the nextxlogfile size from hard-coded
25 bytes to MAXFNAMELEN (64 bytes).I'm not sure I like this. In other places where XLogFileNameById() is
used, the buffer is known to store longer strings so MAXFNAMELEN is
reasonable. But we don't need to add useless 39 bytes here.
Therefore, even if I wanted to change it, I would replace it with
"XLOG_FNAME_LEN + 1".
I'm fine with doing either of these things. Let's hear from others.
I've added a CF entry - https://commitfest.postgresql.org/40/3927/
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Tue, Oct 04, 2022 at 06:24:18PM +0530, Bharath Rupireddy wrote:
I'm fine with doing either of these things. Let's hear from others.
I've added a CF entry - https://commitfest.postgresql.org/40/3927/
About 0002, I am not sure that it is worth bothering. Sure, this
wastes a few bytes, but I recall that there are quite a few places in
the code where we imply a WAL segment but append a full path to it,
and this creates a few bumps with back-patches.
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -10,6 +10,7 @@
#include <sys/stat.h>
#include <sys/time.h>+#include "access/xlog_internal.h"
#include "common/relpath.h"
#include "libpq-fe.h"
Well, xlog_internal.h includes a few backend-only definitions. I'd
rather have us untangle more the backend/frontend dependencies before
adding more includes of this type, even if I agree that nextxlogfile
would be better with the implied name size limit.
Saying that, 0001 is a nice catch, so applied it. I have switched the
two TLI variables to use TimeLineID, while touching the area.
--
Michael