Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

Started by Heikki Linnakangas2 months ago13 messages
#1Heikki Linnakangas
hlinnaka@iki.fi
1 attachment(s)

At the "make mxidoff 64 bits" thread [1]/messages/by-id/CACG=ezbZo_3_fnx=S5BfepwRftzrpJ+7WET4EkTU6wnjDTsnjg@mail.gmail.com, we're discussing moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h, to make it accessible from
pg_upgrade code. It's currently defined in slru.h, which cannot be
included in frontend code. There are many ways we could fix that, but
moving SLRU_PAGES_PER_SEGMENT to pg_config_manual.h seems best to me.
Patch attached.

There was a suggestion earlier to make SLRU_PAGES_PER_SEGMENT a
configure option [2]/messages/by-id/CAJDiXgiSVjsMj7pCKrXjwoVb2UCo28Fifd2VndNgybfbAhjbpg@mail.gmail.com. I don't want to go that far; pg_config_manual.h
seems like the right level of configurability to me.

I'm raising this in a new thread for visibility, in case someone who
hasn't been following the other threads have objections or better
suggestions.

The patch does not move over this comment from slru.h:

- * Note: because TransactionIds are 32 bits and wrap around at 0xFFFFFFFF,
- * page numbering also wraps around at 0xFFFFFFFF/xxxx_XACTS_PER_PAGE (where
- * xxxx is CLOG or SUBTRANS, respectively), and segment numbering at
- * 0xFFFFFFFF/xxxx_XACTS_PER_PAGE/SLRU_PAGES_PER_SEGMENT. We need
- * take no explicit notice of that fact in slru.c, except when comparing
- * segment and page numbers in SimpleLruTruncate (see PagePrecedes()).

I left it out because there's already a copy of the same comment next to
CLOG_XACTS_PER_PAGE and SUBTRANS_XACTS_PER_PAGE. That seems enough.

[1]: /messages/by-id/CACG=ezbZo_3_fnx=S5BfepwRftzrpJ+7WET4EkTU6wnjDTsnjg@mail.gmail.com
/messages/by-id/CACG=ezbZo_3_fnx=S5BfepwRftzrpJ+7WET4EkTU6wnjDTsnjg@mail.gmail.com

[2]: /messages/by-id/CAJDiXgiSVjsMj7pCKrXjwoVb2UCo28Fifd2VndNgybfbAhjbpg@mail.gmail.com
/messages/by-id/CAJDiXgiSVjsMj7pCKrXjwoVb2UCo28Fifd2VndNgybfbAhjbpg@mail.gmail.com

- Heikki

Attachments:

0001-Move-SLRU_PAGES_PER_SEGMENT-to-pg_config_manual.h.patchtext/x-patch; charset=UTF-8; name=0001-Move-SLRU_PAGES_PER_SEGMENT-to-pg_config_manual.h.patchDownload
From 0915d845a720c68252d61395c48ed908dd18dafd Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 10 Nov 2025 13:14:54 +0200
Subject: [PATCH 1/1] Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

It seems plausible that someone might want to experiment with
different values. The pressing reason though is that I'm reviewing a
patch that requires pg_upgrade to manipulate SLRU files. That patch
needs to access SLRU_PAGES_PER_SEGMENT from pg_upgrade code, and
slru.h, where SLRU_PAGES_PER_SEGMENT is currently defined, cannot be
included from frontend code. Moving it to pg_config_manual.h makes it
accessible.

Now that it's a little more likely that someone might change
SLRU_PAGES_PER_SEGMENT, add a cluster compatibility check for it.

Bump catalog version because of the new field in the control file.

Discussion: https://www.postgresql.org/message-id/CACG%3DezbZo_3_fnx%3DS5BfepwRftzrpJ%2B7WET4EkTU6wnjDTsnjg@mail.gmail.com
---
 src/backend/access/transam/xlog.c | 11 +++++++++++
 src/include/access/slru.h         | 15 ---------------
 src/include/catalog/catversion.h  |  3 ++-
 src/include/catalog/pg_control.h  |  2 ++
 src/include/pg_config_manual.h    | 10 ++++++++++
 5 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 101b616b028..22d0a2e8c3a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4271,6 +4271,7 @@ WriteControlFile(void)
 
 	ControlFile->blcksz = BLCKSZ;
 	ControlFile->relseg_size = RELSEG_SIZE;
+	ControlFile->slru_pages_per_segment = SLRU_PAGES_PER_SEGMENT;
 	ControlFile->xlog_blcksz = XLOG_BLCKSZ;
 	ControlFile->xlog_seg_size = wal_segment_size;
 
