Hi, Peter!
Thanks for your review!
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?
>
Initially, we've made SLRU pages to be int64, and reworked them into uint64
as per Andres Freund's proposal. It is not necessary for a 64xid patchset
though as maximum page number is at least several (>2) times less than the
maximum 64bit xid value. So we've returned them to be signed int64. I don't
see the reason why make SRLU unsigned for introducing 64bit xids.
> #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?
>
Of course, this should be simplified as %012llX and we will do this in
later stage (in 0006 patch in 64xid thread) as this should be done together
with CLOG pg_upgrade. So we've returned this to the initial state in 0001.
Thanks for the notion!
+ 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.
>
Yes, segno should be 64bits because even multiple of SLRU_PAGES_PER_SEGMENT
and CLOG_XACTS_PER_PAGE (and similar for commit_ts and mxact) is far less
than 2^32 and the overall length of clog/commit_ts/mxact is 64bit.
> - 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?
>
This should also be introduced later together with SlruFileName changes. So
we've removed this change from 0001. Later in 0006 we'll add this with
comments.
- SlruFileName(ctl, path, ftag->segno);
> + SlruFileName(ctl, path, (uint64) ftag->segno);
>
> Maybe ftag->segno should be changed to 64 bits as well? Not clear.
>
Same as previous.
> 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.
>
Same as previous.
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.
>
Same as previous.
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.
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.
Fixed, thanks.
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.
There were several other ways that have met opposition above in the thread.
I guess PRId64 will also be opposed as not portable enough. Personally, I
don't see much trouble when we cast the value to be printed to more wide
value and consider this the best choice of all that was discussed. We just
stick to a portable way of printing which was recommended by Tom and in
agreement with 1f8bc448680bf93a974cb5f5
In [1] we initially proposed a 64xid patch to be committed at once. But it
appeared that a patch of this complexity can not be reviewed at once. It
was proposed in [1] that we'd better cut it into separate threads and
commit by parts, some into v15, the other into v16. So we did. In view of
this, I can not accept that 0002 patch doesn't make v15 better. I consider
it is separate enough to be committed as a base to further 64xid parts.
Anyway, big thanks for the review, which is quite useful!
[1]
https://www.postgresql.org/message-id/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com
--
Best regards,
Pavel Borisov
Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>
