Hi! On Wed, Jul 5, 2023 at 4:46 PM Aleksander Alekseev <aleksan...@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 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