Обсуждение: Move SLRU_PAGES_PER_SEGMENT to pg_config_manual.h
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. 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'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] https://www.postgresql.org/message-id/CACG%3DezbZo_3_fnx%3DS5BfepwRftzrpJ%2B7WET4EkTU6wnjDTsnjg%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAJDiXgiSVjsMj7pCKrXjwoVb2UCo28Fifd2VndNgybfbAhjbpg%40mail.gmail.com - Heikki
Вложения
> 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.hseems 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) https://postgr.es/m/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 ratherkeep it short and consistent with the other similar fields around it. I hope that readers who want more informationwill 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'tmatch the name of SLRU_PAGES_PER_SEGMENT anymore. We could rename SLRU_PAGES_PER_SEGMENT too, but I'm not sure it'sworth the code churn, and IMO "pages per segment" is better than "segment size" anyway because it tells you what theunit 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