Horton

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

Aleksander Alekseev <aleksander@timescale.com>

View thread View on postgr.es
From: Aleksander Alekseev <aleksander@timescale.com>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Maxim Orlov <orlovmg@gmail.com>, Japin Li <japinli@hotmail.com>, Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>, Pavel Borisov <pashkin.elfe@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Date: 2023-11-07T11:57:12Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Fix more holes with SLRU code in need of int64 for segment numbers

  2. Use more consistently int64 for page numbers in SLRU-related code

  3. Improve comments in slru.{c,h} about segment name format

  4. Reorder actions in ProcArrayApplyRecoveryInfo()

  5. Fix typo in 5a1dfde8334b

  6. Fix warning due non-standard inline declaration in 4ed8f0913bfdb5f355

  7. Add SLRU tests for 64-bit page case

  8. Make use FullTransactionId in 2PC filenames

  9. Use larger segment file names for pg_notify

  10. Index SLRUs by 64-bit integers rather than by 32-bit integers

  11. Keep track of transaction commit timestamps

  12. Handle 5-char filenames in SlruScanDirectory