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.
