From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Maxim Orlov <orlovmg@gmail.com>
To: Japin Li <japinli@hotmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Ilya Anfimov <ilan@tzirechnoy.com>
Subject: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Date: Wed, 23 Mar 2022 23:33:20 +0100
Message-ID: <131006ee-d309-f77e-7d3e-db8e9d76aeef@enterprisedb.com> (raw)
In-Reply-To: <CACG=ezZF8o0jZdw-uVRf1hic5fs9R+xXvM-Yy59vRpxG==QeLQ@mail.gmail.com>
References: <CAJ7c6TPDOYBYrnCAeyndkBktO0WG2xSdYduTF0nxq+vfkmTF5Q@mail.gmail.com>
<CACG=ezbZcjsu3JASUKgN-cyFHpB_j2pLrQ62QA7neV5M=XKX4g@mail.gmail.com>
<CAJ7c6TNb3O2gnzww76PqS==ycUHm6Usg04=4SGHXW06394u4PA@mail.gmail.com>
<CAJ7c6TNrK9SU5hRC5XCxEcAEFX=hyTU_JWteHfK+S285s8ejsg@mail.gmail.com>
<CALT9ZEHGyrtuL7-PNdR6B9J+s_5NM24wPtxu5fb+XROPXNRJ8A@mail.gmail.com>
<CACG=ezaJQikHm=vViF41ta0SMr05nMc5Qh7Ma1LXYX_GteS_Lg@mail.gmail.com>
<7d25a313-3f98-b6c2-1aa6-36d194a41c99@enterprisedb.com>
<CAJ7c6TPocitQf71gBjRQUVgUMYNO4kfng5sJyEi8bpDMBRT3YQ@mail.gmail.com>
<CALT9ZEEqQw2ODGid4Uq0Z4As_37O9muSVv5MrPAusBZRgA0dOg@mail.gmail.com>
<CACG=ezYV3FM5i9ws2QLyF+rz5WHTqheL59VRsHGsgAwfx8gh4g@mail.gmail.com>
<CAJ7c6TNx3fbWsZw+Pqi4Pp8T90=5xPkm2xL+jO4CFAuj4hxJrw@mail.gmail.com>
<CACG=ezaA2J7M+xoA=7jM4SifAsdVt_p7XLRFQS-Tv=nQDJ2p_w@mail.gmail.com>
<MEYP282MB16691EBC0386973FCD862FAEB6189@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
<CACG=ezZF8o0jZdw-uVRf1hic5fs9R+xXvM-Yy59vRpxG==QeLQ@mail.gmail.com>
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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, orlovmg@gmail.com, japinli@hotmail.com, aleksander@timescale.com, pgsql-hackers@lists.postgresql.org, pashkin.elfe@gmail.com, aekorotkov@gmail.com, horikyota.ntt@gmail.com, ilan@tzirechnoy.com
Subject: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
In-Reply-To: <131006ee-d309-f77e-7d3e-db8e9d76aeef@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox