Horton

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

Alexander Korotkov <aekorotkov@gmail.com>

View thread View on postgr.es
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, 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>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2023-11-06T14:00:57Z
Lists: pgsql-hackers 
On Mon, Nov 6, 2023 at 3:42 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
> > > PFE the corrected patchset v58.
> >
> > I'd like to revive this thread.
> >
> > This patchset is extracted from a larger patchset implementing 64-bit xids.  It converts page numbers in SLRUs into 64 bits.  The most SLRUs save the same file naming scheme, thus their on-disk representation remains the same.  However, the patch 0002 converts pg_notify to actually use a new naming scheme.  Therefore pg_notify can benefit from simplification and getting rid of wraparound.
> >
> > -#define TransactionIdToCTsPage(xid) \
> > -   ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> > +
> > +/*
> > + * Although we return an int64 the actual value can't currently exceeed 2**32.
> > + */
> > +static inline int64
> > +TransactionIdToCTsPage(TransactionId xid)
> > +{
> > +   return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> > +}
> >
> > Is there any reason we transform macro into a function?  If not, I propose to leave this as a macro.  BTW, there is a typo in a word "exceeed".
> If I remember right, the compiler will make equivalent code from
> inline functions and macros, and functions has an additional benefit:
> the compiler will report type mismatch if any. That was the only
> reason.

Then it's OK to leave it as an inline function.

> Also, I looked at v58-0001 and don't quite agree with mentioning the
> authors of the original 64-xid patch, from which the current patch is
> derived, as just "privious input" persons.

+1, for converting all "previous input" persons as additional authors.
That would be a pretty long list of authors though.

BTW, I'm a bit puzzled on who should be the first author now?

------
Regards,
Alexander Korotkov

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