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

Hi!

On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksander(at)timescale(dot)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 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".

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

+ 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

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