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

On Mon, 6 Nov 2023 at 16:07, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> 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.
>
Hi! Great news!

> BTW, there is a typo in a word "exceeed".
>
Fixed.

>
> +static int inline
> +SlruFileName(SlruCtl ctl, char *path, int64 segno)
> +{
> ...
> +}
>
> I think it worth adding asserts here to verify there is no overflow making
> us mapping different segments into the same files.
>
Agree, assertion added.

> + 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.
>
PFA, I've add test for a 64-bit SLRU pages.

By the way, there is another one useful thing we may do here. For now
pg_commit_ts functionality is rather strange: if it was enabled, then
disabled and then enabled again all the data from before will be
discarded. Meanwhile, users expected to have their commit timestamps for
all transactions, which were "logged" when this feature was enabled. It's
weird.

AFICS, the only reason for this behaviour is becouse of transaction
wraparound. It may occur while the feature is disabled end it is safe to
simply remove all the data from previous period. If we switch to
FullTransactionId in commit_ts we can overcome this limitation. But I'm
not sure if it worth to try to fix this in current patchset, since it is
already non trivial.

--
Best regards,
Maxim Orlov.