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

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

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

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

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

Seems reasonable to me.

Committed. Thanks for the quick review!

- Heikki

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

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

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

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

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

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

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