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: Heikki Linnakangas <hlinnaka@iki.fi>, Maxim Orlov <orlovmg@gmail.com>, Jacob Champion <jchampion@timescale.com>, 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>, Alexander Korotkov <aekorotkov@gmail.com>
Date: 2023-07-05T13:45:49Z
Lists: pgsql-hackers

Attachments

Hi,

> Reviewing this now. I think it's almost ready to be committed.
>
> There's another big effort going on to move SLRUs to the regular buffer
> cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> after all.

Somehow I didn't pay too much attention to this effort, thanks. I will
familiarize myself with the patch. Intuitively I don't think that the
patchse should block each other.

> This patch makes the filenames always 12 characters wide (for SLRUs that
> opt-in to the new naming). That's actually not enough for the full range
> that a 64 bit page number can represent. Should we make it 16 characters
> now, to avoid repeating the same mistake we made earlier? Or make it
> more configurable, on a per-SLRU basis. One reason I don't want to just
> make it 16 characters is that WAL segment filenames are also 16 hex
> characters, which could cause confusion.

Good catch. I propose the following solution:

```
 SlruFileName(SlruCtl ctl, char *path, int64 segno)
 {
     if (ctl->long_segment_names)
-        return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
+        /*
+         * We could use 16 characters here but the disadvantage would be that
+         * the SLRU segments will be hard to distinguish from WAL segments.
+         *
+         * For this reason we use 15 characters. It is enough but also means
+         * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
+         */
+        return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
                         (long long) segno);
     else
         return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
```

PFE the corrected patchset v58.

--
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