Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
От | Peter Eisentraut |
---|---|
Тема | Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) |
Дата | |
Msg-id | 131006ee-d309-f77e-7d3e-db8e9d76aeef@enterprisedb.com обсуждение исходный текст |
Ответ на | Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15) (Maxim Orlov <orlovmg@gmail.com>) |
Ответы |
Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)
(Pavel Borisov <pashkin.elfe@gmail.com>)
|
Список | pgsql-hackers |
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.
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Robert HaasДата:
Сообщение: Re: multithreaded zstd backup compression for client and server
Следующее
От: Kenaniah CernyДата:
Сообщение: Re: Proposal: allow database-specific role memberships