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.
