Re: Executing pg_createsubscriber with a non-compatible control file - Mailing list pgsql-hackers

On Thu, Oct 9, 2025 at 11:08 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Oct 09, 2025 at 11:22:47AM -0700, Masahiko Sawada wrote:
> > +1, sounds like a good idea to improve user experience. I think we can
> > use the API in pg_combinebackup.c too since it has a similar function,
> > read_pg_version_file().
>
> Please find attached what I am finishing with.  At the end, I have
> designed an API that can be reused across the board for the following
> tools that do the same things in the tree, removing some duplicated
> code:
> - pg_resetwal
> - pg_upgrade
> - pg_combinebackup
>
> The routine that retrieves the contents gets a uint32 number, and it
> is optionally possible to get the contents of PG_VERSION (pg_upgrade
> wants that for tablespace paths, but that's really for pg_resetwal to
> be able to show back buggy data):
> extern uint32 get_pg_version(const char *datadir, char **version_str);
>
> This support both the pre-v10 and the post-v10 version formats, for
> pg_upgrade.
>
> To ease comparisons with PG_MAJORVERSION_NUM, I have added a small
> helper macro (see GET_PG_MAJORVERSION_NUM).
>
> I have also applied the same method to pg_createsubscriber, on top of
> that, to take care of my original issue.  I have not looked at other
> places where the same concept could be applied, at least it's a start.
>
> Thoughts or comments?

Thank you for creating the patches!

v1-0001-Introduce-API-able-to-retrieve-contents-of-PG_VER.patch:

LGTM

v1-0002-pg_upgrade-Use-PG_VERSION-generic-routine.patch:

LGTM

v1-0003-pg_createsubscriber-Use-PG_VERSION-generic-routin.patch:

+   major_version = GET_PG_MAJORVERSION_NUM(get_pg_version(datadir, NULL));
+   if (major_version != PG_MAJORVERSION_NUM)
    {
-       pg_fatal("directory \"%s\" is not a database cluster directory",
-                datadir);
+       pg_log_error("data directory is of wrong version");
+       pg_log_error_detail("File \"%s\" contains \"%u\", which is not
compatible with this program's version \"%u\".",
+                           "PG_VERSION", major_version, PG_MAJORVERSION_NUM);
+       exit(1);
    }

The new log detail message uses the same message as what pg_resetwal
uses, but pg_createsubscriber shows an integer major version whereas
pg_resetwal shows the raw version string. I guess it's better to unify
the usage for better consistency.

v1-0004-pg_combinebackup-use-PG_VERSION-generic-routine.patch:

+   pg_log_debug("read server version %u from file \"%s\"",
+                GET_PG_MAJORVERSION_NUM(version), "PG_VERSION");

We used to show the full path of PG_VERSION file in the debug message.
While probably we can live with such a small difference, do you think
it's a good idea to move the debug message to get_pg_version()?

v1-0005-pg_resetwal-use-PG_VERSION-generic-routine.patch:

    /* Check that data directory matches our server version */
-   CheckDataVersion();
+   CheckDataVersion(DataDir);

With the patch, pg_resetwal fails to handle a relative path of PGDATA
as follows:

$ bin/pg_resetwal data
pg_resetwal: error: could not open version file "data/PG_VERSION": No
such file or directory

This is because pg_resetwal does chdir() to the given path of the data
directory before checking the version.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com