On Mon, Oct 13, 2025 at 03:35:06PM -0700, Masahiko Sawada wrote: > v1-0001-Introduce-API-able-to-retrieve-contents-of-PG_VER.patch: > > v1-0002-pg_upgrade-Use-PG_VERSION-generic-routine.patch: > v1-0003-pg_createsubscriber-Use-PG_VERSION-generic-routin.patch:
Applied both of these.
> 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.
OK, done as suggested to limit the translation work.
> 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()?
I cannot get into doing that, leaving that up to the caller with the
error message they want. That's a minor point, I guess, I can see
both sides of the coin.
Switched this one to report the full path, like previously.
> 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.
Right. I've tested with absolute paths and forgot relative paths.
For this one, I would just use a ".", as the chdir is before the
version check. Or we could reverse the chdir() and the version check,
but there is no real benefit in doing so.
Updated patch set attached.
--
Michael
From 1446b30bea9786432dcd9029ce61958ae334e46b Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 14 Oct 2025 16:41:19 +0900 Subject: [PATCH v2 1/3] pg_createsubscriber: Use PG_VERSION generic routine --- src/bin/pg_basebackup/pg_createsubscriber.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index d29407413d96..9fa5caaf91d6 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -26,6 +26,7 @@ #include "fe_utils/recovery_gen.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" +#include "fe_utils/version.h" #include "getopt_long.h" #define DEFAULT_SUB_PORT "50432" @@ -407,7 +408,8 @@ static void check_data_directory(const char *datadir) { struct stat statbuf; - char versionfile[MAXPGPATH]; + uint32 major_version; + char *version_str; pg_log_info("checking if directory \"%s\" is a cluster data directory", datadir); @@ -420,11 +422,18 @@ check_data_directory(const char *datadir) pg_fatal("could not access directory \"%s\": %m", datadir); } - snprintf(versionfile, MAXPGPATH, "%s/PG_VERSION", datadir); - if (stat(versionfile, &statbuf) != 0 && errno == ENOENT) + /* + * Retrieve the contents of this cluster's PG_VERSION. We require + * compatibility with the same major version as the one this tool is + * compiled with. + */ + major_version = GET_PG_MAJORVERSION_NUM(get_pg_version(datadir, &version_str)); + 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 \"%s\", which is not compatible with this program's version \"%s\".", + "PG_VERSION", version_str, PG_MAJORVERSION); + exit(1); } } -- 2.51.0
From 7185c5d6eb3ce0afb22d7be197e6901f2cab55c4 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 14 Oct 2025 16:47:03 +0900 Subject: [PATCH v2 2/3] pg_combinebackup: use PG_VERSION generic routine --- src/bin/pg_combinebackup/pg_combinebackup.c | 65 +++------------------ 1 file changed, 9 insertions(+), 56 deletions(-) diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index a330c09d939d..3a3251272092 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -34,6 +34,7 @@ #include "common/relpath.h" #include "copy_file.h" #include "fe_utils/option_utils.h" +#include "fe_utils/version.h" #include "getopt_long.h" #include "lib/stringinfo.h" #include "load_manifest.h" @@ -117,7 +118,6 @@ static void process_directory_recursively(Oid tsoid, manifest_data **manifests, manifest_writer *mwriter, cb_options *opt); -static int read_pg_version_file(char *directory); static void remember_to_cleanup_directory(char *target_path, bool rmtopdir); static void reset_directory_cleanup_list(void); static cb_tablespace *scan_for_existing_tablespaces(char *pathname, @@ -153,7 +153,7 @@ main(int argc, char *argv[]) int c; int n_backups; int n_prior_backups; - int version; + uint32 version; uint64 system_identifier; char **prior_backup_dirs; cb_options opt; @@ -162,6 +162,7 @@ main(int argc, char *argv[]) StringInfo last_backup_label; manifest_data **manifests; manifest_writer *mwriter; + char *pgdata; pg_logging_init(argv[0]); progname = get_progname(argv[0]); @@ -271,7 +272,12 @@ main(int argc, char *argv[]) } /* Read the server version from the final backup. */ - version = read_pg_version_file(argv[argc - 1]); + pgdata = argv[argc - 1]; + version = get_pg_version(pgdata, NULL); + if (GET_PG_MAJORVERSION_NUM(version) < 10) + pg_fatal("server version too old"); + pg_log_debug("read server version %u from file \"%s/%s\"", + GET_PG_MAJORVERSION_NUM(version), pgdata, "PG_VERSION"); /* Sanity-check control files. */ n_backups = argc - optind; @@ -1155,59 +1161,6 @@ process_directory_recursively(Oid tsoid, closedir(dir); } -/* - * Read the version number from PG_VERSION and convert it to the usual server - * version number format. (e.g. If PG_VERSION contains "14\n" this function - * will return 140000) - */ -static int -read_pg_version_file(char *directory) -{ - char filename[MAXPGPATH]; - StringInfoData buf; - int fd; - int version; - char *ep; - - /* Construct pathname. */ - snprintf(filename, MAXPGPATH, "%s/PG_VERSION", directory); - - /* Open file. */ - if ((fd = open(filename, O_RDONLY, 0)) < 0) - pg_fatal("could not open file \"%s\": %m", filename); - - /* Read into memory. Length limit of 128 should be more than generous. */ - initStringInfo(&buf); - slurp_file(fd, filename, &buf, 128); - - /* Close the file. */ - if (close(fd) != 0) - pg_fatal("could not close file \"%s\": %m", filename); - - /* Convert to integer. */ - errno = 0; - version = strtoul(buf.data, &ep, 10); - if (errno != 0 || *ep != '\n') - { - /* - * Incremental backup is not relevant to very old server versions that - * used multi-part version number (e.g. 9.6, or 8.4). So if we see - * what looks like the beginning of such a version number, just bail - * out. - */ - if (version < 10 && *ep == '.') - pg_fatal("%s: server version too old", filename); - pg_fatal("%s: could not parse version number", filename); - } - - /* Debugging output. */ - pg_log_debug("read server version %d from file \"%s\"", version, filename); - - /* Release memory and return result. */ - pfree(buf.data); - return version * 10000; -} - /* * Add a directory to the list of output directories to clean up. */ -- 2.51.0
From 5ab7579870f9d2dee1b707c0914308823479f51c Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Tue, 14 Oct 2025 16:55:04 +0900 Subject: [PATCH v2 3/3] pg_resetwal: use PG_VERSION generic routine --- src/bin/pg_resetwal/pg_resetwal.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c index 7a4e4eb95706..a89d72fc5cfe 100644 --- a/src/bin/pg_resetwal/pg_resetwal.c +++ b/src/bin/pg_resetwal/pg_resetwal.c @@ -55,6 +55,7 @@ #include "common/restricted_token.h" #include "common/string.h" #include "fe_utils/option_utils.h" +#include "fe_utils/version.h" #include "getopt_long.h" #include "pg_getopt.h" #include "storage/large_object.h" @@ -539,35 +540,18 @@ main(int argc, char *argv[]) static void CheckDataVersion(void) { - const char *ver_file = "PG_VERSION"; - FILE *ver_fd; - char rawline[64]; + char *version_str; + uint32 version = get_pg_version(".", &version_str); - if ((ver_fd = fopen(ver_file, "r")) == NULL) - pg_fatal("could not open file \"%s\" for reading: %m", - ver_file); - - /* version number has to be the first line read */ - if (!fgets(rawline, sizeof(rawline), ver_fd)) - { - if (!ferror(ver_fd)) - pg_fatal("unexpected empty file \"%s\"", ver_file); - else - pg_fatal("could not read file \"%s\": %m", ver_file); - } - - /* strip trailing newline and carriage return */ - (void) pg_strip_crlf(rawline); - - if (strcmp(rawline, PG_MAJORVERSION) != 0) + if (GET_PG_MAJORVERSION_NUM(version) != PG_MAJORVERSION_NUM) { pg_log_error("data directory is of wrong version"); pg_log_error_detail("File \"%s\" contains \"%s\", which is not compatible with this program's version \"%s\".", - ver_file, rawline, PG_MAJORVERSION); + "PG_VERSION", + version_str, + PG_MAJORVERSION); exit(1); } - - fclose(ver_fd); } -- 2.51.0
signature.asc
Description: PGP signature
