Subject: Re: Add 64-bit XIDs into PostgreSQL 15

From: Pavel Borisov <[email-masked]>
Date: Wed, 2 Feb 2022 19:10:23 +0400

Hi, Andres!

I've revised the README a little bit to address your corrections and
questions. Thanks for this very much!
A patchset with changed README is attached as v8 here (the code is
unchanged and identical to v7).

> > +The downside of this is that we can not use tuple's XMIN and XMAX right
> away.
> > +We often need to re-read t_xmin and t_xmax - which could actually be
> pointers
> > +into a page in shared buffers and therefore they could be updated by
> any other
> > +backend.
>
> Ugh, that's not great.
>
Agree. This part is one of the candidates for revision as per proposals
above [1] i.e :
"2A. Probably refactor it to store precalculated XMIN/XMAX in memory
tuple representation instead of t_xid_base/t_multi_base".

We are working on this change.

> What happens if the first access happens on a replica?
>
> What is the approach for dealing with multixact files? They have xids
> embedded?  And currently the SLRUs will break if you just let the offsets
> SLRU
> grow without bounds.
>
> Wait. So you just modify the page without WAL logging or marking it dirty
> on a
> standby? I fail to see how that can be correct.
>
> Imagine the cluster is promoted, the page is dirtied, and we write it
> out. You'll have written out a completely changed page, without any WAL
> logging. There's plenty other scenarios.
>
In this part, I suppose you've found a definite bug. Thanks! There are a
couple
of ways how it could be fixed:

1. If we enforce checkpoint at replica promotion then we force full-page
writes after each page modification afterward.

2. Maybe it's worth using BufferDesc bit to mark the page as converted to
64xid but not yet written to disk? For example, one of four bits from
BUF_USAGECOUNT.
BM_MAX_USAGE_COUNT  = 5 so it will be enough 3 bits to store it. This will
change in-memory page representation but will not need WAL-logging which is
impossible on a replica.

What do you think about it?

[1]
https://www.postgresql.org/message-id/CALT9ZEHy9yFQEwptCUznPLciqM9ZSs91yTnNSSiG22m%3DBgCpNA%40mail.gmail.com

---

From: Aleksander Alekseev <[email-masked]>
Date: Wed, 2 Mar 2022 16:25:33 +0300

Hi hackers,

> In this part, I suppose you've found a definite bug. Thanks! There are a couple
> 1. If we enforce checkpoint at replica promotion then we force full-page writes after each page modification afterward.
>
> 2. Maybe it's worth using BufferDesc bit to mark the page as converted to 64xid but not yet written to disk? For example, one of four bits from BUF_USAGECOUNT.
> BM_MAX_USAGE_COUNT  = 5 so it will be enough 3 bits to store it. This will change in-memory page representation but will not need WAL-logging which is impossible on a replica.
>

I'm having difficulties merging and/or testing
v8-0002-Add-64bit-xid.patch since I'm not 100% sure which commit this
patch was targeting. Could you please submit a rebased patch and/or
share your development branch on GitHub?

I agree with Bruce it would be great to deliver this in PG15. Please
let me know if you believe it's unrealistic for any reason so I will
focus on testing and reviewing other patches.

For now, I'm changing the status of the patch to "Waiting on Author".

-- 
Best regards,
Aleksander Alekseev

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 2 Mar 2022 18:43:11 +0400

Hi hackers!

Hi! Here is the rebased version.
I'd like to add a description of what was done in v9:
- The patch is rebased on current master branch
- In-memory tuple storage format was refactored as promised to have
pre-calculated 64bit xmin and xmax, not just copies of pd_xid_base and
pd_multi_base.
- Fixed bug reported by Andres Freund, with lazy conversion of pages
upgraded from 32 to 64 xid when first tuple read (and therefore lazy
conversion) is done in read-only state (read-only xact or on replica). In
this case now in memory buffer descriptor will be marked
with REGBUF_CONVERTED flag. When cluster comes to read-write state this
will lead to emitting full page write xlog instruction for this page.

Relevant changes in README are also done.

We'd very much appreciate enthusiasm to have 64 bit xid's in PG15 and any
effort to review and test this feature.

Alexander, thanks for your attention to the patchset. Your questions and
review are very much welcome!
The participation of other hackers is highly appreciated as always!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 2 Mar 2022 10:35:23 -0600

> Hi! Here is the rebased version.

The patch doesn't apply - I suppose the patch is relative a forked postgres
which already has other patches.

http://cfbot.cputube.org/pavel-borisov.html

Note also that I mentioned an issue with pg_upgrade.  Handling that that well
is probably the most important part of the patch.

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Wed, 2 Mar 2022 13:22:34 -0800

Hi,

On 2022-03-02 16:25:33 +0300, Aleksander Alekseev wrote:
> I agree with Bruce it would be great to deliver this in PG15.

> Please let me know if you believe it's unrealistic for any reason so I will

I don't see 15 as a realistic target for this patch. There's huge amounts of
work left, it has gotten very little review.

I encourage trying to break down the patch into smaller incrementally useful
pieces. E.g. making all the SLRUs 64bit would be a substantial and
independently committable piece.

Greetings,

Andres Freund

---

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 3 Mar 2022 14:07:25 +0300

Hi hackers,

> The patch doesn't apply - I suppose the patch is relative a forked postgres

No, the authors just used a little outdated `master` branch. I
successfully applied it against 31d8d474 and then rebased to the
latest master (62ce0c75). The new version is attached.

Not 100% sure if my rebase is correct since I didn't invest too much
time into reviewing the code. But at least it passes `make
installcheck` locally. Let's see what cfbot will tell us.

> independently committable piece.

Completely agree. And the changes like:

+#if 0 /* XXX remove unit tests */

... suggest that the patch is pretty raw in its current state.

Pavel, Maxim, don't you mind me splitting the patchset, or would you
like to do it yourself and/or maybe include more changes? I don't know
how actively you are working on this.

-- 
Best regards,
Aleksander Alekseev