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

>
> +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!

[1]
https://www.postgresql.org/message-id/flat/CALT9ZEG1Oo9W_bME5yhsE96AYz19VOnEwHxFUNCosBJHmc0bhw%40mail.gmail.com#86813d80ca9827d36524a9a2adc77c4c

--
Best regards,
Maxim Orlov.