Date: Fri, 2 Feb 2024 17:41:56 +0900
From: Yugo NAGATA <nagata@sraoss.co.jp>
To: Artur Zakirov <zaartur@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Checking MINIMUM_VERSION_FOR_WAL_SUMMARIES
Message-Id: <20240202174156.0272031c68033037abfe4fdf@sraoss.co.jp>
In-Reply-To: <CAKNkYnzkkQ0gb_ZmLTY0r2-qV1q6imXgcCWxdA6UoA6yJkujGg@mail.gmail.com>
References: <CAKNkYnzkkQ0gb_ZmLTY0r2-qV1q6imXgcCWxdA6UoA6yJkujGg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20240202174156.0272031c68033037abfe4fdf%40sraoss.co.jp>
Precedence: bulk

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>