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

Hi, Aleksander!

On Thu, Mar 17, 2022 at 4:12 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:

This thread is a fork of [1], created per request by several people in
the discussion. It includes two patches from the patchset that we
believe can be delivered in PG15. The rest of the patches are
targeting PG >= 16 and can be discussed further in [1].

Thank you for putting this into a separate thread.

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.

I'm not a localization expert. Could you clarify what localization
messages should look like if we switch to XID_FMT? And will we have
to change them if change the definition of XID_FMT?

------
Regards,
Alexander Korotkov

On 17.03.22 14:12, Aleksander Alekseev wrote:

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.

This is not a question of simplification. Translatable messages with
embedded macros won't work. This patch isn't going to be acceptable.

On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 17.03.22 14:12, Aleksander Alekseev wrote:

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.

This is not a question of simplification. Translatable messages with
embedded macros won't work. This patch isn't going to be acceptable.

I've suspected this, but wasn't sure. Thank you for clarification.

------
Regards,
Alexander Korotkov

On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 17.03.22 14:12, Aleksander Alekseev wrote:

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.

This is not a question of simplification. Translatable messages with
embedded macros won't work. This patch isn't going to be acceptable.

I've suspected this, but wasn't sure. Thank you for clarification.

Maybe, we should format it to string before for localization messages,
like the following code snippet comes from pg_backup_tar.c.
However, I do not think it is a good way.

snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
fatal("actual file length (%s) does not match expected (%s)",
buf1, buf2);

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

Maybe, we should format it to string before for localization messages,
like the following code snippet comes from pg_backup_tar.c.
However, I do not think it is a good way.

snprintf(buf1, sizeof(buf1), INT64_FORMAT, (int64) len);
snprintf(buf2, sizeof(buf2), INT64_FORMAT, (int64) th->fileLen);
fatal("actual file length (%s) does not match expected (%s)",
buf1, buf2);

That used to be our standard practice before we switched to always
relying on our own snprintf.c. Now, we know that "%lld" with an
explicit cast to long long will work, so that's the preferred method
for printing 64-bit values in localizable strings. Not all of the old
code has been updated, though.

regards, tom lane

On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov <aekorotkov@gmail.com>
wrote:

On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 17.03.22 14:12, Aleksander Alekseev wrote:

