Hi Alexander,
> -#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".
I kept the inline function, as we agreed above.
Typo fixed.
> +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.
Added. I noticed a off-by-one error in the code snippet proposed
above, so my code differs a bit.
> + 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.
Fixed.
> 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.
Fixed. I choose not to change any numbers in the test in order to
check any corner cases, etc. The code patches for long_segment_names =
true and long_segment_names = false are almost the same thus it will
not improve code coverage. Using the current numbers will allow to
easily switch back to long_segment_names = false in the test if
necessary.
PFA the corrected patchset v59.
--
Best regards,
Aleksander Alekseev
v59-0003-Make-use-FullTransactionId-in-2PC-filenames.patch
Description: Binary data
v59-0002-Use-larger-segment-file-names-for-pg_notify.patch
Description: Binary data
v59-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
