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

On 23.03.22 10:51, Maxim Orlov wrote:
>     Thanks for updating the patchs. I have a little comment on 0002.
> 
>     +                                errmsg_internal("found xmax %llu" "
>     (infomask 0x%04x) not frozen, not multi, not normal",
>     +                                                               
>     (unsigned long long) xid, tuple->t_infomask)));
> 
> 
> Thanks for your review! Fixed.

About v25-0001-Use-unsigned-64-bit-numbering-of-SLRU-pages.patch:

-static bool CLOGPagePrecedes(int page1, int page2);
+static bool CLOGPagePrecedes(uint64 page1, uint64 page2);

You are changing the API from signed to unsigned.  Is this intentional? 
It is not explained anywhere.  Are we sure that nothing uses, for 
example, negative values as error markers?

  #define SlruFileName(ctl, path, seg) \
-       snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
+       snprintf(path, MAXPGPATH, "%s/%04X%08X", (ctl)->Dir, \
+                        (uint32) ((seg) >> 32), (uint32) ((seg) & 
(uint64)0xFFFFFFFF))

What's going on here?  Some explanation?  Why not use something like 
%llX or whatever you need?

+   uint64      segno = pageno / SLRU_PAGES_PER_SEGMENT;
+   uint64      rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
[etc.]

Not clear whether segno etc. need to be changed to 64 bits, or whether 
an increase of SLRU_PAGES_PER_SEGMENT should also be considered.

-       if ((len == 4 || len == 5 || len == 6) &&
+       if ((len == 12 || len == 13 || len == 14) &&

This was horrible before, but maybe we can take this opportunity now to 
add a comment?

-   SlruFileName(ctl, path, ftag->segno);
+   SlruFileName(ctl, path, (uint64) ftag->segno);

Maybe ftag->segno should be changed to 64 bits as well?  Not clear.

There is a code comment at the definition of SLRU_PAGES_PER_SEGMENT that 
has some detailed explanations of how the SLRU numbering, SLRU file 
names, and transaction IDs tie together.  This doesn't seem to apply 
anymore after this change.

The reference page of pg_resetwal contains various pieces of information 
of how to map SLRU files back to transaction IDs.  Please check if that 
is still all up to date.


About v25-0002-Use-64-bit-format-to-output-XIDs.patch:

I don't see the point of applying this now.  It doesn't make PG15 any 
better.  It's just a patch part of which we might need later. 
Especially the issue of how we are handwaving past the epoch-enabled 
output sites disturbs me.  At least those should be omitted from this 
patch, since this patch makes these more wrong, not more right for the 
future.

An alternative we might want to consider is that we use PRId64 as 
explained here: 
<https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html>. 
  We'd have to check whether we still support any non-GNU gettext 
implementations and to what extent they support this.  But I think it's 
something to think about if we are going to embark on a journey of much 
more int64 printf output.