v19-0001 changes the format string for XIDs from %u to XID_FMT. This
refactoring allows us to switch to UINT64_FORMAT by changing one
#define in the future patches.

Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
xid)` instead in order to simplify localization of the error messages.
Personally I don't have a strong opinion here. Either approach will
work and will affect the error messages eventually. Please let us know
what you think.

This is not a question of simplification. Translatable messages with
embedded macros won't work. This patch isn't going to be acceptable.

I've suspected this, but wasn't sure. Thank you for clarification.

Hi, hackers!

The need to support localization is very much understood by us. We'll
deliver a patchset soon with localization based on %lld/%llu format and
explicit casts to unsigned/signed long long.
Alexander Alexeev could you wait a little bit and give us time to deliver
v20 patch which will address localization (I propose concurrent work should
stop until that to avoid conflicts)
Advice and discussion help us a lot.

Thanks!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

Hi!

Here is the v20 patch. 0001 and 0002 are proposed into PG15 as
discussed above.
The whole set of patches is added into [1]/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com to be committed into PG16.

In this version we've made a major revision related to printf/elog format
compatible with localization
as was proposed above.

We also think of adding 0003 patch related to 64 bit GUC's into this
thread. Suppose it also may be delivered
into PG15.

Aleksander Alekseev, we've done this major revision mentioned above and you
are free to continue working on this patch set.

Reviews and proposals are very welcome!

[1]: /messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com
/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com

--
Best regards,
Maxim Orlov.

At Thu, 17 Mar 2022 19:25:00 +0300, Maxim Orlov <orlovmg@gmail.com> wrote in

Hi!

Here is the v20 patch. 0001 and 0002 are proposed into PG15 as
discussed above.
The whole set of patches is added into [1] to be committed into PG16.

In this version we've made a major revision related to printf/elog format
compatible with localization
as was proposed above.

We also think of adding 0003 patch related to 64 bit GUC's into this
thread. Suppose it also may be delivered
into PG15.

Aleksander Alekseev, we've done this major revision mentioned above and you
are free to continue working on this patch set.

Reviews and proposals are very welcome!

(I'm afraid that this thread is not for the discussion of the patch?:)

[1]
/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com

+/* printf/elog format compatible with 32 and 64 bit xid. */
+typedef unsigned long long		XID_TYPE;
...
+	 errmsg_internal("found multixact %llu from before relminmxid %llu",
+			 (XID_TYPE) multi, (XID_TYPE) relminmxid)));

"(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
point here is "%llu in format string accepts (long long)" so we should
use literally (or bare) (long long) and the maybe-all precedents does
that.

And.. You looks like applying the change to some inappropriate places?

-      elog(DEBUG2, "deleted page from block %u has safexid %u",
-         blkno, opaque->btpo_level);
+      elog(DEBUG2, "deleted page from block %u has safexid %llu",
+         blkno, (XID_TYPE) opaque->btpo_level);

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 18 Mar 2022 10:20:08 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

"(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
point here is "%llu in format string accepts (long long)" so we should

Of course it's the typo of
"%llu in format string accepts (*unsigned* long long)".

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

At Thu, 17 Mar 2022 19:25:00 +0300, Maxim Orlov <orlovmg@gmail.com> wrote in

+/* printf/elog format compatible with 32 and 64 bit xid. */
+typedef unsigned long long		XID_TYPE;
...
+	 errmsg_internal("found multixact %llu from before relminmxid %llu",
+			 (XID_TYPE) multi, (XID_TYPE) relminmxid)));

"(XID_TYPE) x" is actually equivalent to "(long long) x" here, but the
point here is "%llu in format string accepts (long long)" so we should
use literally (or bare) (long long) and the maybe-all precedents does
that.

Yes. Please do NOT do it like that. Write (long long), not something
else, to cast a value to match an "ll" format specifier. Otherwise
you're just making readers wonder whether your code is correct.

regards, tom lane

Hi hackers,

Aleksander Alekseev, we've done this major revision mentioned above and

you are free to continue working on this patch set.

Reviews and proposals are very welcome!

Many thanks!

Here is an new version with the following changes compared to v20:

- Commit messages and links to the discussions were updated;
- XID_TYPE name seemed to be slightly misleading. I changed it to
XID_FMT_TYPE. Not 100% sure if we really need this typedef though. If not,
XID_FMT_TYPE is easy to replace in the .patch files. Same for
XID32_SCANF_FMT definition;
- I noticed that pg_resetwal.c continues to use %u to format XIDs. Fixed;
- Since v20-0001 modifies gettext() arguments, I'm pretty sure the
corresponding .po files should be modified as well. I addressed this in a
separate patch in order to simplify the review;

To me personally v21 looks almost OK. The comments in c.h should be
rewritten depending on whether we choose to keep XID_FMT_TYPE and/or
XID32_SCANF_FMT. The patchset passes all the tests.

(As a side note, it looks like cfbot was slightly confused by forking the
thread and modifying the CF entry. It couldn't find v20. If somebody knows
how to fix this, please help.)

--
Best regards,
Aleksander Alekseev

Hi hackers,

Here is an new version with the following changes compared to v20:
[...]
To me personally v21 looks almost OK.

For some reason I didn't receive the recent e-mails from Tom and Kyotaro.
I've just discovered them accidentally by reading the thread through the
web interface. These comments were not addressed in v21.

--
Best regards,
Aleksander Alekseev

To me personally v21 looks almost OK.

For some reason I didn't receive the recent e-mails from Tom and Kyotaro.
I've just discovered them accidentally by reading the thread through the
web interface. These comments were not addressed in v21.

Aleksander, as of now we're preparing a new version that addresses a thing
mentioned by Tom&Kyotaro. We'll try to add what you've done in v21, but
please check. We're going to send a patch very soon, most probably today in
several hours.

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

Hi!

Here is v22. It addresses things mentioned by Tom and Kyotaro. Also added
Aleksander's changes from v21.

--
Best regards,
Maxim Orlov.

Hi,

On 2022-03-18 18:14:52 +0300, Maxim Orlov wrote:

Subject: [PATCH v22 3/6] Use 64-bit pages in SLRU

This is one step toward 64-bit XIDs.

I think this should explain in more detail why this move is done. Neither the
commit message nor the rest of the thread does so afaics. It's not too hard to
infer, but the central reason behind a patch shouldn't need to be inferred.

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

I think all of these are actually unsigned integers. If all of this stuff gets
touched, perhaps worth moving to uint64 instead?

/messages/by-id/20220318231430.m5g56yk4ztlz2man@alap3.anarazel.de

Greetings,

Andres Freund

On 2022-03-18 18:14:52 +0300, Maxim Orlov wrote:

Subject: [PATCH v22 3/6] Use 64-bit pages in SLRU

This is one step toward 64-bit XIDs.

I think this should explain in more detail why this move is done. Neither
the
commit message nor the rest of the thread does so afaics. It's not too
hard to
infer, but the central reason behind a patch shouldn't need to be inferred.

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

I think all of these are actually unsigned integers. If all of this stuff
gets
touched, perhaps worth moving to uint64 instead?

/messages/by-id/20220318231430.m5g56yk4ztlz2man@alap3.anarazel.de

We'll try to add these and many similar changes in Slru code, thanks!

On 18.03.22 16:14, Maxim Orlov wrote:

Here is v22. It addresses things mentioned by Tom and Kyotaro. Also
added Aleksander's changes from v21.

The v22-0002-Update-XID-formatting-in-the-.po-files.patch is not
necessary. That is done by a separate procedure.

I'm wondering about things like this:

- 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.

As a more general point, I don't like plastering these bulky casts all
over the place. Casts hide problems. For example, if we currently have

elog(LOG, "xid is %u", xid);

and then xid is changed to a 64-bit type, this will give a compiler
warning until you change the format. If we add a (long long unsigned)
cast here now and then somehow forget to change the type of xid, nothing
will warn us about that. (Note that there is also third-party code
affected by this.) Besides, these casts are quite ugly anyway, and I
don't think the solution for all time should be that we have to add
these casts just to print xids.

I think there needs to be a bit more soul searching here on how to
handle that in the future and how to transition it. I don't think
targeting this patch for PG15 is useful at this point.

Hi Peter,

I think there needs to be a bit more soul searching here on how to
handle that in the future and how to transition it. I don't think
targeting this patch for PG15 is useful at this point.

The patches can be reordered so that we are still able to deliver SLRU
refactoring in PG15.

As a more general point, I don't like plastering these bulky casts all
over the place. Casts hide problems.

Regarding the casts, I don't like them either. I agree that it could
be a good idea to invest a little more time into figuring out if this
transit can be handled in a better way and deliver this change in the
next CF. However, if no one will be able to suggest a better
alternative, I think we should continue making progress here. The
32-bit XIDs are a major inconvenience for many users.

--
Best regards,
Aleksander Alekseev

I think there needs to be a bit more soul searching here on how to
handle that in the future and how to transition it. I don't think
targeting this patch for PG15 is useful at this point.

The patches can be reordered so that we are still able to deliver SLRU
refactoring in PG15.

Sure.

As a more general point, I don't like plastering these bulky casts all
over the place. Casts hide problems.

Regarding the casts, I don't like them either. I agree that it could
be a good idea to invest a little more time into figuring out if this
transit can be handled in a better way and deliver this change in the
next CF. However, if no one will be able to suggest a better
alternative, I think we should continue making progress here. The
32-bit XIDs are a major inconvenience for many users.

I'd like to add that the initial way of shifting to 64bit was based on
XID_FMT in a print formatting template which could be changed from 32 to 64
bit when shifting to 64-bit xids later. But this template is not
localizable so hackers recommended using %lld/%llu with (long
long)/(unsigned long long cast) which is a current best practice elsewhere
in the code (e.g. recent 1f8bc448680bf93a9). So I suppose we already have a
good enough way to stick to.

This approach in 0001 inherently processes both 32/64 bit xids and should
not change with later committing 64bit xids later (
/messages/by-id/CACG=ezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe=pyyjVWA@mail.gmail.com
)

The thing that needs to change then is suppressing output of Epoch. It
should be done when 64-xids are committed and it is done by 0006 in the
mentioned thread. Until that I've left Epoch in the output.

Big thanks for your considerations!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;