Re: XID formatting and SLRU refactorings - Mailing list pgsql-hackers

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