Horton

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

Aleksander Alekseev <aleksander@timescale.com>

View thread View on postgr.es
From: Aleksander Alekseev <aleksander@timescale.com>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Noah Misch <noah@leadboat.com>, Alexander Korotkov <aekorotkov@gmail.com>, Maxim Orlov <orlovmg@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Japin Li <japinli@hotmail.com>, Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>, Pavel Borisov <pashkin.elfe@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2024-06-26T11:09:58Z
Lists: pgsql-hackers

Attachments

Hi Noah,

> This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> 32-bit integers" and left some expressions coercing SLRU page numbers to int.
> Two sources:
>
>  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
>  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 | less -p '.int. from .int64. may alter its value'
>
> (Not every match needs to change.)

I examined the new warnings introduced by 4ed8f09. Most of them seem
to be harmless, for instance:

```
slru.c:657:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  657 |         int                     rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

```
slru.c: In function ‘SlruReportIOError’:
slru.c:962:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  962 |         int                     rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

Interestingly the patch decreased the overall number of warnings.

I prepared the patch for clog.c. The rest of the warnings don't strike
me as something we should immediately act on unless we have a bug
report. Or perhaps there is a particular warning that worries you?

> > @@ -127,7 +127,15 @@ typedef struct SlruCtlData
> >        * the behavior of this callback has no functional implications.)  Use
> >        * SlruPagePrecedesUnitTests() in SLRUs meeting its criteria.
> >        */
> > -     bool            (*PagePrecedes) (int, int);
> > +     bool            (*PagePrecedes) (int64, int64);
> > +
> > +     /*
> > +      * If true, use long segment filenames formed from lower 48 bits of the
> > +      * segment number, e.g. pg_xact/000000001234. Otherwise, use short
> > +      * filenames formed from lower 16 bits of the segment number e.g.
> > +      * pg_xact/1234.
> > +      */
> > +     bool            long_segment_names;
>
> SlruFileName() makes 15-character (60-bit) file names.  Where does the 48-bit
> limit arise?  How does the SlruFileName() comment about a 24-bit limit for
> short names relate this comment's 16-bit limit?

Yes, this comment is wrong. Here is a fix.

[1]: https://www.postgresql.org/message-id/CAJ7c6TNMuKWUuMfh5KWgJJBoJGqPHYdZeN4t%2BLB6WdRLbDfVTw%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Commits

  1. Fix more holes with SLRU code in need of int64 for segment numbers

  2. Use more consistently int64 for page numbers in SLRU-related code

  3. Improve comments in slru.{c,h} about segment name format

  4. Reorder actions in ProcArrayApplyRecoveryInfo()

  5. Fix typo in 5a1dfde8334b

  6. Fix warning due non-standard inline declaration in 4ed8f0913bfdb5f355

  7. Add SLRU tests for 64-bit page case

  8. Make use FullTransactionId in 2PC filenames

  9. Use larger segment file names for pg_notify

  10. Index SLRUs by 64-bit integers rather than by 32-bit integers

  11. Keep track of transaction commit timestamps

  12. Handle 5-char filenames in SlruScanDirectory