Horton

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

Peter Eisentraut <peter.eisentraut@enterprisedb.com>

View thread View on postgr.es
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Maxim Orlov <orlovmg@gmail.com>, Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, Japin Li <japinli@hotmail.com>, Alexander Korotkov <aekorotkov@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Ilya Anfimov <ilan@tzirechnoy.com>
Date: 2022-03-21T18:40:22Z
Lists: pgsql-hackers 
On 18.03.22 16:14, Maxim Orlov wrote:
> Here is v22. It addresses things mentioned by Tom and Kyotaro. Also 
> added Aleksander's changes from v21.

The v22-0002-Update-XID-formatting-in-the-.po-files.patch is not 
necessary.  That is done by a separate procedure.

I'm wondering about things like this:

- psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
-          xmax,
+ psprintf("xmax %llu equals or exceeds next valid transaction ID %u:%llu",
+          (unsigned long long) xmax,
            EpochFromFullTransactionId(ctx->next_fxid),
-          XidFromFullTransactionId(ctx->next_fxid)));
+          (unsigned long long) XidFromFullTransactionId(ctx->next_fxid)));

This %u:%u business is basically an existing workaround for the lack of 
64-bit transaction identifiers.  Presumably, when those are available, 
all of this will be replaced by a single number display, possibly a 
single %llu.  So these sites you change here will have to be touched 
again, and so changing this now doesn't make sense.  At least that's my 
guess.  Maybe there needs to be a fuller explanation of how this is 
meant to be transitioned.

As a more general point, I don't like plastering these bulky casts all 
over the place.  Casts hide problems.  For example, if we currently have

     elog(LOG, "xid is %u", xid);

and then xid is changed to a 64-bit type, this will give a compiler 
warning until you change the format.  If we add a (long long unsigned) 
cast here now and then somehow forget to change the type of xid, nothing 
will warn us about that.  (Note that there is also third-party code 
affected by this.)  Besides, these casts are quite ugly anyway, and I 
don't think the solution for all time should be that we have to add 
these casts just to print xids.

I think there needs to be a bit more soul searching here on how to 
handle that in the future and how to transition it.  I don't think 
targeting this patch for PG15 is useful at this point.

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