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

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.