@@ -4490,6 +4491,16 @@ ReadControlFile(void)
 						   "RELSEG_SIZE", ControlFile->relseg_size,
 						   "RELSEG_SIZE", RELSEG_SIZE),
 				 errhint("It looks like you need to recompile or initdb.")));
+	if (ControlFile->slru_pages_per_segment != SLRU_PAGES_PER_SEGMENT)
+		ereport(FATAL,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("database files are incompatible with server"),
+		/* translator: %s is a variable name and %d is its value */
+				 errdetail("The database cluster was initialized with %s %d,"
+						   " but the server was compiled with %s %d.",
+						   "SLRU_PAGES_PER_SEGMENT", ControlFile->slru_pages_per_segment,
+						   "SLRU_PAGES_PER_SEGMENT", SLRU_PAGES_PER_SEGMENT),
+				 errhint("It looks like you need to recompile or initdb.")));
 	if (ControlFile->xlog_blcksz != XLOG_BLCKSZ)
 		ereport(FATAL,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index 8d57753ed01..8576649b15e 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -23,21 +23,6 @@
  */
 #define SLRU_MAX_ALLOWED_BUFFERS ((1024 * 1024 * 1024) / BLCKSZ)
 
-/*
- * Define SLRU segment size.  A page is the same BLCKSZ as is used everywhere
- * else in Postgres.  The segment size can be chosen somewhat arbitrarily;
- * we make it 32 pages by default, or 256Kb, i.e. 1M transactions for CLOG
- * or 64K transactions for SUBTRANS.
- *
- * Note: because TransactionIds are 32 bits and wrap around at 0xFFFFFFFF,
- * page numbering also wraps around at 0xFFFFFFFF/xxxx_XACTS_PER_PAGE (where
- * xxxx is CLOG or SUBTRANS, respectively), and segment numbering at
- * 0xFFFFFFFF/xxxx_XACTS_PER_PAGE/SLRU_PAGES_PER_SEGMENT.  We need
- * take no explicit notice of that fact in slru.c, except when comparing
- * segment and page numbers in SimpleLruTruncate (see PagePrecedes()).
- */
-#define SLRU_PAGES_PER_SEGMENT	32
-
 /*
  * Page status codes.  Note that these do not include the "dirty" bit.
  * page_dirty can be true only in the VALID or WRITE_IN_PROGRESS states;
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 60e7fd047d1..ef796602c53 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,7 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202511071
+/*  FIXME: bump it */
+#define CATALOG_VERSION_NO	999999999
 
 #endif
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index 63e834a6ce4..1ac0d5126fc 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -207,6 +207,8 @@ typedef struct ControlFileData
 	uint32		blcksz;			/* data block size for this DB */
 	uint32		relseg_size;	/* blocks per segment of large relation */
 
+	uint32		slru_pages_per_segment; /* size of each SLRU segment */
+
 	uint32		xlog_blcksz;	/* block size within WAL files */
 	uint32		xlog_seg_size;	/* size of each WAL segment */
 
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 7e1aa422332..c61a9faf870 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -19,6 +19,16 @@
  */
 #define DEFAULT_XLOG_SEG_SIZE	(16*1024*1024)
 
+/*
+ * SLRU segment size.  A page is the same BLCKSZ as is used everywhere else in
+ * Postgres.  The segment size can be chosen somewhat arbitrarily; we make it
+ * 32 pages by default, or 256Kb, i.e. 1M transactions for CLOG or 64K
+ * transactions for SUBTRANS.
+ *
+ * Changing this requires an initdb.
+ */
+#define SLRU_PAGES_PER_SEGMENT	32
+
 /*
  * Maximum length for identifiers (e.g. table names, column names,
  * function names).  Names actually are limited to one fewer byte than this,
-- 
2.47.3

#2Daniel Gustafsson
daniel@yesql.se
In reply to: Heikki Linnakangas (#1)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10 Nov 2025, at 12:29, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

There was a suggestion earlier to make SLRU_PAGES_PER_SEGMENT a configure option [2]. I don't want to go that far; pg_config_manual.h seems like the right level of configurability to me.

Agreed. The thread referenced above never really answered why a configure time
option would be needed.

+ uint32 slru_pages_per_segment; /* size of each SLRU segment */

Should this be expanded ever so slightly? A new reader of the code might
wonder about the relationship between "pages_per" and "size".

No objections (apart from the catversion =)) from reading the patch.

