Re: Add 64-bit XIDs into PostgreSQL 15

Maxim Orlov <orlovmg@gmail.com>

From: Maxim Orlov <orlovmg@gmail.com>

To: Dilip Kumar <dilipbalaut@gmail.com>

Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Aleksander Alekseev <aleksander@timescale.com>, Postgres hackers <pgsql-hackers@lists.postgresql.org>, Justin Pryzby <pryzby@telsasoft.com>, Stephen Frost <sfrost@snowman.net>, Alexander Korotkov <aekorotkov@gmail.com>, Andres Freund <andres@anarazel.de>, Ilya Anfimov <ilan@tzirechnoy.com>

Date: 2022-09-16T08:59:20Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Add SLRU tests for 64-bit page case
- a60b8a58f435 17.0 landed
Make use FullTransactionId in 2PC filenames
- 5a1dfde8334b 17.0 landed
Use larger segment file names for pg_notify
- 2cdf131c46e6 17.0 landed
Index SLRUs by 64-bit integers rather than by 32-bit integers
- 4ed8f0913bfd 17.0 landed

Attachments

v46-0005-Add-initdb-option-to-initialize-cluster-with-non.patch (application/octet-stream) patch v46-0005
v46-0003-Use-64-bit-FullTransactionId-instead-of-Epoch-xi.patch (application/octet-stream) patch v46-0003
v46-0004-Use-64-bit-pages-representation-in-SLRU-callers.patch (application/octet-stream) patch v46-0004
v46-0002-Use-64-bit-format-to-output-XIDs.patch (application/octet-stream) patch v46-0002
v46-0001-Use-64-bit-numbering-of-SLRU-pages.patch (application/octet-stream) patch v46-0001
v46-0006-README.XID64.patch (application/octet-stream) patch v46-0006
v46-0007-Use-64-bit-GUCs.patch (application/octet-stream) patch v46-0007
v46-0008-Use-64-bit-XIDs.patch (application/octet-stream) patch v46-0008

Thank you for your review.

Since we have converted TransactionId to 64-bit, so do we still need
> the concept of FullTransactionId?  I mean it is really confusing to
> have 3 forms of transaction ids.  i.e. Transaction Id,
> FullTransactionId and ShortTransactionId.
>
Yeah, I totally agree with you. Actually, it is better to get rid of them,
if this patch set will be committed.
We've already tried to do some experiments on this issue. But,
unfortunately, this resulted in bloating
the patch set. So, we decided to address this in the future.

1.
>  typedef struct HeapTupleData
>  {
> + TransactionId t_xmin; /* base value for normal transaction ids */
> + TransactionId t_xmax; /* base value for mutlixact */
>
> I think the field name and comments are not in sync, field says xmin
> and xmax whereas the comment says base value for
> transaction id and multi-xact.
>
Fixed.


> 2.
> extern bool heap_page_prepare_for_xid(Relation relation, Buffer buffer,
>   TransactionId xid, bool multi);
>
> I noticed that this function is returning bool but all the callers are
> ignoring the return type.
>
Fixed.


> 3.
> +static int
> +heap_page_try_prepare_for_xid(Relation relation, Buffer buffer, Page page,
> +   TransactionId xid, bool multi, bool is_toast)
> +{
> + TransactionId base;
> + ShortTransactionId min = InvalidTransactionId,
>
> add function header comments.
>
Fixed. Also, I made some refactoring to make this more clear.


> 4.
> Why no handling for multi?  And this function has absolutely no
> comments to understand the reason for this.
>
Actually, this function works for multi transactions as well as for
"regular" transactions.
But in case of "regular" transactions, we have to look through multi
transactions to
see if any update transactions for particular tuple is present or not.
I add comments around here to make it clear.


> 5.
> Why pd_xid_base can not be just RecentXmin?  Please explain in the
> comments above.
>
We're doing this, If I'm not mistaken, to be able to get all the possible
XID's
values, include InvalidTransactionId, FrozenTransactionId and so on. In
other
words, me must be able to get XID's values including special ones.

Here is a rebased version of a patch set. As always, reviews are very
welcome!

-- 
Best regards,
Maxim Orlov.