Checking MINIMUM_VERSION_FOR_WAL_SUMMARIES
Hi hackers,
during reading the source code of new incremental backup functionality
I noticed that the following condition can by unintentional:
/*
* For newer server versions, likewise create pg_wal/summaries
*/
if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
{
...
if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
errno != EEXIST)
pg_fatal("could not create directory \"%s\": %m", summarydir);
}
This is from src/bin/pg_basebackup/pg_basebackup.c.
Is the condition correct? Shouldn't it be ">=". Otherwise the function
will create "/summaries" only for older PostgreSQL versions.
I've attached a patch to fix it in case this is a typo.
--
Artur
Attachments:
v1-0001-Fix-checking-MINIMUM_VERSION_FOR_WAL_SUMMARIES.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-checking-MINIMUM_VERSION_FOR_WAL_SUMMARIES.patchDownload
From 24227fdcad1fbdb67e38537bc1d70f65d9bdab05 Mon Sep 17 00:00:00 2001
From: Artur Zakirov <zaartur@gmail.com>
Date: Fri, 2 Feb 2024 01:04:27 +0100
Subject: [PATCH v1] Fix checking MINIMUM_VERSION_FOR_WAL_SUMMARIES for
creating pg_wal/summaries directory
---
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 77489af518..62eba4a962 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -700,7 +700,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier,
/*
* For newer server versions, likewise create pg_wal/summaries
*/
- if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
+ if (PQserverVersion(conn) >= MINIMUM_VERSION_FOR_WAL_SUMMARIES)
{
char summarydir[MAXPGPATH];
--
2.40.1
Hi,
On Fri, 2 Feb 2024 at 03:11, Artur Zakirov <zaartur@gmail.com> wrote:
Hi hackers,
during reading the source code of new incremental backup functionality
I noticed that the following condition can by unintentional:/*
* For newer server versions, likewise create pg_wal/summaries
*/
if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
{
...if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
errno != EEXIST)
pg_fatal("could not create directory \"%s\": %m", summarydir);
}This is from src/bin/pg_basebackup/pg_basebackup.c.
Is the condition correct? Shouldn't it be ">=". Otherwise the function
will create "/summaries" only for older PostgreSQL versions.
You seem right, nice catch. Also, this change makes the check in
snprintf(summarydir, sizeof(summarydir), "%s/%s/summaries",
basedir,
PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
redundant. PQserverVersion(conn) will always be higher than
MINIMUM_VERSION_FOR_PG_WAL.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Fri, 2 Feb 2024 01:11:27 +0100
Artur Zakirov <zaartur@gmail.com> wrote:
Hi hackers,
during reading the source code of new incremental backup functionality
I noticed that the following condition can by unintentional:/*
* For newer server versions, likewise create pg_wal/summaries
*/
if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
{
...if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
errno != EEXIST)
pg_fatal("could not create directory \"%s\": %m", summarydir);
}This is from src/bin/pg_basebackup/pg_basebackup.c.
Is the condition correct? Shouldn't it be ">=". Otherwise the function
will create "/summaries" only for older PostgreSQL versions.I've attached a patch to fix it in case this is a typo.
I also think it should be ">="
Also, if so, I don't think the check of MINIMUM_VERSION_FOR_PG_WAL
in the block is required, because the directory name is always pg_wal
in the new versions.
Regards,
Yugo Nagata
--
Artur
--
Yugo NAGATA <nagata@sraoss.co.jp>
On Fri, 2 Feb 2024 at 09:41, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
You seem right, nice catch. Also, this change makes the check in
snprintf(summarydir, sizeof(summarydir), "%s/%s/summaries",
basedir,
PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");redundant. PQserverVersion(conn) will always be higher than
MINIMUM_VERSION_FOR_PG_WAL.
Thank you both for the comments. Indeed, that part now looks redundant.
I've attached a patch to remove checking MINIMUM_VERSION_FOR_PG_WAL.
--
Artur
Attachments:
v2-0001-Fix-checking-MINIMUM_VERSION_FOR_WAL_SUMMARIES-fo.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-checking-MINIMUM_VERSION_FOR_WAL_SUMMARIES-fo.patchDownload
From cc8e636ad47b9dcc8779934e58f351ca43067b05 Mon Sep 17 00:00:00 2001
From: Artur Zakirov <zaartur@gmail.com>
Date: Fri, 2 Feb 2024 10:06:42 +0100
Subject: [PATCH v2] Fix checking MINIMUM_VERSION_FOR_WAL_SUMMARIES for
creating pg_wal/summaries directory
---
src/bin/pg_basebackup/pg_basebackup.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 77489af518..3a9940097c 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -700,14 +700,12 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier,
/*
* For newer server versions, likewise create pg_wal/summaries
*/
- if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
+ if (PQserverVersion(conn) >= MINIMUM_VERSION_FOR_WAL_SUMMARIES)
{
char summarydir[MAXPGPATH];
snprintf(summarydir, sizeof(summarydir), "%s/%s/summaries",
- basedir,
- PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
- "pg_xlog" : "pg_wal");
+ basedir, "pg_wal");
if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
errno != EEXIST)
--
2.40.1
Hi,
On Fri, 2 Feb 2024 at 12:11, Artur Zakirov <zaartur@gmail.com> wrote:
On Fri, 2 Feb 2024 at 09:41, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
You seem right, nice catch. Also, this change makes the check in
snprintf(summarydir, sizeof(summarydir), "%s/%s/summaries",
basedir,
PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");redundant. PQserverVersion(conn) will always be higher than
MINIMUM_VERSION_FOR_PG_WAL.Thank you both for the comments. Indeed, that part now looks redundant.
I've attached a patch to remove checking MINIMUM_VERSION_FOR_PG_WAL.
Thanks for the update. The patch looks good to me.
--
Regards,
Nazir Bilal Yavuz
Microsoft