--
Daniel Gustafsson

#3Álvaro Herrera
alvherre@kurilemu.de
In reply to: Heikki Linnakangas (#1)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 2025-Nov-10, Heikki Linnakangas wrote:

At the "make mxidoff 64 bits" thread [1], we're discussing moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h, to make it accessible from
pg_upgrade code. It's currently defined in slru.h, which cannot be included
in frontend code. There are many ways we could fix that, but moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h seems best to me. Patch
attached.

Seems reasonable to me.

There was a suggestion earlier to make SLRU_PAGES_PER_SEGMENT a configure
option [2]. I don't want to go that far; pg_config_manual.h seems like the
right level of configurability to me.

I agree.

The patch does not move over this comment from slru.h:

- * Note: because TransactionIds are 32 bits and wrap around at 0xFFFFFFFF,
- * page numbering also wraps around at 0xFFFFFFFF/xxxx_XACTS_PER_PAGE (where
- * xxxx is CLOG or SUBTRANS, respectively), and segment numbering at
- * 0xFFFFFFFF/xxxx_XACTS_PER_PAGE/SLRU_PAGES_PER_SEGMENT. We need
- * take no explicit notice of that fact in slru.c, except when comparing
- * segment and page numbers in SimpleLruTruncate (see PagePrecedes()).

I left it out because there's already a copy of the same comment next to
CLOG_XACTS_PER_PAGE and SUBTRANS_XACTS_PER_PAGE. That seems enough.

I agree -- this comment would be out of place in pg_config_manual.h.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this." (Fotis)
/messages/by-id/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

#4Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Daniel Gustafsson (#2)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 13:41, Daniel Gustafsson wrote:

+ uint32 slru_pages_per_segment; /* size of each SLRU segment */

Should this be expanded ever so slightly? A new reader of the code might
wonder about the relationship between "pages_per" and "size".

Hmm, there's not much space for further explanations on that line. We
could add a longer multi-line comment but I'd rather keep it short and
consistent with the other similar fields around it. I hope that readers
who want more information will find the SLRU_PAGES_PER_SEGMENT
definition and the comments there.

I did consider renaming the field to 'slru_seg_size', to rhyme with
'relseg_size' and 'xlog_seg_size'. But then it wouldn't match the name
of SLRU_PAGES_PER_SEGMENT anymore. We could rename
SLRU_PAGES_PER_SEGMENT too, but I'm not sure it's worth the code churn,
and IMO "pages per segment" is better than "segment size" anyway because
it tells you what the unit is.

No objections (apart from the catversion =)) from reading the patch.

Thanks for the review!

- Heikki

