Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) - Mailing list pgsql-hackers
| From | Pavel Borisov |
|---|---|
| Subject | Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) |
| Date | |
| Msg-id | CALT9ZEEf1uywYN+VaRuSwNMGE5=eFOy7ZTwtP2g+W9oJDszqQw@mail.gmail.com Whole thread Raw |
| In response to | Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) (Peter Eisentraut <peter.eisentraut@enterprisedb.com>) |
| Responses |
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
(Pavel Borisov <pashkin.elfe@gmail.com>)
|
| List | pgsql-hackers |
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!
pgsql-hackers by date: