Обсуждение: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Eisentraut
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
On Thu, 17 Mar 2022 at 21:31, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Thu, Mar 17, 2022 at 4:23 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> On 17.03.22 14:12, Aleksander Alekseev wrote:
>> > v19-0001 changes the format string for XIDs from %u to XID_FMT. This
>> > refactoring allows us to switch to UINT64_FORMAT by changing one
>> > #define in the future patches.
>> >
>> > Kyotaro suggested using `errmsg("blah blah %lld ..", (long long)
>> > xid)` instead in order to simplify localization of the error messages.
>> > Personally I don't have a strong opinion here. Either approach will
>> > work and will affect the error messages eventually. Please let us know
>> > what you think.
>>
>> This is not a question of simplification. Translatable messages with
>> embedded macros won't work. This patch isn't going to be acceptable.
>
> I've suspected this, but wasn't sure. Thank you for clarification.
Hi, hackers!
The need to support localization is very much understood by us. We'll deliver a patchset soon with localization based on %lld/%llu format and explicit casts to unsigned/signed long long.
Alexander Alexeev could you wait a little bit and give us time to deliver v20 patch which will address localization (I propose concurrent work should stop until that to avoid conflicts)
Advice and discussion help us a lot.
Thanks!
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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.
Reviews and proposals are very welcome!
--
Best regards,
Maxim Orlov.
Вложения
At Thu, 17 Mar 2022 19:25:00 +0300, Maxim Orlov <orlovmg@gmail.com> wrote in > Hi! > > Here is the v20 patch. 0001 and 0002 are proposed into PG15 as > discussed above. > The whole set of patches is added into [1] to be committed into PG16. > > In this version we've made a major revision related to printf/elog format > compatible with localization > as was proposed above. > > We also think of adding 0003 patch related to 64 bit GUC's into this > thread. Suppose it also may be delivered > into PG15. > > Aleksander Alekseev, we've done this major revision mentioned above and you > are free to continue working on this patch set. > > Reviews and proposals are very welcome! (I'm afraid that this thread is not for the discussion of the patch?:) > [1] > 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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
Hi hackers,
> Aleksander Alekseev, we've done this major revision mentioned above and you are free to continue working on this patch set.
>
> Reviews and proposals are very welcome!
Many thanks!
Here is an new version with the following changes compared to v20:
- Commit messages and links to the discussions were updated;
- XID_TYPE name seemed to be slightly misleading. I changed it to XID_FMT_TYPE. Not 100% sure if we really need this typedef though. If not, XID_FMT_TYPE is easy to replace in the .patch files. Same for XID32_SCANF_FMT definition;
- I noticed that pg_resetwal.c continues to use %u to format XIDs. Fixed;
- Since v20-0001 modifies gettext() arguments, I'm pretty sure the corresponding .po files should be modified as well. I addressed this in a separate patch in order to simplify the review;
To me personally v21 looks almost OK. The comments in c.h should be rewritten depending on whether we choose to keep XID_FMT_TYPE and/or XID32_SCANF_FMT. The patchset passes all the tests.
(As a side note, it looks like cfbot was slightly confused by forking the thread and modifying the CF entry. It couldn't find v20. If somebody knows how to fix this, please help.)
--
Best regards,
Aleksander Alekseev
> Aleksander Alekseev, we've done this major revision mentioned above and you are free to continue working on this patch set.
>
> Reviews and proposals are very welcome!
Many thanks!
Here is an new version with the following changes compared to v20:
- Commit messages and links to the discussions were updated;
- XID_TYPE name seemed to be slightly misleading. I changed it to XID_FMT_TYPE. Not 100% sure if we really need this typedef though. If not, XID_FMT_TYPE is easy to replace in the .patch files. Same for XID32_SCANF_FMT definition;
- I noticed that pg_resetwal.c continues to use %u to format XIDs. Fixed;
- Since v20-0001 modifies gettext() arguments, I'm pretty sure the corresponding .po files should be modified as well. I addressed this in a separate patch in order to simplify the review;
To me personally v21 looks almost OK. The comments in c.h should be rewritten depending on whether we choose to keep XID_FMT_TYPE and/or XID32_SCANF_FMT. The patchset passes all the tests.
(As a side note, it looks like cfbot was slightly confused by forking the thread and modifying the CF entry. It couldn't find v20. If somebody knows how to fix this, please help.)
--
Best regards,
Aleksander Alekseev
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
Hi hackers,
> Here is an new version with the following changes compared to v20:
For some reason I didn't receive the recent e-mails from Tom and Kyotaro. I've just discovered them accidentally by reading the thread through the web interface. These comments were not addressed in v21.
--
Best regards,
Aleksander Alekseev
> Here is an new version with the following changes compared to v20:
> [...]
> To me personally v21 looks almost OK.
--
Best regards,
Aleksander Alekseev
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
> To me personally v21 looks almost OK.For some reason I didn't receive the recent e-mails from Tom and Kyotaro. I've just discovered them accidentally by reading the thread through the web interface. These comments were not addressed in v21.
Aleksander, as of now we're preparing a new version that addresses a thing mentioned by Tom&Kyotaro. We'll try to add what you've done in v21, but please check. We're going to send a patch very soon, most probably today in several hours.
--
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
Here is v22. It addresses things mentioned by Tom and Kyotaro. Also added Aleksander's changes from v21.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Andres Freund
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
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
We'll try to add these and many similar changes in Slru code, thanks!
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Eisentraut
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
> I think there needs to be a bit more soul searching here on how to
> handle that in the future and how to transition it. I don't think
> targeting this patch for PG15 is useful at this point.
The patches can be reordered so that we are still able to deliver SLRU
refactoring in PG15.
Sure.
> As a more general point, I don't like plastering these bulky casts all
> over the place. Casts hide problems.
Regarding the casts, I don't like them either. I agree that it could
be a good idea to invest a little more time into figuring out if this
transit can be handled in a better way and deliver this change in the
next CF. However, if no one will be able to suggest a better
alternative, I think we should continue making progress here. The
32-bit XIDs are a major inconvenience for many users.
I'd like to add that the initial way of shifting to 64bit was based on XID_FMT in a print formatting template which could be changed from 32 to 64 bit when shifting to 64-bit xids later. But this template is not localizable so hackers recommended using %lld/%llu with (long long)/(unsigned long long cast) which is a current best practice elsewhere in the code (e.g. recent 1f8bc448680bf93a9). So I suppose we already have a good enough way to stick to.
This approach in 0001 inherently processes both 32/64 bit xids and should not change with later committing 64bit xids later (https://postgr.es/m/CACG%3DezZe1NQSCnfHOr78AtAZxJZeCvxrts0ygrxYwe%3DpyyjVWA%40mail.gmail.com)
The thing that needs to change then is suppressing output of Epoch. It should be done when 64-xids are committed and it is done by 0006 in the mentioned thread. Until that I've left Epoch in the output.
Big thanks for your considerations!
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
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.
Splitting 64 bit XIDs into a bunch of patches was done to simplify reviewing and making commits in small portions. We have little overhead here like removing Epoch later and now changes are based on the fact that Epoch still exists.
In the SLRU patch we have changed SLRU page numbering from int to int64. There were proposals to make use of SLRU pages numbers that are in fact unsigned and change from int to uint64. I fully support this, but I'm not sure this big SLRU refactoring should be done in this patchset. On the other hand it seems logical to change everything in SLRU at once. I think I need a second opinion in support of this change.
In general, I consider this patchset is ready to commit. It would be great to deliver it in PG15.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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!
--
Best regards,
Maxim Orlov.
Вложения
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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)));
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Eisentraut
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
Here is the v26 patchset. Main changes:
- back to signed int in SLRU pages
- fix printing epoch and xid as single value
- SlruFileName is not changed, thus no need special procedure in pg_upgrade
- remove cast from SlruFileName
- refactoring macro SlruFileName into inline function
Reviews are very welcome!
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
Hi, Peter!
Thanks for your review!
About v25-0001-Use-unsigned-64-bit-numbering-of-SLRU-pages.patch:
-static bool CLOGPagePrecedes(int page1, int page2);
+static bool CLOGPagePrecedes(uint64 page1, uint64 page2);
You are changing the API from signed to unsigned. Is this intentional?
It is not explained anywhere. Are we sure that nothing uses, for
example, negative values as error markers?
Initially, we've made SLRU pages to be int64, and reworked them into uint64 as per Andres Freund's proposal. It is not necessary for a 64xid patchset though as maximum page number is at least several (>2) times less than the maximum 64bit xid value. So we've returned them to be signed int64. I don't see the reason why make SRLU unsigned for introducing 64bit xids.
#define SlruFileName(ctl, path, seg) \
- snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir, seg)
+ snprintf(path, MAXPGPATH, "%s/%04X%08X", (ctl)->Dir, \
+ (uint32) ((seg) >> 32), (uint32) ((seg) &
(uint64)0xFFFFFFFF))
What's going on here? Some explanation? Why not use something like
%llX or whatever you need?
Of course, this should be simplified as %012llX and we will do this in later stage (in 0006 patch in 64xid thread) as this should be done together with CLOG pg_upgrade. So we've returned this to the initial state in 0001. Thanks for the notion!
+ uint64 segno = pageno / SLRU_PAGES_PER_SEGMENT;
+ uint64 rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
[etc.]
Not clear whether segno etc. need to be changed to 64 bits, or whether
an increase of SLRU_PAGES_PER_SEGMENT should also be considered.
Yes, segno should be 64bits because even multiple of SLRU_PAGES_PER_SEGMENT and CLOG_XACTS_PER_PAGE (and similar for commit_ts and mxact) is far less than 2^32 and the overall length of clog/commit_ts/mxact is 64bit.
- if ((len == 4 || len == 5 || len == 6) &&
+ if ((len == 12 || len == 13 || len == 14) &&
This was horrible before, but maybe we can take this opportunity now to
add a comment?
This should also be introduced later together with SlruFileName changes. So we've removed this change from 0001. Later in 0006 we'll add this with comments.
- SlruFileName(ctl, path, ftag->segno);
+ SlruFileName(ctl, path, (uint64) ftag->segno);
Maybe ftag->segno should be changed to 64 bits as well? Not clear.
Same as previous.
There is a code comment at the definition of SLRU_PAGES_PER_SEGMENT that
has some detailed explanations of how the SLRU numbering, SLRU file
names, and transaction IDs tie together. This doesn't seem to apply
anymore after this change.
Same as previous.
The reference page of pg_resetwal contains various pieces of information
of how to map SLRU files back to transaction IDs. Please check if that
is still all up to date.
Same as previous.
About v25-0002-Use-64-bit-format-to-output-XIDs.patch:
I don't see the point of applying this now. It doesn't make PG15 any
better. It's just a patch part of which we might need later.
Especially the issue of how we are handwaving past the epoch-enabled
output sites disturbs me. At least those should be omitted from this
patch, since this patch makes these more wrong, not more right for the
future.
psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
- xmax,
+ psprintf("xmax %llu equals or exceeds next valid transaction ID %u:%llu",
+ (unsigned long long) xmax,
EpochFromFullTransactionId(ctx->next_fxid),
- XidFromFullTransactionId(ctx->next_fxid)));
+ (unsigned long long) XidFromFullTransactionId(ctx->next_fxid)));
This %u:%u business is basically an existing workaround for the lack of
64-bit transaction identifiers. Presumably, when those are available,
all of this will be replaced by a single number display, possibly a
single %llu. So these sites you change here will have to be touched
again, and so changing this now doesn't make sense. At least that's my
guess. Maybe there needs to be a fuller explanation of how this is
meant to be transitioned.
Fixed, thanks.
An alternative we might want to consider is that we use PRId64 as
explained here:
<https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html>.
We'd have to check whether we still support any non-GNU gettext
implementations and to what extent they support this. But I think it's
something to think about if we are going to embark on a journey of much
more int64 printf output.
There were several other ways that have met opposition above in the thread. I guess PRId64 will also be opposed as not portable enough. Personally, I don't see much trouble when we cast the value to be printed to more wide value and consider this the best choice of all that was discussed. We just stick to a portable way of printing which was recommended by Tom and in agreement with 1f8bc448680bf93a974cb5f5
In [1] we initially proposed a 64xid patch to be committed at once. But it appeared that a patch of this complexity can not be reviewed at once. It was proposed in [1] that we'd better cut it into separate threads and commit by parts, some into v15, the other into v16. So we did. In view of this, I can not accept that 0002 patch doesn't make v15 better. I consider it is separate enough to be committed as a base to further 64xid parts.
Anyway, big thanks for the review, which is quite useful!
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
Just forgot to mention that answers in a previous message are describing the changes that are in v26.
--
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Sorry, I forgot to change pg_amcheck tests to correspond to the removed epoch from output in 0002.
Fixed in v27.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
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!
Вложения
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
+SlruScanDirCbFindEarliest(SlruCtl ctl, char *filename, int64 segpage, void *data)
segpage doesn't fit mxtruncinfo.earliestExistingPage. Doesn't it need
to be int64?
I think yes, fixed. Thanks!
+ 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".
I did it the first way. I moved the actual change of segment file name in the next patches that are to be committed in v16 or later.
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.
Fixed. Thanks!
-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.
Fixed. Thanks!
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?
Since it is not related to making XIDs 64 bit it is addressed in the separate thread [1].
-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.
Fixed. Thanks!
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.
Same as "%s/%04llX" issues mentioned above. Moved to the next patches.
-/* 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.)
Same as above.
Thanks for your review!
Here is a new patchset v29.
Major changes:
- fixes from review by Kyotaro mentioned above
- 0002 is split into two patches: 0002 is change output XIDs format only, 0003 is get rid of epoch in output
- 0003 includes changes in controldata file format in order to support both formats: old format with epoch and new as FullTransactionId
I'm not sure if it is worth it at this stage to change pg_resetwal handling on epoch (for example, remove -e option and so on) or do it later?
Opinions are welcome!
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Sorry, I forgot to append a fix for FileTag in v29. Here is v30.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Eisentraut
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Eisentraut
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
Here is rebased version of a patch with minor improvements.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
To keep this thread up to date with [1], here is the rebased v32 version of the patch.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
> here is the rebased v32 version of the patch.
The patchset rotted a bit. Here is a rebased version.
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.
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
Hi hackers, Here is the rebased patchset. -- Best regards, Aleksander Alekseev
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Here is the rebased patchset.
Hi!
While working on v40 of 64-bit XID patch [1], we've noticed a couple of forgotten things in v34 in this thread.
So, update the patchset to v40 from the thread [1].
It seems convenient to use common numbering of versions in this thread and the thread [1].
So, please, don't be surprised to see v40 here just after v34.
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Justin Pryzby
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
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).
I see the main goal of this split is to make discussion of this (easier) thread separate to the discussion of a whole patchset which is expected to be more thorough.
Also I see the chances of this thread to be committed into v16 to be much higher than of a main patch, which will be for v17 then.
Thanks for the advice to add git thread instead of patch posting. Will try to do this.
Best regards,
Pavel Borisov
Pavel Borisov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Simon Riggs
Дата:
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/
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Hamid Akhtar
Дата:
On Tue, 26 Jul 2022 at 21:35, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
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.
This set of patches no longer applies cleanly to the master branch. There are lots of
hunks as well as failures. Please rebase the patches.
There are failures for multiple files including the one given below:
patching file src/backend/replication/logical/worker.c
Hunk #1 succeeded at 1089 (offset 1 line).
Hunk #2 succeeded at 1481 (offset 1 line).
Hunk #3 succeeded at 3322 (offset 2 lines).
Hunk #4 succeeded at 3493 (offset 2 lines).
Hunk #5 FAILED at 4009.
1 out of 5 hunks FAILED -- saving rejects to file src/backend/replication/logical/worker.c.rej
hunks as well as failures. Please rebase the patches.
There are failures for multiple files including the one given below:
patching file src/backend/replication/logical/worker.c
Hunk #1 succeeded at 1089 (offset 1 line).
Hunk #2 succeeded at 1481 (offset 1 line).
Hunk #3 succeeded at 3322 (offset 2 lines).
Hunk #4 succeeded at 3493 (offset 2 lines).
Hunk #5 FAILED at 4009.
1 out of 5 hunks FAILED -- saving rejects to file src/backend/replication/logical/worker.c.rej
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
Here is a rebased version of the patch set.
Major changes are:
1. Fix rare replica fault.
Upon page pruning in heap_page_prune, page fragmentation repair is determined by
a parameter repairFragmentation. At the same time, on a replica, upon handling XLOG_HEAP2_PRUNE record type
in heap_xlog_prune, we always call heap_page_prune_execute with repairFragmentation parameter equal to true.
This caused page inconsistency and lead to the crash of the replica. Fix this by adding new flag in
struct xl_heap_prune.
2. Add support for meson build.
3. Add assertion "buffer is locked" in HeapTupleCopyBaseFromPage.
4. Add assertion "buffer is locked exclusive" in heap_page_shift_base.
5. Prevent excessive growth of xmax in heap_prepare_freeze_tuple.
As always, reviews are very welcome!
--
Best regards,
Maxim Orlov.
Вложения
- v47-0001-Use-64-bit-numbering-of-SLRU-pages.patch
- v47-0004-Use-64-bit-pages-representation-in-SLRU-callers.patch
- v47-0002-Use-64-bit-format-to-output-XIDs.patch
- v47-0003-Use-64-bit-FullTransactionId-instead-of-Epoch-xi.patch
- v47-0005-Add-initdb-option-to-initialize-cluster-with-non.patch
- v47-0007-Use-64-bit-GUCs.patch
- v47-0006-README.XID64.patch
- v47-0008-Use-64-bit-XIDs.patch
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
This is the wrong thread / CF entry. Please see
Yep, my fault. Sorry about that.
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Ian Lawrence Barwick
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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.
--
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Dilip Kumar
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Ian Lawrence Barwick
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
Hi Michael, > The CF bot is showing some failures here. You may want to > double-check. Thanks! PFA v48. -- Best regards, Aleksander Alekseev
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Andres Freund
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Peter Geoghegan
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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.
Should we change status for this thread to "need review"?
--
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Andrey Borodin
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
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?
After applying the whole patch set, SLRU will become 64–bit without a wraparound. Thus, no wraparound
should be there.
0001 - should make SLRU internally 64–bit, no effects from "outside"
0002 - should make SLRU callers 64–bit, SLRU segment files naming are changed
0003 - make upgrade from previous versions feasible
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...
As we did discuss somewhere in the beginning of the discussion, we try to make every commit as independent as possible.
Thus, it is much easier to review and commit. I see no problem to meld these commits into one, if consensus will be reached.
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?
The purpose of the third patch is to make upgrade feasible. Since we've change pg_xact files naming,
Postgres could not read status of "old" transactions from "old" pg_xact files. So, we have to convert those files.
The major problem here is that we must handle possible segment wraparound (in "old" cluster). The whole idea
for an upgrade is to read SLRU pages for pg_xact one by one and write it in a "new" filename.
Maybe, It's just a little bit complicated, since the algorithm is intended to deal with different SLRU pages per segment
in "new" and "old" clusters. But, on the other hand, it is already created in original patch set of 64–bit XIDs and will be useful
in the future. AFAICS, arguably, any variant of 64–bit XIDs should lead to increase of an amount of SLRU pages per segment.
And as for other SLRUs, they cannot survive pg_upgrade mostly by the fact, that cluster must be stopped upon upgrade.
Thus, no conversion needed.
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Fri, 6 Jan 2023 at 09:51, Japin Li <japinli@hotmail.com> wrote:
For v51-0003. We can use GetClogDirName instead of GET_MAJOR_VERSION in
copy_subdir_files().
Of course! Tanks! I'll address this in the next iteration, v52.
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
Here is a new patch set.
I've added comments and make use GetClogDirName call in copy_subdir_files.--
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Jacob Champion
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Tue, 21 Feb 2023 at 16:59, Aleksander Alekseev <aleksander@timescale.com> wrote:
Hi,
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.
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Heikki Linnakangas
Дата:
(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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Mon, 27 Feb 2023 at 12:10, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
(CC list trimmed, gmail wouldn't let me send otherwise)
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.
Yep, that's my fault. I've forgotten about pg_multixacts. But for the pg_commit_ts it's a bit complicated story.
The thing is, if we do upgrade, old files from pg_commit_ts not copied into a new server.
For example, I've checked one more time on the current master branch:
1). initdb
2). add "track_commit_timestamp = on" into postgresql.conf
3). pgbench
4). $ ls pg_commit_ts/
0000 0005 000A 000F 0014 0019 001E 0023...
0000 0005 000A 000F 0014 0019 001E 0023...
...009A 009F 00A4 00A9 00AE 00B3 00B8 00BD 00C2
5). do pg_upgrade
6). $ ls pg_commit_ts/
0000 00C2
0000 00C2
Either I do not understand something, or the files from pg_commit_ts directory are not copied.
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Heikki Linnakangas
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Heikki Linnakangas
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Tue, 17 Jan 2023 at 16:33, Aleksander Alekseev <aleksander@timescale.com> wrote:
Hi hackers,Maxim, perhaps you could share with us what your reasoning was here?
I'm really sorry for late response, but better late than never. Yes, we can not access shared memory without lock.
In this particular case, we use XidGenLock. That is why we use lock argument to take it is it was not taken previously.
Actually, we may place assertion in this insist.
As for xid compare: we do not compare xids here, we are checking for wraparound, so, AFAICS, this code is correct.
On Mon, 6 Mar 2023 at 22:48, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
1. Use 64 bit page numbers in SLRUs (this patch)
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_multixact so that pg_multixact/members is addressed by
64-bit offsets.
4. Extend pg_subtrans to 64-bits.
5. Extend pg_xact to 64-bits.
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.
...
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
Yes, this is a great idea! My only concern here is that we're going in circles here. You see, patch 1 is what was proposed
in the beginning of this thread. Anyway, I will be happy if we are being able to push this topic forward.
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.
In pg_xact we do not have such a problem, we do have epoch for transacions, so conversion should be pretty obvious:
0000 -> 000000000000
0001 -> 000000000001
...
0FFE -> 000000000FFE
0FFF -> 000000000FFF
0000 -> 000000010000
0001 -> 000000010001
So, in my view, the plan should be:
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)
Thoughts?
Best regards,
Maxim Orlov.
On Mon, 6 Mar 2023 at 22:48, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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
--
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Heikki Linnakangas
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Tue, 7 Mar 2023 at 15:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
Yes, you're totally correct. If it will be committable that way, I'm all for that.
Best regards,
Maxim Orlov.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Hi!
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.
--
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Heikki Linnakangas
Дата:
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)
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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. -- Best regards, Aleksander Alekseev
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
> > 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.
Good idea
> > 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.
Good idea
发件人: Aleksander Alekseev <aleksander@timescale.com>
发送时间: 2023年9月4日 22:41
收件人: Postgres hackers <pgsql-hackers@lists.postgresql.org>
抄送: Heikki Linnakangas <hlinnaka@iki.fi>; Maxim Orlov <orlovmg@gmail.com>; Jacob Champion <jchampion@timescale.com>; Japin Li <japinli@hotmail.com>; Andres Freund <andres@anarazel.de>; Michael Paquier <michael@paquier.xyz>; Pavel Borisov <pashkin.elfe@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; Alexander Korotkov <aekorotkov@gmail.com>
主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
发送时间: 2023年9月4日 22:41
收件人: Postgres hackers <pgsql-hackers@lists.postgresql.org>
抄送: Heikki Linnakangas <hlinnaka@iki.fi>; Maxim Orlov <orlovmg@gmail.com>; Jacob Champion <jchampion@timescale.com>; Japin Li <japinli@hotmail.com>; Andres Freund <andres@anarazel.de>; Michael Paquier <michael@paquier.xyz>; Pavel Borisov <pashkin.elfe@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; Alexander Korotkov <aekorotkov@gmail.com>
主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
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
> > 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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
Hi!
> 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 64 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 simplification and 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 typo in a word "exceeed".
- ((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 typo in a word "exceeed".
+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 same files.
+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 same files.
+ 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 in extreme cases. But I still think it will more safe and easier to read to write "occupied >= max_notify_queue"_pages here.
diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
I'm not sure if the current code could actually allow to occupy more than max_notify_queue_pages. Probably not even in extreme cases. But I still think it will more safe and easier to read to write "occupied >= max_notify_queue"_pages here.
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_notify actually use higher than 2**32 page numbers. Thus, I think test/modules/test_slru is a good place to give high page numbers a good test.
------
Regards,
Alexander Korotkov
------
Regards,
Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
On Mon, Nov 6, 2023 at 4:38 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
> > > 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 same files.
>
> Sorry, I didn't understand this one. Maybe you could provide the exact code?
I actually meant this.
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.
*/
Assert(segno >= 0 && segno <= 0x1000000000000000);
return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
(long long) segno);
}
else
{
Assert(segno >= 0 && segno <= 0x10000);
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
(unsigned int) segno);
}
}
As I now get, snprintf() wouldn't just truncate the high signs, instead it will use more characters. But I think assertions are useful anyway.
------
Regards,
Alexander Korotkov
> > > 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 same files.
>
> Sorry, I didn't understand this one. Maybe you could provide the exact code?
I actually meant this.
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.
*/
Assert(segno >= 0 && segno <= 0x1000000000000000);
return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
(long long) segno);
}
else
{
Assert(segno >= 0 && segno <= 0x10000);
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
(unsigned int) segno);
}
}
As I now get, snprintf() wouldn't just truncate the high signs, instead it will use more characters. But I think assertions are useful anyway.
------
Regards,
Alexander Korotkov
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
On Mon, 6 Nov 2023 at 16: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.
Hi! Great news!
BTW, there is a typo in a word "exceeed".
Fixed.
+static int inline
+SlruFileName(SlruCtl ctl, char *path, int64 segno)
+{
...
+}
I think it worth adding asserts here to verify there is no overflow making us mapping different segments into the same files.
Agree, assertion added.
+ 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 in extreme 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.cThe actual 64-bitness of SLRU pages isn't much exercised in our automated tests. It would be too exhausting to make pg_notify actually use higher than 2**32 page numbers. Thus, I think test/modules/test_slru is a good place to give high page numbers a good test.
PFA, I've add test for a 64-bit SLRU pages.
By the way, there is another one useful thing we may do here. For now pg_commit_ts functionality is rather strange: if it was enabled, then disabled and then enabled again all the data from before will be discarded. Meanwhile, users expected to have their commit timestamps for all transactions, which were "logged" when this feature was enabled. It's weird.
AFICS, the only reason for this behaviour is becouse of transaction wraparound. It may occur while the feature is disabled end it is safe to simply remove all the data from previous period. If we switch to FullTransactionId in commit_ts we can overcome this limitation. But I'm not sure if it worth to try to fix this in current patchset, since it is already non trivial.
AFICS, the only reason for this behaviour is becouse of transaction wraparound. It may occur while the feature is disabled end it is safe to simply remove all the data from previous period. If we switch to FullTransactionId in commit_ts we can overcome this limitation. But I'm not sure if it worth to try to fix this in current patchset, since it is already non trivial.
--
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Maxim Orlov
Дата:
Aleksander Alekseev,
> 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-bit SLRU pages.
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);
Best regards,
Maxim Orlov.
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Lakhin
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
Андрей, привет!
.pg_stats and range statistics
Tracking statements entry timestamp in pg_stat_statements
Уже закоммичены.
XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Если всё будет и замечаний не возникнет ок, завтра утром закоммичу.
Add semi-join pushdown to postgres_fdw
Переработал патч, сделал обработку условий более аккурантно. Хочу попросить ещё 2 часа на финальное ревью и коммит.
May be BUG. Periodic burst growth of the checkpoint_req counter on replica.
Сегодня вечером планирую доделать и выложить review.
------
Regards,
Alexander Korotkov
Tracking statements entry timestamp in pg_stat_statements
Уже закоммичены.
XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
Если всё будет и замечаний не возникнет ок, завтра утром закоммичу.
Add semi-join pushdown to postgres_fdw
Переработал патч, сделал обработку условий более аккурантно. Хочу попросить ещё 2 часа на финальное ревью и коммит.
May be BUG. Periodic burst growth of the checkpoint_req counter on replica.
Сегодня вечером планирую доделать и выложить review.
------
Regards,
Alexander Korotkov
------
Regards,
Alexander Korotkov
Regards,
Alexander Korotkov
On Mon, Nov 27, 2023 at 9:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
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)
On Tue, 28 Nov 2023 at 14:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
I agree with you that would be good to add a comment instead of XXX and commit.
Pavel
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
On Thu, 30 Nov 2023 at 08:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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 not at 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 at beginning 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 at beginning 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 at beginning 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 not at 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 beginning of 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 at beginning 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 at beginning 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 at beginning 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 beginning of 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 beginning of 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 beginning of 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 beginning of 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 not at 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 beginning of 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 beginning of declaration [-Wold-style-declaration]
regards, tom lane
Agree. The fix is attached.
Regards,
Pavel
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
John Naylor
Дата:
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?
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Pavel Borisov
Дата:
On Mon, 4 Dec 2023 at 10:34, John Naylor <johncnaylorls@gmail.com> wrote:
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?
I think this is complete and could be closed.
Regards,
Pavel
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
John Naylor
Дата:
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 orlovmg@gmail.com
Best regards
发件人: John Naylor <johncnaylorls@gmail.com>
发送时间: 2023年12月4日 18:22
收件人: Pavel Borisov <pashkin.elfe@gmail.com>
抄送: Alexander Korotkov <aekorotkov@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Alexander Lakhin <exclusion@gmail.com>; Maxim Orlov <orlovmg@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; Postgres hackers <pgsql-hackers@lists.postgresql.org>; Heikki Linnakangas <hlinnaka@iki.fi>; Japin Li <japinli@hotmail.com>; Andres Freund <andres@anarazel.de>; Michael Paquier <michael@paquier.xyz>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
发送时间: 2023年12月4日 18:22
收件人: Pavel Borisov <pashkin.elfe@gmail.com>
抄送: Alexander Korotkov <aekorotkov@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Alexander Lakhin <exclusion@gmail.com>; Maxim Orlov <orlovmg@gmail.com>; Aleksander Alekseev <aleksander@timescale.com>; Postgres hackers <pgsql-hackers@lists.postgresql.org>; Heikki Linnakangas <hlinnaka@iki.fi>; Japin Li <japinli@hotmail.com>; Andres Freund <andres@anarazel.de>; Michael Paquier <michael@paquier.xyz>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
主题: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
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.
> 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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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);
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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
Вложения
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Aleksander Alekseev
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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.
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Alexander Korotkov
Дата:
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
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От
Michael Paquier
Дата:
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.