Re: 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