Checking MINIMUM_VERSION_FOR_WAL_SUMMARIES

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

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

On Fri, Feb 02, 2024 at 03:11:40PM +0300, Nazir Bilal Yavuz wrote:

Thanks for the update. The patch looks good to me.

Yes, you're right. We want the opposite to happen here. I've applied
the patch on HEAD.
--
Michael