Hi, On Wed, Oct 08, 2025 at 09:22:48AM +0900, Michael Paquier wrote: > On Tue, Oct 07, 2025 at 11:51:45AM -0700, Masahiko Sawada wrote: > > Just to be clear, did you mean that pg_checksums and pg_rewind already > > do such checks? IIUC pg_checksums does CRC check for the control file, > > and if we execute v18-pg_checksums against v17-cluster we end up with > > a similar error but with a different log message like "pg_checksums: > > error: pg_control CRC value is incorrect". Those log messages are not > > helpful either. > > For the case of pg_checksums, we don't really have a constraint based > on the major version, currently. However, there is a PG_VERSION_NUM > hardcoded when syncing the data folder, so we are pretty much assuming > that it is the case already. > > A check based on PG_VERSION has more benefits in the long-term, IMO. > When the CRC check of the control file fails, it would be tempting to > use some of the contents read from the control file but that would > also mean that we could expose some corrupted values to the user, so > that would not be useful.
So it seems the decision was that pg_checksums does not need this major version check? I agree that it is not entirely obvious is does, but on the other hand, those "pg_control CRC value is incorrect" are not helpful either. But even if pg_controldata() manages to read the control file, pg_checksums then errors out with "cluster is not compatible with this version of pg_checksums" (without further information) in case the control file version is different. I tried a few combinations, and current HEAD works with v18 clusters, while v16 pg_checksums can read v17 pg_control but then errors out due to that having a different version. All other combinations I tried so far yield the CRC error. So I think something like the attached should also be applied. A while ago I was actually looking at doing the exact same thing you did, i.e. extracting the PG_VERSION check out of pg_rewind for usage in pg_checksums but got side-tracked. While looking at the commit message of fa55be2a5, I noticed "This puts pg_createsubscriber in line with the documentation" and now I wonder whether the current major version dependency of pg_checksums shouldn't be mentioned in the documentation as well? If (that is a tall order maybe) on the other hand we agree that pg_checksums should work across major versions, then maybe the proper fix (on top of making sure everything else works correctly for each version) would be to teach get_controlfile() how to read older control files somehow, and pass it a major version as argument? Not sure that wouldn't be a hazard/ maintenance-nightmare and I don't volunteer to do that. Michael
>From 8b9f720854c8c4f9be7fb0e7ff186ddf1cfdcbf1 Mon Sep 17 00:00:00 2001 From: Michael Banck <[email protected]> Date: Fri, 17 Oct 2025 10:22:20 +0200 Subject: [PATCH] pg_checksums: Use new routine to retrieve data of PG_VERSION pg_checksums currently refuses to work with clusters of a different version. Until this limitation is lifted, running it on a cluster with a different major version errors out either with a "pg_control CRC value is incorrect" message, or, if that works, with a "cluster is not compatible with this version of pg_checksums" message in case the control file is different. The former is confusing as the control file is correct: only the expected version does not match. This commit integrates pg_checksums with the facility added by cd0be131ba6f, where the contents of PG_VERSION are read and compared with the value of PG_MAJORVERSION_NUM expected by the tool. --- src/bin/pg_checksums/pg_checksums.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index f20be82862a..46cb2f36efa 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -25,6 +25,7 @@ #include "common/logging.h" #include "common/relpath.h" #include "fe_utils/option_utils.h" +#include "fe_utils/version.h" #include "getopt_long.h" #include "pg_getopt.h" #include "storage/bufpage.h" @@ -448,6 +449,8 @@ main(int argc, char *argv[]) int c; int option_index; bool crc_ok; + uint32 major_version; + char *version_str; pg_logging_init(argv[0]); set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_checksums")); @@ -543,6 +546,20 @@ main(int argc, char *argv[]) exit(1); } + /* + * 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_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); + } + /* Read the control file and check compatibility */ ControlFile = get_controlfile(DataDir, &crc_ok); if (!crc_ok) -- 2.39.5