#5Daniel Gustafsson
daniel@yesql.se
In reply to: Heikki Linnakangas (#4)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10 Nov 2025, at 13:18, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Hmm, there's not much space for further explanations on that line. We could add a longer multi-line comment but I'd rather keep it short and consistent with the other similar fields around it. I hope that readers who want more information will find the SLRU_PAGES_PER_SEGMENT definition and the comments there.

Fair enough.

I did consider renaming the field to 'slru_seg_size', to rhyme with 'relseg_size' and 'xlog_seg_size'. But then it wouldn't match the name of SLRU_PAGES_PER_SEGMENT anymore. We could rename SLRU_PAGES_PER_SEGMENT too, but I'm not sure it's worth the code churn, and IMO "pages per segment" is better than "segment size" anyway because it tells you what the unit is.

Agreed, renaming would be a net negative overall I think.

--
Daniel Gustafsson

#6Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Álvaro Herrera (#3)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 14:13, Álvaro Herrera wrote:

Seems reasonable to me.

Committed. Thanks for the quick review!

- Heikki

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#6)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Committed. Thanks for the quick review!

I think the number this should have bumped is PG_CONTROL_VERSION
(thanks to the new field therein). Bumping CATALOG_VERSION_NO
seems quite beside the point.

regards, tom lane

#8Andres Freund
andres@anarazel.de
In reply to: Heikki Linnakangas (#1)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

Hi,

On 2025-11-10 13:29:08 +0200, Heikki Linnakangas wrote:

At the "make mxidoff 64 bits" thread [1], we're discussing moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h, to make it accessible from
pg_upgrade code. It's currently defined in slru.h, which cannot be included
in frontend code. There are many ways we could fix that, but moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h seems best to me. Patch
attached.

I'm not convinced that pg_config_manual.h is a good place - that suggests it
makes sense to change the value for some installations, which I don't see any
reason for. Wouldn't an slrudefs.h or such be more appropriate?

There was a suggestion earlier to make SLRU_PAGES_PER_SEGMENT a configure
option [2]. I don't want to go that far; pg_config_manual.h seems like the
right level of configurability to me.

-1 for both levels of configurability, I think that just makes things more
complicated without any real-world gain.

Greetings,

Andres Freund

#9Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Andres Freund (#8)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 18:11, Andres Freund wrote:

Hi,

On 2025-11-10 13:29:08 +0200, Heikki Linnakangas wrote:

At the "make mxidoff 64 bits" thread [1], we're discussing moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h, to make it accessible from
pg_upgrade code. It's currently defined in slru.h, which cannot be included
in frontend code. There are many ways we could fix that, but moving
SLRU_PAGES_PER_SEGMENT to pg_config_manual.h seems best to me. Patch
attached.

I'm not convinced that pg_config_manual.h is a good place - that suggests it
makes sense to change the value for some installations, which I don't see any
reason for. Wouldn't an slrudefs.h or such be more appropriate?

The comment in pg_config_manual.h says about settings defined there:

In all cases, changing them is only useful in very rare situations
or for developers.

That seems fitting for SLRU_PAGES_PER_SEGMENT. I don't feel strongly
either way though, I'm happy to revert and move to slrudefs.h if that's
the consensus.

I think the control file compatibility check is nice to have in any case
and we should keep that.

- Heikki

#10Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Tom Lane (#7)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 17:16, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Committed. Thanks for the quick review!

I think the number this should have bumped is PG_CONTROL_VERSION
(thanks to the new field therein). Bumping CATALOG_VERSION_NO
seems quite beside the point.

Ah thanks, I forgot we have that as a separate version number. I'll go
bump that now. (I will not try to revert the CATALOG_VERSION_NO change,
that would be very confusing.)

- Heikki

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#10)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 10/11/2025 17:16, Tom Lane wrote:

I think the number this should have bumped is PG_CONTROL_VERSION
(thanks to the new field therein). Bumping CATALOG_VERSION_NO
seems quite beside the point.

Ah thanks, I forgot we have that as a separate version number. I'll go
bump that now. (I will not try to revert the CATALOG_VERSION_NO change,
that would be very confusing.)

Agreed, undoing it would accomplish nothing good.

regards, tom lane

#12Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Heikki Linnakangas (#10)
2 attachment(s)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 18:38, Heikki Linnakangas wrote:

On 10/11/2025 17:16, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Committed. Thanks for the quick review!

I think the number this should have bumped is PG_CONTROL_VERSION
(thanks to the new field therein).  Bumping CATALOG_VERSION_NO
seems quite beside the point.

Ah thanks, I forgot we have that as a separate version number. I'll go
bump that now. (I will not try to revert the CATALOG_VERSION_NO change,
that would be very confusing.)

Fixed. And I just noticed another thing I forgot: pg_resetwal and
pg_controldata.

While testing, I noticed that pg_controldata doesn't check
PG_CONTROL_VERSION. If you add a field to ControlFileData that changes
the length, you'll get a warning that the CRC doesn't match:

pg_controldata: warning: calculated CRC checksum does not match value stored in control file
pg_controldata: detail: Either the control file is corrupt, or it has a different layout than this program is expecting. The results below are untrustworthy.

but if you make any changes that *don't* change ControlFileData's size,
pg_controldata will merrily try to interpret the values with no warning.
Surely it should also check PG_CONTROL_VERSION?

- Heikki

Attachments:

0001-Add-missing-pg_resetwal-and-pg_control-data-support-.patchtext/x-patch; charset=UTF-8; name=0001-Add-missing-pg_resetwal-and-pg_control-data-support-.patchDownload
From bcfc03715fd2d6b1b899ee66a3707409f9ced6c8 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 10 Nov 2025 19:44:41 +0200
Subject: [PATCH 1/2] Add missing pg_resetwal and pg_control data support for
 new field

I forgot these in commit 3e0ae46d90
---
 src/bin/pg_controldata/pg_controldata.c | 2 ++
 src/bin/pg_resetwal/pg_resetwal.c       | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 10de058ce91..5c77f40313b 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -317,6 +317,8 @@ main(int argc, char *argv[])
 		   ControlFile->blcksz);
 	printf(_("Blocks per segment of large relation: %u\n"),
 		   ControlFile->relseg_size);
+	printf(_("Pages per SLRU segment:               %u\n"),
+		   ControlFile->slru_pages_per_segment);
 	printf(_("WAL block size:                       %u\n"),
 		   ControlFile->xlog_blcksz);
 	printf(_("Bytes per WAL segment:                %u\n"),
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index a89d72fc5cf..69a7cf0416d 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -697,6 +697,7 @@ GuessControlValues(void)
 	ControlFile.floatFormat = FLOATFORMAT_VALUE;
 	ControlFile.blcksz = BLCKSZ;
 	ControlFile.relseg_size = RELSEG_SIZE;
+	ControlFile.slru_pages_per_segment = SLRU_PAGES_PER_SEGMENT;
 	ControlFile.xlog_blcksz = XLOG_BLCKSZ;
 	ControlFile.xlog_seg_size = DEFAULT_XLOG_SEG_SIZE;
 	ControlFile.nameDataLen = NAMEDATALEN;
@@ -766,6 +767,8 @@ PrintControlValues(bool guessed)
 		   ControlFile.blcksz);
 	printf(_("Blocks per segment of large relation: %u\n"),
 		   ControlFile.relseg_size);
+	printf(_("Pages per SLRU segment:               %u\n"),
+		   ControlFile.slru_pages_per_segment);
 	printf(_("WAL block size:                       %u\n"),
 		   ControlFile.xlog_blcksz);
 	printf(_("Bytes per WAL segment:                %u\n"),
-- 
2.47.3

0002-Add-warning-to-pg_controldata-on-PG_CONTROL_VERSION-.patchtext/x-patch; charset=UTF-8; name=0002-Add-warning-to-pg_controldata-on-PG_CONTROL_VERSION-.patchDownload
From 288af0403e003ff858e812238245380480c44e57 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 10 Nov 2025 19:45:21 +0200
Subject: [PATCH 2/2] Add warning to pg_controldata on PG_CONTROL_VERSION
 mismatch

---
 src/bin/pg_controldata/pg_controldata.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 5c77f40313b..79f78d5fec9 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -167,7 +167,12 @@ main(int argc, char *argv[])
 
 	/* get a copy of the control file */
 	ControlFile = get_controlfile(DataDir, &crc_ok);
-	if (!crc_ok)
+	if (ControlFile->pg_control_version != PG_CONTROL_VERSION)
+	{
+		pg_log_warning("control file's version does not match the version understood by this program");
+		pg_log_warning_detail("Either the control file is corrupt, or it has been created with a different version "
+							  "of PostgreSQL.  The results below are untrustworthy.");
+	} else if (!crc_ok)
 	{
 		pg_log_warning("calculated CRC checksum does not match value stored in control file");
 		pg_log_warning_detail("Either the control file is corrupt, or it has a different layout than this program "
-- 
2.47.3

#13Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Heikki Linnakangas (#12)
Re: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h

On 10/11/2025 19:46, Heikki Linnakangas wrote:

Fixed. And I just noticed another thing I forgot: pg_resetwal and
pg_controldata.

Fixed that.

While testing, I noticed that pg_controldata doesn't check
PG_CONTROL_VERSION. If you add a field to ControlFileData that changes
the length, you'll get a warning that the CRC doesn't match:

pg_controldata: warning: calculated CRC checksum does not match value
stored in control file
pg_controldata: detail: Either the control file is corrupt, or it has
a different layout than this program is expecting.  The results below
are untrustworthy.

but if you make any changes that *don't* change ControlFileData's size,
pg_controldata will merrily try to interpret the values with no warning.
Surely it should also check PG_CONTROL_VERSION?

Committed an additional version check to pg_controldata. It now gives a
a more explicit warning than just checksum failure if the version in the
control file doesn't match the PG_CONTROL_VERSION that the binary was
built with:

~/git-sandbox-pgsql/master$ ./src/bin/pg_controldata/pg_controldata -D ~/pgsql.18stable/data
pg_controldata: warning: control file version (1800) does not match the version understood by this program (1900)
pg_controldata: detail: Either the control file has been created with a different version of PostgreSQL, or it is corrupt. The results below are untrustworthy.
pg_controldata: warning: invalid WAL segment size in control file (64 bytes)
pg_controldata: detail: The WAL segment size must be a power of two between 1 MB and 1 GB.
pg_controldata: detail: The file is corrupt and the results below are untrustworthy.
pg_control version number: 1800
Catalog version number: 202506291
Database system identifier: 7571514922284774749
Database cluster state: shut down
pg_control last modified: Tue 11 Nov 2025 19:04:53 EET
...

- Heikki