Horton

Re: XID formatting and SLRU refactorings

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, Maxim Orlov <orlovmg@gmail.com>, Aleksander Alekseev <aleksander@timescale.com>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, Japin Li <japinli@hotmail.com>, Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2023-11-28T10:38:54Z
Lists: pgsql-hackers 
On Tue, 28 Nov 2023 at 14:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> On 28/11/2023 12:14, Pavel Borisov wrote:
> > On Tue, 28 Nov 2023 at 13:13, Heikki Linnakangas <hlinnaka@iki.fi>
> wrote:
> >>
> >> On 27/11/2023 01:43, Alexander Korotkov wrote:
> >>> v61 looks good to me.  I'm going to push it as long as there are no
> objections.
> >> This was discussed earlier, but is still present in v61:
> >>
> >>> +/*
> >>> + * An internal function used by SlruScanDirectory().
> >>> + *
> >>> + * Returns true if a file with a name of a given length may be a
> correct
> >>> + * SLRU segment.
> >>> + */
> >>> +static inline bool
> >>> +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
> >>> +{
> >>> +     if (ctl->long_segment_names)
> >>> +             return (len == 15); /* see SlruFileName() */
> >>> +     else
> >>> +             /*
> >>> +              * Commit 638cf09e76d allowed 5-character lengths. Later
> commit
> >>> +              * 73c986adde5 allowed 6-character length.
> >>> +              *
> >>> +              * XXX should we still consider such names to be valid?
> >>> +              */
> >>> +             return (len == 4 || len == 5 || len == 6);
> >>> +}
> >>> +
> >>
> >> I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6
> >> chars long. For pg_multixact/members, which introduced the 5-char case,
> >> I think we should always pad the filenames 5 characters, and for
> >> commit_ts which introduced the 6 char case, always pad to 6 characters.
> >>
> >> Instead of a "long_segment_names" boolean, how about an integer field,
> >> to specify the length.
> >>
> >> That means that we'll need pg_upgrade to copy pg_multixact/members files
> >> under the new names. That should be pretty straightforward.
> >
> > I think what's done in patch 0001 is just an extension of existing
> > logic and moving it into separate function.
>
> That's right. I'm arguing that now is a good time to clean it up.
>
> I won't insist if Alexander prefers to commit it as it is, though. But
> let's at least explain how this works in the comment, instead of the XXX.
>
I agree with you that would be good to add a comment instead of XXX and
commit.

Pavel

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