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

Hi hackers,

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

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.

v19-0002 refactors SLRU and the dependent code so that `pageno`s
become int64's. This is a requirement for the rest of the patches.

The patches were in pretty good shape last time I checked several days
ago, I even suggested changing their status to "Ready for Committer".
Let's see what cfbot will tell us.

[1]: https://postgr.es/m/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

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

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]
> https://www.postgresql.org/message-id/flat/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.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,

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?

https://www.postgresql.org/message-id/20220318231430.m5g56yk4ztlz2man%40alap3.anarazel.de

Greetings,

Andres Freund

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

Hi hackers,

> Here is v23. As was suggested by Alexander above, I've changed the order of the patches and improved the commit
message.Now, SLRU patch is the first.
 

Many thanks!

> There were proposals to make use of SLRU pages numbers that are in fact unsigned and change from int to uint64. I
fullysupport this, but I'm not sure this big SLRU refactoring should be done in this patchset.
 

If it takes a lot of effort and doesn't bring us any closer to 64-bit
XIDs, I suggest not doing this in v23-0001. I can invest some time
into this refactoring in April and create a separate CF entry, if
someone will second the idea.

> In general, I consider this patchset is ready to commit. It would be great to deliver it in PG15.

+1.

v23-0002 seems to have two extra sentences in the commit message that
are outdated, but this is a minor issue. The commit message should be:

"""
Replace the %u formatting string for XIDs with %llu and cast to
unsigned long long. While actually XIDs are still 32 bit, this patch
completely supports both 32 and 64 bit.
"""

Since Peter expressed some concerns regarding v23-0002, maybe we
should discuss it a bit more. Although personally I doubt that we can
do much better than that, and as I recall this particular change was
explicitly requested by several people.

-- 
Best regards,
Aleksander Alekseev

On Wed, 23 Mar 2022 at 01:22, Maxim Orlov <orlovmg@gmail.com> wrote:
> Hi!
>
> Here is v24. Changes are:
> - correct commit messages for 0001 and 0002
> - use uint64 for SLRU page numbering instead of int64 in v23
> - fix code formatting and indent
> - and minor fixes in slru.c
>
> Reviews are very welcome!
>

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)));

IMO, we can remove the double quote inside the sentence.

                                 errmsg_internal("found xmax %llu (infomask 0x%04x) not frozen, not multi, not
normal",
                                                                 (unsigned long long) xid, tuple->t_infomask)));

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

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.

At Fri, 25 Mar 2022 00:02:55 +0400, Pavel Borisov <pashkin.elfe@gmail.com> wrote in 
> Hi!
> It seems that CFbot was still unhappy with pg_upgrade test due to epoch
> removal from NextXid in controldata.
> I've reverted this change as support for "epochless" 64-bit control data
> with xids that haven't yet switched to 64-bit would otherwise need extra
> temporary code to support.
> I suppose this should be committed with the main 64xid (0006) patch later.
> 
> PFA v28 patch.
> Thanks, you all for your attention, interest, and help with this patch!

+SlruScanDirCbFindEarliest(SlruCtl ctl, char *filename, int64 segpage, void *data)

segpage doesn't fit mxtruncinfo.earliestExistingPage.  Doesn't it need
to be int64?


+    return snprintf(path, MAXPGPATH, "%s/%04llX", ctl->Dir, (long long) segno);

We have two way to go here.  One way is expanding the file name
according to the widened segno, another is keep the old format string
then cast the segno to (int).  Since the objective of this patch is
widen pageno, I think, as Pavel's comment upthread, we should widen
the file format to "%s/%012llX".



As Peter suggested upthread,

+    int64        segno = pageno / SLRU_PAGES_PER_SEGMENT;
+    int64        rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
+    int64        offset = rpageno * BLCKSZ;

rpageno is apparently over-sized. So offset is also over-sized.  segno
can be up to 48 bits (maybe) so int64 is appropriate.

-SlruPhysicalWritePage(SlruCtl ctl, int pageno, int slotno, SlruWriteAll fdata)
+SlruPhysicalWritePage(SlruCtl ctl, int64 pageno, int slotno, SlruWriteAll fdata)

This function does the followng.

>    FileTag        tag;
>
>    INIT_SLRUFILETAG(tag, ctl->sync_handler, segno);

tag.segno is uin32, which is too narrow here.



This is not an issue of this patch, but..

-         errdetail("Could not read from file \"%s\" at offset %u: %m.",
-                   path, offset)));

Why do we print int by "%u" here, even though that doesn't harm at all?



-SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int segpage, void *data)
+SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int64 segpage,
+                            void *data)
 {
-    int            cutoffPage = *(int *) data;
+    int64        cutoffPage = *(int64 *) data;

SlruMayDeleteSegment, called from this function, still thinks page
numbers as int.


         if ((len == 4 || len == 5 || len == 6) &&
             strspn(clde->d_name, "0123456789ABCDEF") == len)
         {
-            segno = (int) strtol(clde->d_name, NULL, 16);
+            segno = strtoi64(clde->d_name, NULL, 16);

(I'm not sure about "len == 5 || len == 6", though), the name of the
file is (I think) now expanded to 12 bytes.  Otherwise, strtoi64 is
not needed here.



-/* Currently, no field of AsyncQueueEntry requires more than int alignment */
-#define QUEUEALIGN(len)        INTALIGN(len)
+/* AsyncQueueEntry.xid requires 8-byte alignment */
+#define QUEUEALIGN(len)        TYPEALIGN(8, len)
 
I think we haven't expanded xid yet? (And the first member of
AsyncQueueEntry is int even after expanding xid.)


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi hackers,

> Here is v30.

I took another look at the patchset. Personally I don't think it will get much
better than it is now. I'm inclined to change the status of the CF entry to
"Ready for Committer" unless anyone disagrees.

cfbot reports a problem with t/013_partition.pl but the test seems to be flaky
on `master` [1]. I couldn't find anything useful in the logs except for
"[postmaster] LOG:  received immediate shutdown request". Then I re-checked the
patchset on FreeBSD 13 locally. The patchset passed `make installcked-world`.

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

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

I understand how disappointing this may be.

Personally I don't have a strong opinion here. Merging the patch sooner will
allow us to move toward 64-bit XIDs faster (e.g. to gather the feedback from
the early adopters, allow the translators to do their thing earlier, etc).
Merging it later will make PG15 more stable (you can't break anything new if
you don't change anything). As always, engineering is all about compromises.

It's up to the committer to decide.

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogfish&dt=2022-03-25%2018%3A37%3A10

--
Best regards,
Aleksander Alekseev

On 28.03.22 12:46, Aleksander Alekseev wrote:
> Personally I don't have a strong opinion here. Merging the patch sooner will
> allow us to move toward 64-bit XIDs faster (e.g. to gather the feedback from
> the early adopters, allow the translators to do their thing earlier, etc).
> Merging it later will make PG15 more stable (you can't break anything new if
> you don't change anything). As always, engineering is all about compromises.

At this point, I'm more concerned that code review is still finding 
bugs, and that we have no test coverage for any of this, so we are 
supposed to gain confidence in this patch by staring at it very hard. ;-)

AFAICT, this patch provides no actual functionality change, so holding 
it a bit for early in the PG16 cycle wouldn't lose anything.

Hi hackers,

> I took another look at the patchset. Personally I don't think it will get much
> better than it is now. I'm inclined to change the status of the CF entry to
> "Ready for Committer" unless anyone disagrees.

> > 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.
>
> AFAICT, this patch provides no actual functionality change, so holding
> it a bit for early in the PG16 cycle wouldn't lose anything.

OK. As I understand we still have a consensus that v30-0001 (SLRU refactoring,
not the XID formatting) is targeting PG15, so I changed the CF entry to
"Ready for Committer" for this single patch. I rechecked it again on the
current `master` branch without the other patches and it is OK. cfbot is happy
with the patchset as well.

-- 
Best regards,
Aleksander Alekseev

On 29.03.22 15:09, Aleksander Alekseev wrote:
> Hi hackers,
> 
>> I took another look at the patchset. Personally I don't think it will get much
>> better than it is now. I'm inclined to change the status of the CF entry to
>> "Ready for Committer" unless anyone disagrees.
> 
>>> 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.
>>
>> AFAICT, this patch provides no actual functionality change, so holding
>> it a bit for early in the PG16 cycle wouldn't lose anything.
> 
> OK. As I understand we still have a consensus that v30-0001 (SLRU refactoring,
> not the XID formatting) is targeting PG15, so I changed the CF entry to
> "Ready for Committer" for this single patch. I rechecked it again on the
> current `master` branch without the other patches and it is OK. cfbot is happy
> with the patchset as well.

That isn't really what I meant.  When I said

 > At this point, I'm more concerned that code review is still finding
 > bugs, and that we have no test coverage for any of this

this meant especially the SLRU refactoring patch.

Hi Peter,

> That isn't really what I meant.  When I said
>
>  > At this point, I'm more concerned that code review is still finding
>  > bugs, and that we have no test coverage for any of this
>
> this meant especially the SLRU refactoring patch.

Got it, and sorry for the confusion. I decided to invest some time
into improving the SLRU test coverage. I created a new thread, please
join the discussion [1].

Maxim, Pavel and I agreed (offlist) that I will rebase v30 patchset if
[1] will be merged.

[1]: https://postgr.es/m/CAJ7c6TOFoWcHOW4BVe3BG_uikCrO9B91ayx9d6rh5JZr_tPESg%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> here is the rebased v32 version of the patch.

The patchset rotted a bit. Here is a rebased version.

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

Here is the rebased patchset.

-- 
Best regards,
Aleksander Alekseev

Hi Maxim,

> It seems convenient to use common numbering of versions in this thread and the thread [1].

Agree!

Here is the rebased patchset v41.

-- 
Best regards,
Aleksander Alekseev

On Fri, May 13, 2022 at 04:21:29PM +0300, Maxim Orlov wrote:
> We have posted an updated version v34 of the whole patchset in [1].
> Changes of patches 0001-0003 there are identical to v33. So, no update is
> needed in this thread.

Is there any reason to continue with two separate threads and CF entries ?
The original reason was to have a smaller patch for considerate late in v15.

But right now, it just seems to cause every update to imply two email messages
rather than one.

Since the patch is split into 0001, 0002, 0003, 0004+, both can continue in the
main thread.  The early patches can still be applied independently from each
later patch (the same as with any other patch series).

Also, since this patch series is large, and expects a lot of conflicts, it
seems better to update the cfapp with a "git link" [0] where you can maintain a
rebased branch.  It avoids the need to mail new patches to the list several
times more often than it's being reviewed.

[0] Note that cirrusci will run the same checks as cfbot if you push a branch
to github.

Pavel, Justin,

>> Is there any reason to continue with two separate threads and CF entries ?
>> The original reason was to have a smaller patch for considerate late in v15.

> I see the main goal of this split is to make discussion of this (easier) thread separate to the discussion of a whole
patchsetwhich is expected to be more thorough.
 

+1. This was done per explicit request in the first thread.

>> But right now, it just seems to cause every update to imply two email messages
>> rather than one.
>>
>> Also, since this patch series is large, and expects a lot of conflicts, it
>> seems better to update the cfapp with a "git link" [0] where you can maintain a
>> rebased branch.  It avoids the need to mail new patches to the list several
>> times more often than it's being reviewed.

Yep, this is a bit inconvenient. Personally I didn't expect that
merging patches in this thread would take that long. They are in
"Ready for Committer" state for a long time now and there are no known
issues with them other than unit tests for SLRU [1] should be merged
first.

I suggest we use "git link" for the larger patchset in the other
thread since I'm not contributing to it right now and all in all that
thread is waiting for this one. For this thread we continue using
patches since several people contribute to them.

[1]: https://commitfest.postgresql.org/38/3608/

-- 
Best regards,
Aleksander Alekseev

On Fri, 15 Jul 2022 at 12:29, Aleksander Alekseev
<aleksander@timescale.com> wrote:
> Personally I didn't expect that
> merging patches in this thread would take that long. They are in
> "Ready for Committer" state for a long time now and there are no known
> issues with them other than unit tests for SLRU [1] should be merged
> first.

These patches look ready to me, including the SLRU tests.

Even though they do very little, these patches touch many aspects of
the code, so it would make sense to apply these as the last step in
the CF.

To encourage committers to take that next step, let's have a
democratic vote on moving this forwards:
+1 from me.

-- 
Simon Riggs                http://www.EnterpriseDB.com/

Hi Hamid,

> There are failures for multiple files including the one given below:

Thanks for letting us know. There was a little conflict in
src/backend/replication/logical/worker.c since the message format has
changed. Nothing particularly awful.

Here is the rebased patchset.

> These patches look ready to me, including the SLRU tests.

> To encourage committers to take that next step, let's have a
> democratic vote on moving this forwards: +1 from me.

Thanks, Simon. Not 100% sure who exactly is invited to vote, but just
in case here is +1 from me. These patches were "Ready for Committer"
for several months now and are unlikely to get any better. So I
suggest we merge them.

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> Thanks, Simon. Not 100% sure who exactly is invited to vote, but just
> in case here is +1 from me. These patches were "Ready for Committer"
> for several months now and are unlikely to get any better. So I
> suggest we merge them.

Here is the rebased patchset.

-- 
Best regards,
Aleksander Alekseev

Maxim,

> Here is a rebased version of the patch set.

This is the wrong thread / CF entry. Please see
http://cfbot.cputube.org/ and https://commitfest.postgresql.org/ and
the first email in the thread.

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> Sorry about that.

No problem.

> Thanks, Simon. Not 100% sure who exactly is invited to vote, but just
> in case here is +1 from me. These patches were "Ready for Committer"
> for several months now and are unlikely to get any better. So I
> suggest we merge them.

Here is the rebased patchset number 44.

-- 
Best regards,
Aleksander Alekseev

2022年10月10日(月) 18:16 Aleksander Alekseev <aleksander@timescale.com>:
>
> Hi hackers,
>
> > Sorry about that.
>
> No problem.
>
> > Thanks, Simon. Not 100% sure who exactly is invited to vote, but just
> > in case here is +1 from me. These patches were "Ready for Committer"
> > for several months now and are unlikely to get any better. So I
> > suggest we merge them.
>
> Here is the rebased patchset number 44.

Hi

This entry was marked "Ready for committer" in the CommitFest app but cfbot
reports the patch no longer applies.

We've marked it as "Waiting on Author". As CommitFest 2022-11 is
currently underway, this would be an excellent time update the patch.

Once you think the patchset is ready for review again, you (or any
interested party) can move the patch entry forward by visiting

    https://commitfest.postgresql.org/40/3489/

and changing the status to "Ready for committer".


Thanks

Ian Barwick

On Thu, Nov 3, 2022 at 1:46 PM Maxim Orlov <orlovmg@gmail.com> wrote:
>
> Hi!
>
>>
>> This entry was marked "Ready for committer" in the CommitFest app but cfbot
>> reports the patch no longer applies.
>
> Thanks for the reminder. I think, this should work.

Have we measured the WAL overhead because of this patch set? maybe
these particular patches will not impact but IIUC this is ground work
for making xid 64 bit.  So each XLOG record size will increase at
least by 4 bytes because the XLogRecord contains the xid.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

2022年11月3日(木) 17:15 Maxim Orlov <orlovmg@gmail.com>:
>
> Hi!
>
>>
>> This entry was marked "Ready for committer" in the CommitFest app but cfbot
>> reports the patch no longer applies.
>
> Thanks for the reminder. I think, this should work.

Thanks for the quick update, cfbot reports no issues.

I've set the CF entry back to "Ready for Committer",

Regards

Ian Barwick

Hi Ian,

> I've set the CF entry back to "Ready for Committer",

Thanks. Here is the rebased patchset.

Dilip asked a good question above about the rest of the 64-bit XIDs
patches. I'm going to do some testing and post the results to the main
64-bit XIDs thread [1].

[1]: https://www.postgresql.org/message-id/CAJ7c6TOkpJi78A9chR-j0OOMvP6G%3DuR%2BscpEKsM4jtw0dK9-3Q%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> > I've set the CF entry back to "Ready for Committer",
>
> Thanks. Here is the rebased patchset.

After merging 006b69fd [1] the 0001 patch needed a rebase. PFA v46.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=006b69fd

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> > > I've set the CF entry back to "Ready for Committer",
> >
> > Thanks. Here is the rebased patchset.
>
> After merging 006b69fd the 0001 patch needed a rebase. PFA v46.

After merging 1489b1ce [1] the patchset needed a rebase. PFA v47.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1489b1ce

-- 
Best regards,
Aleksander Alekseev

On Mon, Nov 21, 2022 at 12:21:09PM +0300, Aleksander Alekseev wrote:
> After merging 1489b1ce [1] the patchset needed a rebase. PFA v47.
>
> [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1489b1ce

The CF bot is showing some failures here.  You may want to
double-check.
--
Michael

Hi Michael,

> The CF bot is showing some failures here.  You may want to
> double-check.

Thanks! PFA v48.

-- 
Best regards,
Aleksander Alekseev

Hi,

On 2022-12-07 11:40:08 +0300, Aleksander Alekseev wrote:
> Hi Michael,
>
> > The CF bot is showing some failures here.  You may want to
> > double-check.
>
> Thanks! PFA v48.

This causes a lot of failures with ubsan:

https://cirrus-ci.com/task/6035600772431872

performing post-bootstrap initialization ... ../src/backend/access/transam/slru.c:1520:9: runtime error: load of
misalignedaddress 0x7fff6362db8c for type 'int64', which requires 8 byte alignment
 
0x7fff6362db8c: note: pointer points here
  01 00 00 00 00 00 00 00  d0 02 00 00 00 00 00 00  d0 02 00 00 00 00 00 00  01 00 00 00 00 00 00 00
              ^
==18947==Using libbacktrace symbolizer.
    #0 0x564d7c45cc6b in SlruScanDirCbReportPresence ../src/backend/access/transam/slru.c:1520
    #1 0x564d7c45cd88 in SlruScanDirectory ../src/backend/access/transam/slru.c:1595
    #2 0x564d7c44872c in TruncateCLOG ../src/backend/access/transam/clog.c:889
    #3 0x564d7c62ecd7 in vac_truncate_clog ../src/backend/commands/vacuum.c:1779
    #4 0x564d7c6320a8 in vac_update_datfrozenxid ../src/backend/commands/vacuum.c:1642
    #5 0x564d7c632a78 in vacuum ../src/backend/commands/vacuum.c:537
    #6 0x564d7c63347d in ExecVacuum ../src/backend/commands/vacuum.c:273
    #7 0x564d7ca4afea in standard_ProcessUtility ../src/backend/tcop/utility.c:866
    #8 0x564d7ca4b723 in ProcessUtility ../src/backend/tcop/utility.c:530
    #9 0x564d7ca46e81 in PortalRunUtility ../src/backend/tcop/pquery.c:1158
    #10 0x564d7ca4755d in PortalRunMulti ../src/backend/tcop/pquery.c:1315
    #11 0x564d7ca47c02 in PortalRun ../src/backend/tcop/pquery.c:791
    #12 0x564d7ca40ecb in exec_simple_query ../src/backend/tcop/postgres.c:1238
    #13 0x564d7ca43c01 in PostgresMain ../src/backend/tcop/postgres.c:4551
    #14 0x564d7ca441a4 in PostgresSingleUserMain ../src/backend/tcop/postgres.c:4028
    #15 0x564d7c74d883 in main ../src/backend/main/main.c:197
    #16 0x7fde7793dd09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09)
    #17 0x564d7c2d30c9 in _start (/tmp/cirrus-ci-build/build/tmp_install/usr/local/pgsql/bin/postgres+0x8530c9)

Greetings,

Andres Freund

On Wed, Dec 7, 2022 at 9:50 AM Andres Freund <andres@anarazel.de> wrote:
> performing post-bootstrap initialization ... ../src/backend/access/transam/slru.c:1520:9: runtime error: load of
misalignedaddress 0x7fff6362db8c for type 'int64', which requires 8 byte alignment
 
> 0x7fff6362db8c: note: pointer points here
>   01 00 00 00 00 00 00 00  d0 02 00 00 00 00 00 00  d0 02 00 00 00 00 00 00  01 00 00 00 00 00 00 00

I bet that this alignment issue can be fixed by using PGAlignedBlock
instead of a raw char buffer for a page. (I'm guessing, I haven't
directly checked.)

-- 
Peter Geoghegan

Hi Andres,

> This causes a lot of failures with ubsan

Thanks for reporting this!

I managed to reproduce the issue locally and to fix it. UBSAN is happy
now. PFA v49.

> I bet that this alignment issue can be fixed by using PGAlignedBlock
> instead of a raw char buffer for a page. (I'm guessing, I haven't
> directly checked.)

No, actually the problem was much simpler.

0001 changes SLRU page numbering from 32-bit to 64-bit one. This also
changed the SlruScanDirCbReportPresence() callback:

```
bool
SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int64 segpage,
                            void *data)
{
    int64        cutoffPage = *(int64 *) data;
```

However TruncateCLOG() and TruncateCommitTs() were not changed
accordingly in v47, they were passing a pointer to int32 as *data.

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> I managed to reproduce the issue locally and to fix it. UBSAN is happy
> now. PFA v49.

Maxim Orlov pointed out offlist that this cast is not needed:

```
-       SimpleLruTruncate(CommitTsCtl, (int)cutoffPage);
+       SimpleLruTruncate(CommitTsCtl, cutoffPage);
```

SimpleLruTruncate() accepts cutoffPage as uint64 in 0001.

PFA the corrected patchset v50.

-- 
Best regards,
Aleksander Alekseev

On Mon, Dec 19, 2022 at 6:41 AM Maxim Orlov <orlovmg@gmail.com> wrote:
>
> As always, reviews and opinions are very welcome.


Hi! I think that 64-bit xids are a very important feature and I want
to help advance it. That's why I want to try to understand a patch
better.
Do I get it right that the proposed v51 patchset only changes the SLRU
filenames and type of pageno representation? Is SLRU wraparound still
exactly there after 0xFFFFFFFF byte?

The thing is we had some nasty bugs because SLRU wraparound is tricky.
And I think it would be beneficial if we could get to continuous SLRU
space. But the patch seems to avoid addressing this problem.

Also, I do not understand what is the reason for splitting 1st and 2nd
steps. Certainly, there must be some logic behind it, but I just can't
grasp it...
And the purpose of the 3rd step with pg_upgrade changes is a complete
mystery for me. Please excuse my incompetence in the topic, but maybe
some commit message or comments would help. What kind of xact segments
conversion we do? Why is it only necessary for xacts, but not other
SLRUs?

Thank you for working on this important project!


Best regards, Andrey Borodin.

On Mon, 19 Dec 2022 at 22:40, Maxim Orlov <orlovmg@gmail.com> wrote:
> Hi!
>
> As a result of discussion in the thread [0], Robert Haas proposed to focus
> on making SLRU 64 bit, as a first step towards 64 bit XIDs.
> Here is the patch set.
>
> In overall, code of this patch set is based on the existing code from [0]
> and may be simplified, due to the fact, that SLRU_PAGES_PER_SEGMENT is not
> meant to be changed now.
> But I decided to leave it that way. At least for now.
>
> As always, reviews and opinions are very welcome.
>

For v51-0003. We can use GetClogDirName instead of GET_MAJOR_VERSION in
copy_subdir_files().

diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 1c49c63444..3934978b97 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -857,10 +857,7 @@ copy_xact_xlog_xid(void)
         pfree(new_path);
     }
     else
-        copy_subdir_files(GET_MAJOR_VERSION(old_cluster.major_version) <= 906 ?
-                          "pg_clog" : "pg_xact",
-                          GET_MAJOR_VERSION(new_cluster.major_version) <= 906 ?
-                          "pg_clog" : "pg_xact");
+        copy_subdir_files(GetClogDirName(old_cluster), GetClogDirName(new_cluster));
 
     prep_status("Setting oldest XID for new cluster");
     exec_prog(UTILITY_LOG_FILE, NULL, true, true,

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

Hi Andrey,

> Hi! I think that 64-bit xids are a very important feature and I want
> to help advance it. That's why I want to try to understand a patch
> better.

Thanks for your interest to the patchset!

> Do I get it right that the proposed v51 patchset only changes the SLRU
> filenames and type of pageno representation? Is SLRU wraparound still
> exactly there after 0xFFFFFFFF byte?

OK, let me give some background then. I suspect you already know this,
but this can be useful for other reviewers. Additionally we have two
rather large threads on our hands and it's easy to lose track of
things.

SLRU is basically a general-purpose LRU implementation with ReadPage()
/ WritePage() interface with the only exception that instead of
something like Page* object it operates slot numbers (array indexes).
SLRU is used as an underlying container for several internal
PostgreSQL structures, most importantly CLOG. Despite the name CLOG is
not a log (journal) but rather a large bit array. For every
transaction it stores two bits that reflect the status of the
transaction (more detail in clog.c / clog.h).

Currently SLRU operates 32-bit page numbers. What we currently agreed
on [1] and what we are trying to achieve in this thread is to make
SLRU pages 64-bit. The rest of the 64-bit XIDs is discussed in another
thread [2].

As Robert Haas put it:

> Specifically, there are a couple of patches in here that
> have to do with making SLRUs indexed by 64-bit integers rather than by
> 32-bit integers. We've had repeated bugs in the area of handling SLRU
> wraparound in the past, some of which have caused data loss. Just by
> chance, I ran across a situation just yesterday where an SLRU wrapped
> around on disk for reasons that I don't really understand yet and
> chaos ensued. Switching to an indexing system for SLRUs that does not
> ever wrap around would probably enable us to get rid of a whole bunch
> of crufty code, and would also likely improve the general reliability
> of the system in situations where wraparound is threatened. It seems
> like a really, really good idea.

So our goal here is to eliminate wrap-around for SLRU. It means that
if I save something to the page 0x0000000012345678 it will stay there
forever. Other parts of the system however have to form proper 64-bit
page numbers in order to make it work. If they don't the wrap-around
is possible for these particular subsystems (but not SLRU per se).

> Also, I do not understand what is the reason for splitting 1st and 2nd
> steps. Certainly, there must be some logic behind it, but I just can't
> grasp it...

0001 patch changes the SLRU internals without affecting the callers.
It also preserves the short SLRU filenames which means nothing changes
for an outside observer. All it changes is PostgreSQL binary. It can
be merged any time and even backported to the previous versions if we
want to.

The 0002 patch makes changes to the callers and also enlarges SLRU
filenames. For sure we could do everything at once, but it would
complicate testing and more importantly code review. Personally I
believe Maxim did a great job here. Both patches were easy to read and
understand (relatively, of course).

> And the purpose of the 3rd step with pg_upgrade changes is a complete
> mystery for me.

0001 and 0002 will work fine for new PostgreSQL instances. But if you
have an instance that already has on-disk state we have to move the
SLRU segments accordingly. This is what 0003 does.

That's the theory at least. Personally I still have to meditate a bit
more on the code in order to get a good understanding of it,
especially the parts that deal with transaction epochs because this is
something I have limited experience with. Also I wouldn't exclude the
possibility of bugs. Particularly this part of 0003:

```
+    oldseg.segno = pageno / SLRU_PAGES_PER_SEGMENT;
+    oldseg.pageno = pageno % SLRU_PAGES_PER_SEGMENT;
+
+    newseg.segno = pageno / SLRU_PAGES_PER_SEGMENT;
+    newseg.pageno = pageno % SLRU_PAGES_PER_SEGMENT;
```

looks suspicious to me.

I agree that adding a couple of additional comments could be
appropriate, especially when it comes to epochs.

[1]: https://www.postgresql.org/message-id/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com
-- 
Best regards,
Aleksander Alekseev

Hi Maxim,

> Here is a new patch set.
> I've added comments and make use GetClogDirName call in copy_subdir_files.

Jacob Champion pointed out (offlist, cc:'ed) that we may be wrong on this one:

> 0001 patch changes the SLRU internals without affecting the callers.

> 0001 - should make SLRU internally 64–bit, no effects from "outside"

... and apparently Jacob is right.

Besides other things 0001 modifies CLOG_ZEROPAGE and CLOG_TRUNCATE WAL
records - it makes changes to WriteZeroPageXlogRec() and
WriteTruncateXlogRec() and corresponding changes to clog_desc() and
clog_redo().

Firstly, it means that the patch doesn't change what it claims to
change. I think these changes should be part of 0002.

Secondly, shouldn't we introduce a new WAL record type in order to
make the code backward compatible with previous PG versions? I'm not
100% sure how the upgrade procedure works in all the details. If it
requires the DBMS to be gracefully shut down before the upgrade then
we are probably fine here.

--
Best regards,
Aleksander Alekseev

Hi Maxim,

> Secondly, shouldn't we introduce a new WAL record type in order to
> make the code backward compatible with previous PG versions? I'm not
> 100% sure how the upgrade procedure works in all the details. If it
> requires the DBMS to be gracefully shut down before the upgrade then
> we are probably fine here.

After reading [1] carefully it looks like we shouldn't worry about
this. The upgrade procedure explicitly requires to run `pg_ctl stop`
during step 8 of the upgrade procedure, i.e. not in the immediate mode
[2]. It also has explicit instructions regarding the replicas. From
what I can tell there is no way they will see WAL records they
wouldn't understand.

[1]: https://www.postgresql.org/docs/current/pgupgrade.html
[2]: https://www.postgresql.org/docs/current/app-pg-ctl.html

-- 
Best regards,
Aleksander Alekseev

On Wed, Jan 11, 2023 at 1:48 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> After reading [1] carefully it looks like we shouldn't worry about
> this. The upgrade procedure explicitly requires to run `pg_ctl stop`
> during step 8 of the upgrade procedure, i.e. not in the immediate mode
> [2].

Yeah, pg_upgrade will briefly start and stop the old server to make
sure all the WAL is replayed, and won't transfer any of the files
over. AFAIK, major-version WAL changes are fine; it was the previous
claim that we could do it in a minor version that I was unsure about.

Thanks!
--Jacob

Hi hackers,

> Yeah, pg_upgrade will briefly start and stop the old server to make
> sure all the WAL is replayed, and won't transfer any of the files
> over. AFAIK, major-version WAL changes are fine; it was the previous
> claim that we could do it in a minor version that I was unsure about.

OK, here is the patchset v53 where I mostly modified the commit
messages. It is explicitly said that 0001 modifies the WAL records and
why we decided to do it in this patch. Additionally any mention of
64-bit XIDs is removed since it is not guaranteed that the rest of the
patches are going to be accepted. 64-bit SLRU page numbering is a
valuable change per se.

Changing the status of the CF entry to RfC apparently was a bit
premature. It looks like the patchset can use a few more rounds of
review.

In 0002:

```
-#define TransactionIdToCTsPage(xid) \
-    ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
+static inline int64
+TransactionIdToCTsPageInternal(TransactionId xid, bool lock)
+{
+    FullTransactionId    fxid,
+                        nextXid;
+    uint32                epoch;
+
+    if (lock)
+        LWLockAcquire(XidGenLock, LW_SHARED);
+
+    /* make a local copy */
+    nextXid = ShmemVariableCache->nextXid;
+
+    if (lock)
+        LWLockRelease(XidGenLock);
+
+    epoch = EpochFromFullTransactionId(nextXid);
+    if (xid > XidFromFullTransactionId(nextXid))
+        --epoch;
+
+    fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
+
+    return fxid.value / (uint64) COMMIT_TS_XACTS_PER_PAGE;
+}
```

I'm pretty confident that shared memory can't be accessed like this,
without taking a lock. Although it may work on x64 generally we can
get garbage, unless nextXid is accessed atomically and has a
corresponding atomic type. On top of that I'm pretty sure
TransactionIds can't be compared with the regular comparison
operators. All in all, so far I don't understand why this piece of
code should be so complicated.

The same applies to:

```
-#define TransactionIdToPage(xid) ((xid) / (TransactionId)
SUBTRANS_XACTS_PER_PAGE)
+static inline int64
+TransactionIdToPageInternal(TransactionId xid, bool lock)
```

... in subtrans.c

Maxim, perhaps you could share with us what your reasoning was here?

-- 
Best regards,
Aleksander Alekseev

Hi,

> OK, here is the patchset v53 where I mostly modified the commit
> messages. It is explicitly said that 0001 modifies the WAL records and
> why we decided to do it in this patch. Additionally any mention of
> 64-bit XIDs is removed since it is not guaranteed that the rest of the
> patches are going to be accepted. 64-bit SLRU page numbering is a
> valuable change per se.
>
> Changing the status of the CF entry to RfC apparently was a bit
> premature. It looks like the patchset can use a few more rounds of
> review.
>
> In 0002:
>
> [...]
>
> Maxim, perhaps you could share with us what your reasoning was here?

I played with the patch a bit and managed to figure out what you tried
to accomplish. Unfortunately generally you can't derive a
FullTransactionId from a TransactionId, and you can't access
ShmemVariableCache fields without taking a lock unless during the
startup when there are no concurrent processes.

I don't think this patch should do anything but change the SLRU
indexing from 32-bit to 64-bit one. Trying to address the wraparounds
would be nice but I'm afraid we are not quite there yet.

Also I found strage little changes that seemed to be unrelated to the
patch. I believe they ended up here by accident (used to be a part of
64-bit XIDs patchset) and removed them.

PFA the cleaned up version of the patch. I noticed that splitting it
into parts doesn't help much with the review or testing, nor seems it
likely that the patches are going to be merged separately one by one.
For these reasons I merged everything into a single patch.

The convert_pg_xact_segments() function is still obviously
overengineered. As I understand, all it has to do is simply renaming
pg_xact/XXXX to pg_xact/00000000XXXX. Unfortunately I used up all the
mana for today and have to take a long rest in order to continue.

-- 
Best regards,
Aleksander Alekseev

Hi,

> The convert_pg_xact_segments() function is still obviously
> overengineered. As I understand, all it has to do is simply renaming
> pg_xact/XXXX to pg_xact/00000000XXXX. Unfortunately I used up all the
> mana for today and have to take a long rest in order to continue.

PFA the corrected patch v55.

One thing that still bothers me is that during the upgrade we only
migrate the CLOG segments (i.e. pg_xact / pg_clog) and completely
ignore all the rest of SLRUs:

* pg_commit_ts
* pg_multixact/offsets
* pg_multixact/members
* pg_subtrans
* pg_notify
* pg_serial

My knowledge in this area is somewhat limited and I can't tell whether
this is OK. I will investigate but also I could use some feedback from
the reviewers.

-- 
Best regards,
Aleksander Alekseev

(CC list trimmed, gmail wouldn't let me send otherwise)

On 22/02/2023 16:29, Maxim Orlov wrote:
> On Tue, 21 Feb 2023 at 16:59, Aleksander Alekseev 
> <aleksander@timescale.com <mailto:aleksander@timescale.com>> wrote:
>     One thing that still bothers me is that during the upgrade we only
>     migrate the CLOG segments (i.e. pg_xact / pg_clog) and completely
>     ignore all the rest of SLRUs:
> 
>     * pg_commit_ts
>     * pg_multixact/offsets
>     * pg_multixact/members
>     * pg_subtrans
>     * pg_notify
>     * pg_serial
> 
> Hi! We do ignore these values, since in order to pg_upgrade the server 
> it must be properly stopped and no transactions can outlast this moment.

That sounds right for pg_serial, pg_notify, and pg_subtrans. But not for 
pg_commit_ts and the pg_multixacts.

This needs tests for pg_upgrading those SLRUs, after 0, 1 and N wraparounds.

I'm surprised that these patches extend the page numbering to 64 bits, 
but never actually uses the high bits. The XID "epoch" is not used, and 
pg_xact still wraps around and the segment names are still reused. I 
thought we could stop doing that. Certainly if we start supporting 
64-bit XIDs properly, that will need to change and we will pg_upgrade 
will need to rename the segments again.

The previous versions of these patches did that, but I think you changed 
tact in response to Robert's suggestion at [1]:

> Lest we miss the forest for the trees, there is an aspect of this
> patch that I find to be an extremely good idea and think we should try
> to get committed even if the rest of the patch set ends up in the
> rubbish bin. Specifically, there are a couple of patches in here that
> have to do with making SLRUs indexed by 64-bit integers rather than by
> 32-bit integers. We've had repeated bugs in the area of handling SLRU
> wraparound in the past, some of which have caused data loss. Just by
> chance, I ran across a situation just yesterday where an SLRU wrapped
> around on disk for reasons that I don't really understand yet and
> chaos ensued. Switching to an indexing system for SLRUs that does not
> ever wrap around would probably enable us to get rid of a whole bunch
> of crufty code, and would also likely improve the general reliability
> of the system in situations where wraparound is threatened. It seems
> like a really, really good idea.

These new versions of this patch don't achieve the goal of avoiding 
wraparound. I think the previous versions that did that was the right 
approach.

[1] 
https://www.postgresql.org/message-id/CA%2BTgmoZFmTGjgkmjgkcm2-vQq3_TzcoMKmVimvQLx9oJLbye0Q%40mail.gmail.com

- Heikki

On 28/02/2023 16:17, Maxim Orlov wrote:
> Either I do not understand something, or the files from pg_commit_ts 
> directory are not copied.

Huh, yeah you're right, pg_upgrade doesn't copy pg_commit_ts.

- Heikki

Hi,

> I'm surprised that these patches extend the page numbering to 64 bits,
> but never actually uses the high bits. The XID "epoch" is not used, and
> pg_xact still wraps around and the segment names are still reused. I
> thought we could stop doing that.

To clarify, the idea is to let CLOG grow indefinitely and simply store
FullTransactionId -> TransactionStatus (two bits). Correct?

I didn't investigate this in much detail but it may affect quite some
amount of code since TransactionIdDidCommit() and
TransactionIdDidCommit() currently both deal with TransactionId, not
FullTransactionId. IMO, this would be a nice change however, assuming
we are ready for it.

In the previous version of the patch there was an attempt to derive
FullTransactionId from TransactionId but it was wrong for the reasons
named above in the thread. Thus is was removed and the patch
simplified.

-- 
Best regards,
Aleksander Alekseev

On 01/03/2023 12:21, Aleksander Alekseev wrote:
> Hi,
> 
>> I'm surprised that these patches extend the page numbering to 64 bits,
>> but never actually uses the high bits. The XID "epoch" is not used, and
>> pg_xact still wraps around and the segment names are still reused. I
>> thought we could stop doing that.
> 
> To clarify, the idea is to let CLOG grow indefinitely and simply store
> FullTransactionId -> TransactionStatus (two bits). Correct?

Correct.

> I didn't investigate this in much detail but it may affect quite some
> amount of code since TransactionIdDidCommit() and
> TransactionIdDidCommit() currently both deal with TransactionId, not
> FullTransactionId. IMO, this would be a nice change however, assuming
> we are ready for it.

Yep, it's a lot of code churn..

> In the previous version of the patch there was an attempt to derive
> FullTransactionId from TransactionId but it was wrong for the reasons
> named above in the thread. Thus is was removed and the patch
> simplified.

Yeah, it's tricky to get it right. Clearly we need to do it at some 
point though.

All in all, this is a big effort. I spent some more time reviewing this 
in the last few days, and thought a lot about what the path forward here 
could be. And I haven't looked at the actual 64-bit XIDs patch set yet, 
just this patch to use 64-bit addressing in SLRUs.

This patch is the first step, but we have a bit of a chicken and egg 
problem, because this patch on its own isn't very interesting, but on 
the other hand, we need it to work on the follow up items. Here's how I 
see the development path for this (and again, this is just for the 
64-bit SLRUs work, not the bigger 64-bit-XIDs-in-heapam effort):

1. Use 64 bit page numbers in SLRUs (this patch)

I would like to make one change here: I'd prefer to keep the old 4-digit 
segment names, until we actually start to use the wider address space. 
Let's allow each SLRU to specify how many digits to use in the 
filenames, so that we convert one SLRU at a time.

If we do that, and don't change any of the existing SLRUs to actually 
use the wider space of page and segment numbers yet, this patch becomes 
just refactoring with no on-disk format changes. No pg_upgrade needed.

The next patches will start to make use of the wider address space, one 
SLRU at a time.

2. Use the larger segment file names in async.c, to lift the current 8 
GB limit on the max number of pending notifications.

No one actually minds the limit, it's quite generous as it is. But there 
is some code and complexity in async.c to avoid the wraparound that 
could be made simpler if we used longer SLRU segment names and avoided 
the wraparound altogether.

I wonder if we should actually add an artificial limit, as a GUC. If 
there are gigabytes of notifications queued up, something's probably 
wrong with the system, and you're not going to be happy if we just 
remove the limit so it can grow to terabytes until you run out of disk 
space.

3. Extend pg_multixact so that pg_multixact/members is addressed by 
64-bit offsets.

Currently, multi-XIDs can wrap around, requiring anti-wraparound 
freezing, but independently of that, the pg_multixact/members SLRU can 
also wrap around. We track both, and trigger anti-wraparound if either 
SLRU is about to wrap around. If we redefine MultiXactOffset as a 64-bit 
integer, we can avoid the pg_multixact/members wraparound altogether. A 
downside is that pg_multixact/offsets will take twice as much space, but 
I think that's a good tradeoff. Or perhaps we can play tricks like store 
a single 64-bit offset on each pg_multixact/offsets page, and a 32-bit 
offset from that for each XID, to avoid making it so much larger.

This would reduce the need to do anti-wraparound VACUUMs on systems that 
use multixacts heavily. Needs pg_upgrade support.

4. Extend pg_subtrans to 64-bits.

This isn't all that interesting because the active region of pg_subtrans 
cannot be wider than 32 bits anyway, because you'll still reach the 
general 32-bit XID wraparound. But it might be less confusing in some 
places.

I actually started to write a patch to do this, to see how complicated 
it is. It quickly proliferates into expanding other XIDs to 64-bits, 
like TransactionXmin, frozenXid calculation in vacuum.c, known-assigned 
XID tracking in procarray.c. etc. It's going to be necessary to convert 
32-bit XIDs to FullTransactionIds at some boundaries, and I'm not sure 
where exactly that should happen. It's easier to do the conversions 
close to subtrans.c, but then I'm not sure how much it gets us in terms 
of reducing confusion. It's easy to get confused with the epochs during 
conversions, as you noted. On the other hand, if we change much more of 
the backend to use FullTransactionIds, the patch becomes much more invasive.

Nice thing with pg_subtrans, though, is that it doesn't require 
pg_upgrade support.

5. Extend pg_xact to 64-bits.

Similar to pg_subtrans, really, but needs pg_upgrade support.

6. (a bonus thing that I noticed while thinking of pg_xact.) Extend 
pg_twophase.c, to use FullTransactionIds.

Currently, the twophase state files in pg_twophase are named according 
to the 32 bit Xid of the transaction. Let's switch to FullTransactionId 
there.



As we start to refactor these things, I also think it would be good to 
have more explicit tracking of the valid range of SLRU pages in each 
SLRU. Take pg_subtrans for example: it's not very clear what pages have 
been initialized, especially during different stages of startup. It 
would be good to have clear start and end page numbers, and throw an 
error if you try to look up anything outside those bounds. Same for all 
other SLRUs.

I propose that we try to finish 1 and 2 for v16. And maybe 6. I think 
that's doable. It doesn't have any great user-visible benefits yet, but 
we need to start somewhere.

- Heikki

On 07/03/2023 13:38, Maxim Orlov wrote:
> As for making pg_multixact 64 bit, I spend the last couple of days to 
> make proper pg_upgrade for pg_multixact's and for pg_xact's
> with wraparound and I've understood, that it is not a simple task 
> compare to pg_xact's. The problem is, we do not have epoch for
> multixacts, so we do not have ability to "overcome" wraparound. The 
> solution may be adding some kind of epoch for multixacts or
> make them 64 bit in "main" 64-xid patch, but in perspective of this 
> thread, in my view, this should be last in line here.

That is true for pg_multixact/offsets. We will indeed need to add an 
epoch and introduce the concept of FullMultiXactIds for that. However, 
we can change pg_multixact/members independently of that. We can extend 
MultiXactOffset from 32 to 64 bits, and eliminate pg_multixact/members 
wraparound, while keeping multi-xids 32 bits wide.

- Heikki

Hi,

> Here's how I see the development path for this [...]

> So, in my view, the plan should be [...]
> Thoughts?

The plan looks great! I would also explicitly include this:

> As we start to refactor these things, I also think it would be good to
> have more explicit tracking of the valid range of SLRU pages in each
> SLRU. Take pg_subtrans for example: it's not very clear what pages have
> been initialized, especially during different stages of startup. It
> would be good to have clear start and end page numbers, and throw an
> error if you try to look up anything outside those bounds. Same for all
> other SLRUs.

So the plan becomes:

1. Use internal 64 bit page numbers in SLRUs without changing segments naming.
2. Use the larger segment file names in async.c, to lift the current 8
GB limit on the max number of pending notifications.
3. Extend pg_xact to 64-bits.
4. Extend pg_subtrans to 64-bits.
5. Extend pg_multixact so that pg_multixact/members is addressed by
64-bit offsets.
6. Extend pg_twophase.c, to use FullTransactionIds. (a bonus thing)
7. More explicit tracking of the valid range of SLRU pages in each SLRU

Where our main focus for PG16 is going to be 1 and 2, and we may try
to deliver 6 and 7 too if time will permit.

Maxim and I agreed (offlist) that I'm going to submit 1 and 2. The
patch 1 is ready, please see the attachment. I'm currently working on
2 and going to submit it in a bit. It seems to be relatively
straightforward but I don't want to rush things and want to make sure
I didn't miss something.

> I wonder if we should actually add an artificial limit, as a GUC.

Yes, I think we need some sort of limit. Using a GUC would be the most
straightforward approach. Alternatively we could derive the limit from
the existing GUCs, similarly to how we derive the default value of
wal_buffers from shared_buffers [1]. However, off the top of my head
we only have max_wal_size and it doesn't strike me as a good candidate
for deriving something for NOTIFY / LISTEN.

So I'm going to add max_notify_segments GUC with the default value of
65536 as it is now. If the new GUC will receive a push back from the
community we can always use a hard-coded value instead, or no limit at
all.

[1]: https://www.postgresql.org/docs/current/runtime-config-wal.html#GUC-WAL-BUFFERS

-- 
Best regards,
Aleksander Alekseev

Hi,

> 1. Use internal 64 bit page numbers in SLRUs without changing segments naming.
> 2. Use the larger segment file names in async.c, to lift the current 8
> GB limit on the max number of pending notifications.
> [...]
>
> Where our main focus for PG16 is going to be 1 and 2, and we may try
> to deliver 6 and 7 too if time will permit.
>
> Maxim and I agreed (offlist) that I'm going to submit 1 and 2. The
> patch 1 is ready, please see the attachment. I'm currently working on
> 2 and going to submit it in a bit. It seems to be relatively
> straightforward but I don't want to rush things and want to make sure
> I didn't miss something.

PFA the patch v57 which now includes both 1 and 2.

-- 
Best regards,
Aleksander Alekseev

Hi,

> As suggested before by Heikki Linnakangas, I've added a patch for making 2PC transaction state 64-bit.
> At first, my intention was to rebuild all twophase interface to use FullTransactionId. But doing this in a proper
> manner would lead to switching from TransactionId to FullTransactionId in PGPROC and patch become too
> big to handle here.
>
> So I decided to keep it simple for now and use wrap logic trick and calc FullTransactionId on current epoch,
> since the span of active xids cannot exceed one epoch at any given time.
>
> Patches 1 and 2 are the same as above.

Thanks! 0003 LGTM. I'll mark the CF entry as RfC.

> I propose that we try to finish 1 and 2 for v16. And maybe 6. I think
> that's doable. It doesn't have any great user-visible benefits yet, but
> we need to start somewhere.

+1.

However there are only a few days left if we are going to do this
within March CF...

-- 
Best regards,
Aleksander Alekseev

On 09/03/2023 17:21, Aleksander Alekseev wrote:
> v57-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch

Reviewing this now. I think it's almost ready to be committed.

There's another big effort going on to move SLRUs to the regular buffer 
cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving 
to 64 bit page numbers will affect that. BlockNumber is still 32 bits, 
after all.

> +/*
> + * An internal function used by SlruScanDirectory().
> + *
> + * Returns true if a file with a name of a given length may be a correct
> + * SLRU segment.
> + */
> +static inline bool
> +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
> +{
> +       if (ctl->long_segment_names)
> +               return (len == 12);
> +       else
> +
> +               /*
> +                * XXX Should we still consider 5 and 6 to be a correct length? It
> +                * looks like it was previously allowed but now SlruFileName() can't
> +                * return such a name.
> +                */
> +               return (len == 4 || len == 5 || len == 6);
> +}

Huh, I didn't realize that 5 and 6 character SLRU segment names are 
possible. But indeed they are. Commit 638cf09e76d allowed 5-character 
lengths:

> commit 638cf09e76d70dd83d8123e7079be6c0532102d2
> Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date:   Thu Jan 2 18:17:29 2014 -0300
> 
>     Handle 5-char filenames in SlruScanDirectory
>     
>     Original users of slru.c were all producing 4-digit filenames, so that
>     was all that that code was prepared to handle.  Changes to multixact.c
>     in the course of commit 0ac5ad5134f made pg_multixact/members create
>     5-digit filenames once a certain threshold was reached, which
>     SlruScanDirectory wasn't prepared to deal with; in particular,
>     5-digit-name files were not removed during truncation.  Change that
>     routine to make it aware of those files, and have it process them just
>     like any others.
>     
>     Right now, some pg_multixact/members directories will contain a mixture
>     of 4-char and 5-char filenames.  A future commit is expected fix things
>     so that each slru.c user declares the correct maximum width for the
>     files it produces, to avoid such unsightly mixtures.
>     
>     Noticed while investigating bug #8673 reported by Serge Negodyuck.
> 

And later commit 73c986adde5, which introduced commit_ts, allowed 6 
characters. With 8192 block size, pg_commit_ts segments are indeed 5 
chars wide, and with 512 block size, 6 chars are needed.

This patch makes the filenames always 12 characters wide (for SLRUs that 
opt-in to the new naming). That's actually not enough for the full range 
that a 64 bit page number can represent. Should we make it 16 characters 
now, to avoid repeating the same mistake we made earlier? Or make it 
more configurable, on a per-SLRU basis. One reason I don't want to just 
make it 16 characters is that WAL segment filenames are also 16 hex 
characters, which could cause confusion.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Hi,

> Reviewing this now. I think it's almost ready to be committed.
>
> There's another big effort going on to move SLRUs to the regular buffer
> cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> after all.

Somehow I didn't pay too much attention to this effort, thanks. I will
familiarize myself with the patch. Intuitively I don't think that the
patchse should block each other.

> This patch makes the filenames always 12 characters wide (for SLRUs that
> opt-in to the new naming). That's actually not enough for the full range
> that a 64 bit page number can represent. Should we make it 16 characters
> now, to avoid repeating the same mistake we made earlier? Or make it
> more configurable, on a per-SLRU basis. One reason I don't want to just
> make it 16 characters is that WAL segment filenames are also 16 hex
> characters, which could cause confusion.

Good catch. I propose the following solution:

```
 SlruFileName(SlruCtl ctl, char *path, int64 segno)
 {
     if (ctl->long_segment_names)
-        return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
+        /*
+         * We could use 16 characters here but the disadvantage would be that
+         * the SLRU segments will be hard to distinguish from WAL segments.
+         *
+         * For this reason we use 15 characters. It is enough but also means
+         * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
+         */
+        return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
                         (long long) segno);
     else
         return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
```

PFE the corrected patchset v58.

--
Best regards,
Aleksander Alekseev

Hi,

> > Reviewing this now. I think it's almost ready to be committed.
> >
> > There's another big effort going on to move SLRUs to the regular buffer
> > cache (https://commitfest.postgresql.org/43/3514/). I wonder how moving
> > to 64 bit page numbers will affect that. BlockNumber is still 32 bits,
> > after all.
>
> Somehow I didn't pay too much attention to this effort, thanks. I will
> familiarize myself with the patch. Intuitively I don't think that the
> patchse should block each other.
>
> > This patch makes the filenames always 12 characters wide (for SLRUs that
> > opt-in to the new naming). That's actually not enough for the full range
> > that a 64 bit page number can represent. Should we make it 16 characters
> > now, to avoid repeating the same mistake we made earlier? Or make it
> > more configurable, on a per-SLRU basis. One reason I don't want to just
> > make it 16 characters is that WAL segment filenames are also 16 hex
> > characters, which could cause confusion.
>
> Good catch. I propose the following solution:
>
> ```
>  SlruFileName(SlruCtl ctl, char *path, int64 segno)
>  {
>      if (ctl->long_segment_names)
> -        return snprintf(path, MAXPGPATH, "%s/%012llX", ctl->Dir,
> +        /*
> +         * We could use 16 characters here but the disadvantage would be that
> +         * the SLRU segments will be hard to distinguish from WAL segments.
> +         *
> +         * For this reason we use 15 characters. It is enough but also means
> +         * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
> +         */
> +        return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
>                          (long long) segno);
>      else
>          return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> ```
>
> PFE the corrected patchset v58.

While triaging the patches for the September CF [1] a consensus was
reached that the patchset needs another round of review. Also I
removed myself from the list of reviewers in order to make it clear
that a review from somebody else would be appreciated.

[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
-- 
Best regards,
Aleksander Alekseev

On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi!
>
> On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
> > PFE the corrected patchset v58.
>
> I'd like to revive this thread.
>
> This patchset is extracted from a larger patchset implementing 64-bit xids.  It converts page numbers in SLRUs into
64bits.  The most SLRUs save the same file naming scheme, thus their on-disk representation remains the same.  However,
thepatch 0002 converts pg_notify to actually use a new naming scheme.  Therefore pg_notify can benefit from
simplificationand getting rid of wraparound. 
>
> -#define TransactionIdToCTsPage(xid) \
> -   ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> +
> +/*
> + * Although we return an int64 the actual value can't currently exceeed 2**32.
> + */
> +static inline int64
> +TransactionIdToCTsPage(TransactionId xid)
> +{
> +   return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> +}
>
> Is there any reason we transform macro into a function?  If not, I propose to leave this as a macro.  BTW, there is a
typoin a word "exceeed". 
If I remember right, the compiler will make equivalent code from
inline functions and macros, and functions has an additional benefit:
the compiler will report type mismatch if any. That was the only
reason.

Also, I looked at v58-0001 and don't quite agree with mentioning the
authors of the original 64-xid patch, from which the current patch is
derived, as just "privious input" persons.

Regards, Pavel Borisov

On Mon, Nov 6, 2023 at 3:42 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
> > > PFE the corrected patchset v58.
> >
> > I'd like to revive this thread.
> >
> > This patchset is extracted from a larger patchset implementing 64-bit xids.  It converts page numbers in SLRUs into
64bits.  The most SLRUs save the same file naming scheme, thus their on-disk representation remains the same.  However,
thepatch 0002 converts pg_notify to actually use a new naming scheme.  Therefore pg_notify can benefit from
simplificationand getting rid of wraparound. 
> >
> > -#define TransactionIdToCTsPage(xid) \
> > -   ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> > +
> > +/*
> > + * Although we return an int64 the actual value can't currently exceeed 2**32.
> > + */
> > +static inline int64
> > +TransactionIdToCTsPage(TransactionId xid)
> > +{
> > +   return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> > +}
> >
> > Is there any reason we transform macro into a function?  If not, I propose to leave this as a macro.  BTW, there is
atypo in a word "exceeed". 
> If I remember right, the compiler will make equivalent code from
> inline functions and macros, and functions has an additional benefit:
> the compiler will report type mismatch if any. That was the only
> reason.

Then it's OK to leave it as an inline function.

> Also, I looked at v58-0001 and don't quite agree with mentioning the
> authors of the original 64-xid patch, from which the current patch is
> derived, as just "privious input" persons.

+1, for converting all "previous input" persons as additional authors.
That would be a pretty long list of authors though.

BTW, I'm a bit puzzled on who should be the first author now?

------
Regards,
Alexander Korotkov

On Mon, 6 Nov 2023 at 18:01, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Mon, Nov 6, 2023 at 3:42 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> >
> > On Mon, 6 Nov 2023 at 17:07, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
> > > > PFE the corrected patchset v58.
> > >
> > > I'd like to revive this thread.
> > >
> > > This patchset is extracted from a larger patchset implementing 64-bit xids.  It converts page numbers in SLRUs
into64 bits.  The most SLRUs save the same file naming scheme, thus their on-disk representation remains the same.
However,the patch 0002 converts pg_notify to actually use a new naming scheme.  Therefore pg_notify can benefit from
simplificationand getting rid of wraparound. 
> > >
> > > -#define TransactionIdToCTsPage(xid) \
> > > -   ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> > > +
> > > +/*
> > > + * Although we return an int64 the actual value can't currently exceeed 2**32.
> > > + */
> > > +static inline int64
> > > +TransactionIdToCTsPage(TransactionId xid)
> > > +{
> > > +   return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> > > +}
> > >
> > > Is there any reason we transform macro into a function?  If not, I propose to leave this as a macro.  BTW, there
isa typo in a word "exceeed". 
> > If I remember right, the compiler will make equivalent code from
> > inline functions and macros, and functions has an additional benefit:
> > the compiler will report type mismatch if any. That was the only
> > reason.
>
> Then it's OK to leave it as an inline function.
>
> > Also, I looked at v58-0001 and don't quite agree with mentioning the
> > authors of the original 64-xid patch, from which the current patch is
> > derived, as just "privious input" persons.
>
> +1, for converting all "previous input" persons as additional authors.
> That would be a pretty long list of authors though.
> BTW, I'm a bit puzzled on who should be the first author now?
Thanks! It's long, I agree, but the activity around this was huge and
involved many people, the patch itself already has more than
half-hundred iterations to date. I think at least people mentioned in
the commit message of v58 are fair to have author status.

As for the first, I'm not sure, it's hard for me to evaluate what is
more important, the initial prototype, or the final improvement
iterations. I don't think the existing order in a commit message has
some meaning. Maybe it's worth throwing a dice.

Regards, Pavel

Hi,

> > > If I remember right, the compiler will make equivalent code from
> > > inline functions and macros, and functions has an additional benefit:
> > > the compiler will report type mismatch if any. That was the only
> > > reason.
> >
> > Then it's OK to leave it as an inline function.

+1

> > BTW, I'm a bit puzzled on who should be the first author now?
> Thanks! It's long, I agree, but the activity around this was huge and
> involved many people, the patch itself already has more than
> half-hundred iterations to date. I think at least people mentioned in
> the commit message of v58 are fair to have author status.
>
> As for the first, I'm not sure, it's hard for me to evaluate what is
> more important, the initial prototype, or the final improvement
> iterations. I don't think the existing order in a commit message has
> some meaning. Maybe it's worth throwing a dice.

Personally I was not aware that the order of the authors in a commit
message carries any information. To my knowledge it doesn't not. I
believe this patchset is a team effort, so I suggest keeping the
commit message as is or shuffling the authors randomly. I believe we
should do our best to reflect the input of people who previously
contributed to the effort, if anyone are aware of them, and add them
to the commit message if they are not there yet. Pretty sure Git will
forgive us if the list ends up being long. Hopefully so will people
who we mistakenly forget.

-- 
Best regards,
Aleksander Alekseev

Alexander,

> > PFE the corrected patchset v58.
>
> I'd like to revive this thread.

Many thanks for your comments and suggestions.

> I think it worth adding asserts here to verify there is no overflow making us mapping different segments into the
samefiles.
 

Sorry, I didn't understand this one. Maybe you could provide the exact code?

-- 
Best regards,
Aleksander Alekseev

Hi Alexander,

> -#define TransactionIdToCTsPage(xid) \
> -   ((xid) / (TransactionId) COMMIT_TS_XACTS_PER_PAGE)
> +
> +/*
> + * Although we return an int64 the actual value can't currently exceeed 2**32.
> + */
> +static inline int64
> +TransactionIdToCTsPage(TransactionId xid)
> +{
> +   return xid / (int64) COMMIT_TS_XACTS_PER_PAGE;
> +}
>
> Is there any reason we transform macro into a function?  If not, I propose to leave this as a macro.  BTW, there is a
typoin a word "exceeed". 

I kept the inline function, as we agreed above.

Typo fixed.

> +static int inline
> +SlruFileName(SlruCtl ctl, char *path, int64 segno)
> +{
> +   if (ctl->long_segment_names)
> +       /*
> +        * We could use 16 characters here but the disadvantage would be that
> +        * the SLRU segments will be hard to distinguish from WAL segments.
> +        *
> +        * For this reason we use 15 characters. It is enough but also means
> +        * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
> +        */
> +       return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
> +                       (long long) segno);
> +   else
> +       return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
> +                       (unsigned int) segno);
> +}
>
> I think it worth adding asserts here to verify there is no overflow making us mapping different segments into the
samefiles. 

Added. I noticed a off-by-one error in the code snippet proposed
above, so my code differs a bit.

> +   return occupied == max_notify_queue_pages;
>
> I'm not sure if the current code could actually allow to occupy more than max_notify_queue_pages.  Probably not even
inextreme cases.  But I still think it will more safe and easier to read to write "occupied >= max_notify_queue"_pages
here.

Fixed.

> diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
>
> The actual 64-bitness of SLRU pages isn't much exercised in our automated tests.  It would be too exhausting to make
pg_notifyactually use higher than 2**32 page numbers.  Thus, I think test/modules/test_slru is a good place to give
highpage numbers a good test. 

Fixed. I choose not to change any numbers in the test in order to
check any corner cases, etc. The code patches for long_segment_names =
true and long_segment_names = false are almost the same thus it will
not improve code coverage. Using the current numbers will allow to
easily switch back to long_segment_names = false in the test if
necessary.

PFA the corrected patchset v59.

--
Best regards,
Aleksander Alekseev

Hi again,

> PFA the corrected patchset v59.

On second thought, I believe this Assert is incorrect:

```
+    else
+    {
+        Assert(segno >= 0 && segno <= 0xFFFF);
+        return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
+                        (unsigned int) segno);
+    }
```

See SlruCorrectSegmentFilenameLength():

```
    if (ctl->long_segment_names)
        return (len == 15); /* see SlruFileName() */
    else
        /*
         * Commit 638cf09e76d allowed 5-character lengths. Later commit
         * 73c986adde5 allowed 6-character length.
         *
         * XXX should we still consider such names to be valid?
         */
        return (len == 4 || len == 5 || len == 6);
```

Should we just drop it or check that segno is <= 0xFFFFFF?

-- 
Best regards,
Aleksander Alekseev

Maxim,

I see both of us accounted for Alexanders feedback and submitted v59.
Your newer version seems to have issues on cfbot, so resubmitting the
previous patchset that passes the tests. Please feel free to add
changes.

> See SlruCorrectSegmentFilenameLength():
>
> ```
>     if (ctl->long_segment_names)
>         return (len == 15); /* see SlruFileName() */
>     else
>         /*
>          * Commit 638cf09e76d allowed 5-character lengths. Later commit
>          * 73c986adde5 allowed 6-character length.
>          *
>          * XXX should we still consider such names to be valid?
>          */
>         return (len == 4 || len == 5 || len == 6);
> ```
>
> Should we just drop it or check that segno is <= 0xFFFFFF?

I also choose to change this Assert and to add a corresponding comment:

        else
        {
-               Assert(segno >= 0 && segno <= 0xFFFF);
+               /*
+                * Despite the fact that %04X format string is used up to 24 bit
+                * integers are allowed. See SlruCorrectSegmentFilenameLength()
+                */
+               Assert(segno >= 0 && segno <= 0xFFFFFF);
                return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
                                                (unsigned int) segno);
        }


--
Best regards,
Aleksander Alekseev

Alexander, Maxim,

Thank you for revisions.

On Thu, Nov 9, 2023 at 6:22 PM Maxim Orlov <orlovmg@gmail.com> wrote:
> Aleksander Alekseev,
>
> > Maxim,
> > I see both of us accounted for Alexanders feedback and submitted v59.
> > Your newer version seems to have issues on cfbot, so resubmitting the
> > previous patchset that passes the tests. Please feel free to add
> > changes.
>
> For unknown reasons, I do not receive any of your emails from after 2023-11-07 11:57:12 (Message-ID:
CAJ7c6TN1hKqNPGrNcq96SUyD=Z61raKGXF8iqq36qr90oudxRg@mail.gmail.com).
> Even after resend.
>
> Anyway, PFA patch set of version 61.  I've made some minor changes in the 0001 and add 004 in order to test actual
64-bitSLRU pages. 
>
> As for CF bot had failed on my v59 patch set, this is because of the bug.  It's manifested because of added 64-bit
pagestests. 
> The problem was in segno calculation, since we convert it from file name using strtol call.  But strtol return long,
> which is 4 byte long in x86.
>
> -           segno = (int) strtol(clde->d_name, NULL, 16);
> +           segno = strtoi64(clde->d_name, NULL, 16);

v61 looks good to me.  I'm going to push it as long as there are no objections.

------
Regards,
Alexander Korotkov

Hello Alexander,

27.11.2023 02:43, Alexander Korotkov wrote:

> v61 looks good to me.  I'm going to push it as long as there are no objections.
>

I've looked at the patch set and found a typo:
occured

And a warning:
$ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-clobbered 
-Wno-missing-field-initializers" ./configure -q && make -s
slru.c:63:1: warning: ‘inline’ is not at beginning of declaration [-Wold-style-declaration]
    63 | static int      inline
       | ^~~~~~

Maybe it's worth fixing before committing...

Best regards,
Alexander

On 27/11/2023 01:43, Alexander Korotkov wrote:
> v61 looks good to me.  I'm going to push it as long as there are no objections.
This was discussed earlier, but is still present in v61:

> +/*
> + * An internal function used by SlruScanDirectory().
> + *
> + * Returns true if a file with a name of a given length may be a correct
> + * SLRU segment.
> + */
> +static inline bool
> +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
> +{
> +    if (ctl->long_segment_names)
> +        return (len == 15); /* see SlruFileName() */
> +    else
> +        /*
> +         * Commit 638cf09e76d allowed 5-character lengths. Later commit
> +         * 73c986adde5 allowed 6-character length.
> +         *
> +         * XXX should we still consider such names to be valid?
> +         */
> +        return (len == 4 || len == 5 || len == 6);
> +}
> +

I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6 
chars long. For pg_multixact/members, which introduced the 5-char case, 
I think we should always pad the filenames 5 characters, and for 
commit_ts which introduced the 6 char case, always pad to 6 characters.

Instead of a "long_segment_names" boolean, how about an integer field, 
to specify the length.

That means that we'll need pg_upgrade to copy pg_multixact/members files 
under the new names. That should be pretty straightforward.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Hi, Heikki!

On Tue, 28 Nov 2023 at 13:13, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 27/11/2023 01:43, Alexander Korotkov wrote:
> > v61 looks good to me.  I'm going to push it as long as there are no objections.
> This was discussed earlier, but is still present in v61:
>
> > +/*
> > + * An internal function used by SlruScanDirectory().
> > + *
> > + * Returns true if a file with a name of a given length may be a correct
> > + * SLRU segment.
> > + */
> > +static inline bool
> > +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
> > +{
> > +     if (ctl->long_segment_names)
> > +             return (len == 15); /* see SlruFileName() */
> > +     else
> > +             /*
> > +              * Commit 638cf09e76d allowed 5-character lengths. Later commit
> > +              * 73c986adde5 allowed 6-character length.
> > +              *
> > +              * XXX should we still consider such names to be valid?
> > +              */
> > +             return (len == 4 || len == 5 || len == 6);
> > +}
> > +
>
> I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6
> chars long. For pg_multixact/members, which introduced the 5-char case,
> I think we should always pad the filenames 5 characters, and for
> commit_ts which introduced the 6 char case, always pad to 6 characters.
>
> Instead of a "long_segment_names" boolean, how about an integer field,
> to specify the length.
>
> That means that we'll need pg_upgrade to copy pg_multixact/members files
> under the new names. That should be pretty straightforward.

I think what's done in patch 0001 is just an extension of existing
logic and moving it into separate function.

- if ((len == 4 || len == 5 || len == 6) &&
+ if (SlruCorrectSegmentFilenameLength(ctl, len) &&
  strspn(clde->d_name, "0123456789ABCDEF") == len)
  {
- segno = (int) strtol(clde->d_name, NULL, 16);
+ segno = strtoi64(clde->d_name, NULL, 16);
  segpage = segno * SLRU_PAGES_PER_SEGMENT;

I'd prefer to leave it as it is as a part of 64-bit extension patch.

Regards,
Pavel.

On 28/11/2023 12:14, Pavel Borisov wrote:
> On Tue, 28 Nov 2023 at 13:13, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 27/11/2023 01:43, Alexander Korotkov wrote:
>>> v61 looks good to me.  I'm going to push it as long as there are no objections.
>> This was discussed earlier, but is still present in v61:
>>
>>> +/*
>>> + * An internal function used by SlruScanDirectory().
>>> + *
>>> + * Returns true if a file with a name of a given length may be a correct
>>> + * SLRU segment.
>>> + */
>>> +static inline bool
>>> +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len)
>>> +{
>>> +     if (ctl->long_segment_names)
>>> +             return (len == 15); /* see SlruFileName() */
>>> +     else
>>> +             /*
>>> +              * Commit 638cf09e76d allowed 5-character lengths. Later commit
>>> +              * 73c986adde5 allowed 6-character length.
>>> +              *
>>> +              * XXX should we still consider such names to be valid?
>>> +              */
>>> +             return (len == 4 || len == 5 || len == 6);
>>> +}
>>> +
>>
>> I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6
>> chars long. For pg_multixact/members, which introduced the 5-char case,
>> I think we should always pad the filenames 5 characters, and for
>> commit_ts which introduced the 6 char case, always pad to 6 characters.
>>
>> Instead of a "long_segment_names" boolean, how about an integer field,
>> to specify the length.
>>
>> That means that we'll need pg_upgrade to copy pg_multixact/members files
>> under the new names. That should be pretty straightforward.
> 
> I think what's done in patch 0001 is just an extension of existing
> logic and moving it into separate function.

That's right. I'm arguing that now is a good time to clean it up.

I won't insist if Alexander prefers to commit it as it is, though. But 
let's at least explain how this works in the comment, instead of the XXX.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Hi,

>> > I think what's done in patch 0001 is just an extension of existing
>> > logic and moving it into separate function.
>>
>> That's right. I'm arguing that now is a good time to clean it up.
>>
>> I won't insist if Alexander prefers to commit it as it is, though. But
>> let's at least explain how this works in the comment, instead of the XXX.
>
> I agree with you that would be good to add a comment instead of XXX and commit.

+1

One could argue that getting rid of short filenames entirely in the
long term (i.e. always long_segment_names == true) could be a better
strategy. Maybe it's not but I believe this should be discussed
separately from the patchset under question.

-- 
Best regards,
Aleksander Alekseev

Hi!

On Tue, Nov 28, 2023 at 2:06 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> >> > I think what's done in patch 0001 is just an extension of existing
> >> > logic and moving it into separate function.
> >>
> >> That's right. I'm arguing that now is a good time to clean it up.
> >>
> >> I won't insist if Alexander prefers to commit it as it is, though. But
> >> let's at least explain how this works in the comment, instead of the XXX.
> >
> > I agree with you that would be good to add a comment instead of XXX and commit.
>
> +1
>
> One could argue that getting rid of short filenames entirely in the
> long term (i.e. always long_segment_names == true) could be a better
> strategy. Maybe it's not but I believe this should be discussed
> separately from the patchset under question.


Heikki, thank you for catching this.

This mess with file names formats already lasts quite long.  I don't
think we should hurry unifying this as long as we're anyway going to
change that in near future.

Please, find the revised patchset with relevant comment.

------
Regards,
Alexander Korotkov

Alexander Lakhin <exclusion@gmail.com> writes:
> And a warning:
> $ CC=gcc-12 CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-clobbered
> -Wno-missing-field-initializers" ./configure -q && make -s
> slru.c:63:1: warning: ‘inline’ is not at beginning of declaration [-Wold-style-declaration]
>     63 | static int      inline
>        | ^~~~~~

> Maybe it's worth fixing before committing...

This should have been fixed before commit, because there are now a
dozen buildfarm animals complaining about it, as well as who-knows-
how-many developers' compilers.

 calliphoridae | 2023-11-30 02:48:59 |
/home/bf/bf-build/calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is
notat beginning of declaration [-Wold-style-declaration] 
 canebrake     | 2023-11-29 14:22:10 |
/home/bf/bf-build/canebrake/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 culicidae     | 2023-11-30 02:49:06 |
/home/bf/bf-build/culicidae/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 desmoxytes    | 2023-11-30 03:11:15 |
/home/bf/bf-build/desmoxytes/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 flaviventris  | 2023-11-30 02:53:19 |
/home/bf/bf-build/flaviventris/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is
notat beginning of declaration [-Wold-style-declaration] 
 francolin     | 2023-11-30 02:26:08 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 grassquit     | 2023-11-30 02:58:36 |
/home/bf/bf-build/grassquit/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 komodoensis   | 2023-11-30 03:07:52 |
/home/bf/bf-build/komodoensis/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 phycodurus    | 2023-11-29 14:29:02 |
/home/bf/bf-build/phycodurus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not
atbeginning of declaration [-Wold-style-declaration] 
 piculet       | 2023-11-30 02:32:57 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 pogona        | 2023-11-29 14:22:31 |
/home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 rorqual       | 2023-11-30 02:32:41 | ../pgsql/src/backend/access/transam/slru.c:63:1: warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 serinus       | 2023-11-30 02:47:05 |
/home/bf/bf-build/serinus/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 skink         | 2023-11-29 14:23:05 |
/home/bf/bf-build/skink-master/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is
notat beginning of declaration [-Wold-style-declaration] 
 taipan        | 2023-11-30 03:03:52 |
/home/bf/bf-build/taipan/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 
 tamandua      | 2023-11-30 02:49:50 |
/home/bf/bf-build/tamandua/HEAD/pgsql.build/../pgsql/src/backend/access/transam/slru.c:63:1:warning: 'inline' is not at
beginningof declaration [-Wold-style-declaration] 

            regards, tom lane

On Thu, Nov 30, 2023 at 10:29 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> Agree. The fix is attached.

What an oversight.
Thank you, pushed!

------
Regards,
Alexander Korotkov

On Thu, Nov 30, 2023 at 4:37 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Thu, Nov 30, 2023 at 10:29 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > Agree. The fix is attached.
>
> What an oversight.
> Thank you, pushed!

With that, is there any more work pending, or can we close the CF entry?

On Mon, Dec 4, 2023 at 3:12 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> I think this is complete and could be closed.

Done.

Hi,

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=loach&dt=2023-12-16%2005%3A25%3A18

TRAP: failed Assert("epoch > 0"), File: "twophase.c", Line: 969, PID: 71030
0xa8edcd <ExceptionalCondition+0x6d> at
/usr/home/pgbf/buildroot/HEAD/pgsql.build/tmp_install/home/pgbf/buildroot/HEAD/inst/bin/postgres
0x613863 <ReadTwoPhaseFile+0x463> at
/usr/home/pgbf/buildroot/HEAD/pgsql.build/tmp_install/home/pgbf/buildroot/HEAD/inst/bin/postgres

That's the new assertion from 5a1dfde8:

+ * The wrap logic is safe here because the span of active xids cannot
exceed one
+ * epoch at any given time.
...
+       if (unlikely(xid > nextXid))
+       {
+               /* Wraparound occured, must be from a prev epoch. */
+               Assert(epoch > 0);

On Sun, Dec 17, 2023 at 1:48 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=loach&dt=2023-12-16%2005%3A25%3A18
>
> TRAP: failed Assert("epoch > 0"), File: "twophase.c", Line: 969, PID: 71030
> 0xa8edcd <ExceptionalCondition+0x6d> at
> /usr/home/pgbf/buildroot/HEAD/pgsql.build/tmp_install/home/pgbf/buildroot/HEAD/inst/bin/postgres
> 0x613863 <ReadTwoPhaseFile+0x463> at
> /usr/home/pgbf/buildroot/HEAD/pgsql.build/tmp_install/home/pgbf/buildroot/HEAD/inst/bin/postgres
>
> That's the new assertion from 5a1dfde8:
>
> + * The wrap logic is safe here because the span of active xids cannot
> exceed one
> + * epoch at any given time.
> ...
> +       if (unlikely(xid > nextXid))
> +       {
> +               /* Wraparound occured, must be from a prev epoch. */
> +               Assert(epoch > 0);

Thank you for noticing this.  I did some investigations.
AdjustToFullTransactionId() uses TransamVariables->nextXid to convert
TransactionId into FullTransactionId.  However,
ProcArrayApplyRecoveryInfo() first checks two phase transactions then
updates  TransamVariables->nextXid.  Please, see the draft patch
fixing this.  I'll do further check if it has some side-effects.

------
Regards,
Alexander Korotkov

On Tue, Nov 28, 2023 at 11:14 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6
> chars long. For pg_multixact/members, which introduced the 5-char case,
> I think we should always pad the filenames 5 characters, and for
> commit_ts which introduced the 6 char case, always pad to 6 characters.
>
> Instead of a "long_segment_names" boolean, how about an integer field,
> to specify the length.
>
> That means that we'll need pg_upgrade to copy pg_multixact/members files
> under the new names. That should be pretty straightforward.

Do you think it could be useful if the file names were not sequential
numbers ...0000, ...0001, ...0002 but instead used the 64 bit 'index'
number for the contained data?  In the cases where the index is an
fxid, such as pg_xact, pg_serial etc that seems easy to grok, and for
the multixacts or notify it's a bit more arbitrary but that's not
worse (and it is perhaps even meaningful, number of multixacts etc).
For example, pg_serial holds a uint64_t for every xid, so that's 32768
= 0x8000 xids per 256kB file, and you might see the following files on
disk:

0000000000000000
0000000000008000
0000000000010000

... so that it's very clear what fxid ranges are being held.  It might
also make the file unlinking logic more straightforward in the
non-modulo cases (not sure).  Of course you can work it out with
simple arithmetic but I wonder if human administrators who don't have
a C-level understanding of PostgreSQL would find this scheme more
cromulent when trying to understand, quickly, whether the system is
retaining expected data.

(Assuming we actually get the indexes to be 64 bit in the first place.
I started thinking/hacking around how to do that for the specific case of
pg_serial because it's [by its own admission] a complete mess right now,
and I was investigating its disk usage, see nearby thread, but then
I found my way here and realised I'm probably duplicating work that's
already been/being done so I'm trying to catch up here... forgive me if
the above was already covered, so many messages...)

On Mon, Nov 27, 2023 at 01:43:26AM +0200, Alexander Korotkov wrote:
> v61 looks good to me.  I'm going to push it as long as there are no objections.

This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
32-bit integers" and left some expressions coercing SLRU page numbers to int.
Two sources:

  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 | less -p '.int. from .int64. may alter
itsvalue'
 

(Not every match needs to change.)

> --- a/src/include/access/slru.h
> +++ b/src/include/access/slru.h

> @@ -127,7 +127,15 @@ typedef struct SlruCtlData
>       * the behavior of this callback has no functional implications.)  Use
>       * SlruPagePrecedesUnitTests() in SLRUs meeting its criteria.
>       */
> -    bool        (*PagePrecedes) (int, int);
> +    bool        (*PagePrecedes) (int64, int64);
> +
> +    /*
> +     * If true, use long segment filenames formed from lower 48 bits of the
> +     * segment number, e.g. pg_xact/000000001234. Otherwise, use short
> +     * filenames formed from lower 16 bits of the segment number e.g.
> +     * pg_xact/1234.
> +     */
> +    bool        long_segment_names;

SlruFileName() makes 15-character (60-bit) file names.  Where does the 48-bit
limit arise?  How does the SlruFileName() comment about a 24-bit limit for
short names relate this comment's 16-bit limit?

nm

Hi Noah,

> This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> 32-bit integers" and left some expressions coercing SLRU page numbers to int.
> Two sources:
>
>  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
>  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 | less -p '.int. from .int64. may alter
itsvalue' 
>
> (Not every match needs to change.)

I examined the new warnings introduced by 4ed8f09. Most of them seem
to be harmless, for instance:

```
slru.c:657:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  657 |         int                     rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

```
slru.c: In function ‘SlruReportIOError’:
slru.c:962:43: warning: conversion from ‘int64’ {aka ‘long int’} to
‘int’ may change value [-Wconversion]
  962 |         int                     rpageno = pageno %
SLRU_PAGES_PER_SEGMENT;
```

Interestingly the patch decreased the overall number of warnings.

I prepared the patch for clog.c. The rest of the warnings don't strike
me as something we should immediately act on unless we have a bug
report. Or perhaps there is a particular warning that worries you?

> > @@ -127,7 +127,15 @@ typedef struct SlruCtlData
> >        * the behavior of this callback has no functional implications.)  Use
> >        * SlruPagePrecedesUnitTests() in SLRUs meeting its criteria.
> >        */
> > -     bool            (*PagePrecedes) (int, int);
> > +     bool            (*PagePrecedes) (int64, int64);
> > +
> > +     /*
> > +      * If true, use long segment filenames formed from lower 48 bits of the
> > +      * segment number, e.g. pg_xact/000000001234. Otherwise, use short
> > +      * filenames formed from lower 16 bits of the segment number e.g.
> > +      * pg_xact/1234.
> > +      */
> > +     bool            long_segment_names;
>
> SlruFileName() makes 15-character (60-bit) file names.  Where does the 48-bit
> limit arise?  How does the SlruFileName() comment about a 24-bit limit for
> short names relate this comment's 16-bit limit?

Yes, this comment is wrong. Here is a fix.

[1]: https://www.postgresql.org/message-id/CAJ7c6TNMuKWUuMfh5KWgJJBoJGqPHYdZeN4t%2BLB6WdRLbDfVTw%40mail.gmail.com

--
Best regards,
Aleksander Alekseev

On Wed, Jun 26, 2024 at 02:09:58PM +0300, Aleksander Alekseev wrote:
> > This yielded commit 4ed8f09 "Index SLRUs by 64-bit integers rather than by
> > 32-bit integers" and left some expressions coercing SLRU page numbers to int.
> > Two sources:
> >
> >  grep -i 'int\b.*page' $(git grep -l SimpleLruInit)
> >  make && touch $(git grep -l SimpleLruInit) && make PROFILE=-Wconversion 2>&1 | less -p '.int. from .int64. may
alterits value'
 
> >
> > (Not every match needs to change.)
> 
> I examined the new warnings introduced by 4ed8f09. Most of them seem
> to be harmless, for instance:
[...]
> I prepared the patch for clog.c. The rest of the warnings don't strike
> me as something we should immediately act on unless we have a bug
> report. Or perhaps there is a particular warning that worries you?

Is "int" acceptable or unacceptable in the following grep match?

src/backend/commands/async.c:1274:    int            headPage = QUEUE_POS_PAGE(QUEUE_HEAD);

Hi,

> > I prepared the patch for clog.c. The rest of the warnings don't strike
> > me as something we should immediately act on unless we have a bug
> > report. Or perhaps there is a particular warning that worries you?
>
> Is "int" acceptable or unacceptable in the following grep match?
>
> src/backend/commands/async.c:1274:      int                     headPage = QUEUE_POS_PAGE(QUEUE_HEAD);

Good catch. We better use int64s here.

Here is the corrected patchset.

-- 
Best regards,
Aleksander Alekseev

Hi,

> Here is the corrected patchset.

TWIMC this is currently listed as an open item for PG17 [1].
Sorry if everyone interested is already aware.

[1]: https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items

-- 
Best regards,
Aleksander Alekseev

On Mon, Jul 08, 2024 at 12:30:09PM +0300, Aleksander Alekseev wrote:
> TWIMC this is currently listed as an open item for PG17 [1].
> Sorry if everyone interested is already aware.
>
> [1]: https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items

The proposed patch looks rather incomplete to me, based on the fact
that this stuff has a lot of inconsistencies with the types used when
manipulating 64b SLRU pages.  Some of them are harder to catch as the
variables don't specifically refer to pages.

So, even after v2, there are two more of these in asyncQueueUsage()
with the two QUEUE_POS_PAGE() for the head and tail positions:
    int         headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
    int         tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);

asyncQueueReadAllNotifications() also has one:
int         curpage = QUEUE_POS_PAGE(pos);

asyncQueueAdvanceTail() declares the following:
    int         oldtailpage;
    int         newtailpage;
    int         boundary;

AsyncQueueControl.stopPage is an int.

And that's only for async.c.  Alexander K., as the owner of the open
item, are you planning to look at that?
--
Michael

Hi,

> The proposed patch looks rather incomplete to me, based on the fact
> that this stuff has a lot of inconsistencies with the types used when
> manipulating 64b SLRU pages.  Some of them are harder to catch as the
> variables don't specifically refer to pages.
>
> So, even after v2, there are two more of these in asyncQueueUsage()
> with the two QUEUE_POS_PAGE() for the head and tail positions:
>     int         headPage = QUEUE_POS_PAGE(QUEUE_HEAD);
>     int         tailPage = QUEUE_POS_PAGE(QUEUE_TAIL);
>
> asyncQueueReadAllNotifications() also has one:
> int         curpage = QUEUE_POS_PAGE(pos);
>
> asyncQueueAdvanceTail() declares the following:
>     int         oldtailpage;
>     int         newtailpage;
>     int         boundary;
>
> AsyncQueueControl.stopPage is an int.
>
> And that's only for async.c.  Alexander K., as the owner of the open
> item, are you planning to look at that?

Thanks, Michael. I prepared a corrected patchset.

-- 
Best regards,
Aleksander Alekseev

On Thu, Jul 11, 2024 at 01:11:05PM +0300, Aleksander Alekseev wrote:
> Thanks, Michael. I prepared a corrected patchset.

A comment about v3-0001.

-     * If true, use long segment filenames formed from lower 48 bits of the
-     * segment number, e.g. pg_xact/000000001234. Otherwise, use short
-     * filenames formed from lower 16 bits of the segment number e.g.
-     * pg_xact/1234.
+     * If true, use long segment filenames. Use short filenames otherwise.
+     * See SlruFileName().

We're losing some details here even if SlruFileName() has some
explanations, because one would need to read through the snprintf's
04X to know that short file names include 4 characters.  I'm OK to
mention SlruFileName() rather than duplicate the knowledge here, but
SlruFileName() should also be updated to mention the same level of
details with some examples of file names.
--
Michael

Hi,

> A comment about v3-0001.
>
> -        * If true, use long segment filenames formed from lower 48 bits of the
> -        * segment number, e.g. pg_xact/000000001234. Otherwise, use short
> -        * filenames formed from lower 16 bits of the segment number e.g.
> -        * pg_xact/1234.
> +        * If true, use long segment filenames. Use short filenames otherwise.
> +        * See SlruFileName().
>
> We're losing some details here even if SlruFileName() has some
> explanations, because one would need to read through the snprintf's
> 04X to know that short file names include 4 characters.  I'm OK to
> mention SlruFileName() rather than duplicate the knowledge here, but
> SlruFileName() should also be updated to mention the same level of
> details with some examples of file names.

Fair enough. Here is the updated patchset.


--
Best regards,
Aleksander Alekseev

On Fri, Jul 12, 2024 at 12:44:54PM +0300, Aleksander Alekseev wrote:
> Fair enough. Here is the updated patchset.

Hearing nothing but cicadas as now is their season, I have taken the
initiative here to address this open item.

0001 felt a bit too complicated in slru.h, so I have simplified it and
kept all the details in slru.c with SlruFileName().

I have reviewed all the code that uses SLRUs, and spotted three more
problematic code paths in predicate.c that needed an update like the
others for some pagenos.  I've added these, and applied 0002.  We
should be good now.
--
Michael

Hi,

> Hearing nothing but cicadas as now is their season, I have taken the
> initiative here to address this open item.
>
> 0001 felt a bit too complicated in slru.h, so I have simplified it and
> kept all the details in slru.c with SlruFileName().
>
> I have reviewed all the code that uses SLRUs, and spotted three more
> problematic code paths in predicate.c that needed an update like the
> others for some pagenos.  I've added these, and applied 0002.  We
> should be good now.

Thank you!

-- 
Best regards,
Aleksander Alekseev

On Tue, Jul 23, 2024 at 06:01:44PM +0900, Michael Paquier wrote:
> Hearing nothing but cicadas as now is their season, I have taken the
> initiative here to address this open item.
> 
> 0001 felt a bit too complicated in slru.h, so I have simplified it and
> kept all the details in slru.c with SlruFileName().
> 
> I have reviewed all the code that uses SLRUs, and spotted three more
> problematic code paths in predicate.c that needed an update like the
> others for some pagenos.  I've added these, and applied 0002.  We
> should be good now.

I'm still seeing need for s/int/int64/ at:

- "pagesegno" variable
- return value of MultiXactIdToOffsetSegment()
- return value of MXOffsetToMemberSegment()
- callers of previous two

Only the first should be a live bug, since multixact isn't electing the higher
pageno ceiling.

On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote:
> I'm still seeing need for s/int/int64/ at:

Nice catches!  I've missed these.

> - "pagesegno" variable
> - return value of MultiXactIdToOffsetSegment()

Only used in four places for two elog(DEBUG1) entries with %x.

> - return value of MXOffsetToMemberSegment()

Also used in four places for two elog(DEBUG1) entries with %x, plus
three callers in PerformMembersTruncation(), nothing fancy.

> Only the first should be a live bug, since multixact isn't electing the higher
> pageno ceiling.

Yes, and it makes a switch to long segment names everywhere easier.
There is a patch in the air to do that, without the pg_upgrade
additions required to do the transfer, though.

I am attaching a patch for all these you have spotted, switching the
logs to use %llx.  Does that look fine for you?
--
Michael

On Thu, Jul 25, 2024 at 10:52:13AM +0900, Michael Paquier wrote:
> On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote:
> > I'm still seeing need for s/int/int64/ at:

> I am attaching a patch for all these you have spotted, switching the
> logs to use %llx.  Does that look fine for you?

Yes.  I think that completes the project.

On Fri, Jul 26, 2024 at 3:42 AM Noah Misch <noah@leadboat.com> wrote:
> On Thu, Jul 25, 2024 at 10:52:13AM +0900, Michael Paquier wrote:
> > On Wed, Jul 24, 2024 at 06:00:59AM -0700, Noah Misch wrote:
> > > I'm still seeing need for s/int/int64/ at:
>
> > I am attaching a patch for all these you have spotted, switching the
> > logs to use %llx.  Does that look fine for you?
>
> Yes.  I think that completes the project.

Thanks to everybody for working on this.  It's pity I didn't notice
this is v17 open item on me.  Sorry for this.

I tool a look on commits and on other slru code for similar issue.
Everything looks good for me.

------
Regards,
Alexander Korotkov
Supabase

On Fri, Jul 26, 2024 at 11:50:48PM +0300, Alexander Korotkov wrote:
> Thanks to everybody for working on this.  It's pity I didn't notice
> this is v17 open item on me.  Sorry for this.

No problem.  I've just applied now the remaining pieces down to 17.
--
Michael

On Sat, Jul 27, 2024 at 07:24:33AM +0900, Michael Paquier wrote:
> I've just applied now the remaining pieces down to 17.

Comparing commit c9e2457 to the patch in ZqGvzSbW5TGKqZcE@paquier.xyz, the
commit lacks the slru.c portion.