BUG #19760: CLUSTER loses a row the transaction inserted and then updated in a rolled-back subtransaction

Hi

Thank you for reporting this.

CLUSTER (and REPACK on master) silently drops a live row when the same
transaction inserted the row and then updated it inside a subtransaction
that was rolled back. No error is raised; the row is gone after COMMIT.

CREATE TABLE t1(c0 int, c1 int);
CREATE INDEX t1_i0 ON t1 (c1);
BEGIN;
INSERT INTO t1 VALUES (1, 1);
SAVEPOINT s1; UPDATE t1 SET c0 = 2; ROLLBACK TO s1;
CLUSTER t1 USING t1_i0;
SELECT count(*) FROM t1; -- 0, expected 1
COMMIT;
SELECT count(*) FROM t1; -- 0, expected 1

While I was reproducing the bug, I noticed that if we SELECT rows before
CLUSTER, the issue won't happen. This suggests that not setting hint bit
is very likely to be related to the issue.

----
BEGIN;
INSERT INTO t1 VALUES (1, 1);
SAVEPOINT s1;
UPDATE t1 SET c0 = 2;
ROLLBACK TO s1;

SELECT count(*) FROM t1; -- SELECT before CLUSTER

CLUSTER VERBOSE t1 USING t1_i0;
SELECT count(*) FROM t1;
---

-- RESULT --
count
-------
1
(1 row)

INFO: repacking "public.t1" using index scan on "t1_i0"
INFO: "public.t1": found 0 removable, 1 nonremovable row versions in 1
pages
DETAIL: 0 dead row versions cannot be removed yet.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
CLUSTER
count
-------
1
(1 row)

COMMIT
count
-------
1
(1 row)

REL_18_STABLE 4d62ef92fda. CLUSTER VERBOSE reports a
negative "dead row versions cannot be removed yet" count (-1 here).

The value is tups_recently_dead in heapam_relation_copy_for_cluster().
tups_recently_dead decreases only when rewrite_heap_dead_tuple()
returnes TRUE in rewriteheap.c. I guess there is something wrong with
visibility check invloving hint bits in rewriteheap.c. I'll look into
this further.

Regards,
Takeshi Ideriha

Hi,

On Fri, 16 Oct 2026 at 08:25, Takeshi Ideriha <iderihatakeshi@gmail.com> wrote:

Hi

Thank you for reporting this.

CLUSTER (and REPACK on master) silently drops a live row when the same
transaction inserted the row and then updated it inside a subtransaction
that was rolled back. No error is raised; the row is gone after COMMIT.

CREATE TABLE t1(c0 int, c1 int);
CREATE INDEX t1_i0 ON t1 (c1);
BEGIN;
INSERT INTO t1 VALUES (1, 1);
SAVEPOINT s1; UPDATE t1 SET c0 = 2; ROLLBACK TO s1;
CLUSTER t1 USING t1_i0;
SELECT count(*) FROM t1; -- 0, expected 1
COMMIT;
SELECT count(*) FROM t1; -- 0, expected 1

While I was reproducing the bug, I noticed that if we SELECT rows before
CLUSTER, the issue won't happen. This suggests that not setting hint bit
is very likely to be related to the issue.

----
BEGIN;
INSERT INTO t1 VALUES (1, 1);
SAVEPOINT s1;
UPDATE t1 SET c0 = 2;
ROLLBACK TO s1;

SELECT count(*) FROM t1; -- SELECT before CLUSTER

CLUSTER VERBOSE t1 USING t1_i0;
SELECT count(*) FROM t1;
---

-- RESULT --
count
-------
1
(1 row)

INFO: repacking "public.t1" using index scan on "t1_i0"
INFO: "public.t1": found 0 removable, 1 nonremovable row versions in 1
pages
DETAIL: 0 dead row versions cannot be removed yet.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
CLUSTER
count
-------
1
(1 row)

COMMIT
count
-------
1
(1 row)

REL_18_STABLE 4d62ef92fda. CLUSTER VERBOSE reports a
negative "dead row versions cannot be removed yet" count (-1 here).

The value is tups_recently_dead in heapam_relation_copy_for_cluster().
tups_recently_dead decreases only when rewrite_heap_dead_tuple()
returnes TRUE in rewriteheap.c. I guess there is something wrong with
visibility check invloving hint bits in rewriteheap.c. I'll look into
this further.

I think you are right. It looks like the missing HEAP_XMAX_INVALID
hint in HeapTupleSatisfiesVacuumHorizon() lets the rewrite defer copying
the live original tuple and then discard it when it encounters the aborted
successor.

Regards,
Ayush

Hi,

Here is a patch. rewrite_heap_dead_tuple() drops the held tuple when its
successor is dead, but that is only right if the successor's inserter
committed. If it aborted, the update never happened, so 0001 keeps the
tuple and adds its logical rewrite mapping.

Setting the hint in HeapTupleSatisfiesVacuum() is not enough. VACUUM
FULL on a catalog can see a transaction that is still open, and this
loses the pg_class row of a committed table:

s1: BEGIN; CREATE TABLE t (a int);
s1: SAVEPOINT sp; ALTER TABLE t ADD COLUMN b int; ROLLBACK TO sp;
s2: VACUUM FULL pg_class;
s1: COMMIT;

0002 is an isolation test for that case, it is optional. But I want it in
tree to prevent
future regression in that area.

Thanks,
Shihao

Hi Shihao, Ayush

On 2026/10/19 12:31, shihao zhong wrote:

Hi,

Here is a patch. rewrite_heap_dead_tuple() drops the held tuple when its
successor is dead, but that is only right if the successor's inserter
committed. If it aborted, the update never happened, so 0001 keeps the
tuple and adds its logical rewrite mapping.

Setting the hint in HeapTupleSatisfiesVacuum() is not enough. VACUUM
FULL on a catalog can see a transaction that is still open, and this
loses the pg_class row of a committed table:

I was actually also writing a code to fix the issue and have just
noticed the patch was already submitted. Attached a patch for reference.
I haven't considered into logical_rewrite_heap_tuple(). I need to check
this... Thank you.

{
+		/*
+		 * If the inserter of this tuple aborted, the update >never happened
+		 * and the earlier tuple is not dead.  It will be >written at the end.
+		 */
+		if (!HeapTupleHeaderXminCommitted(old_tuple->t_data) &&
+			!>TransactionIdDidCommit(HeapTupleHeaderGetXmin(old_tuple->t_data)))
+			return false;

I am thinking like the below since HeapTupleSatisfiesVacuum set the xmin
invalid hint bit for the dead tuple. So I believe we can just rely on
HeapTupleHeaderXminInvalid hint bit.

+               if (HeapTupleHeaderXminInvalid(old_tuple->t_data))
+                       return false;

What do you think above?

0002 is an isolation test for that case, it is optional. But I want it
in tree to prevent
future regression in that area.

+1. I also think this is optional but I believe the issue is easy to
oversight, so adding a new test would be helpful.

Regards,
Takeshi Ideriha

Hi Takeshi,

So I believe we can just rely on HeapTupleHeaderXminInvalid hint bit.

I tried it and it keeps the row in both cases. But it also needs the
logical_rewrite_heap_tuple() call in end_heap_rewrite(). Without it,
logical decoding after VACUUM FULL pg_class fails:

ERROR: could not map filenumber "base/16490/16491" to relation OID

Thanks,
Shihao

Hi Shihao

So I believe we can just rely on HeapTupleHeaderXminInvalid hint bit.

I tried it and it keeps the row in both cases. But it also needs the
logical_rewrite_heap_tuple() call in end_heap_rewrite(). Without it,
logical decoding after VACUUM FULL pg_class fails:

  ERROR:  could not map filenumber "base/16490/16491" to relation OID

Thank you for replying. Glad to know it worked for both two cases.

Regarding logical_rewrite_heap_tuple() part, you are completely right. I
tested by myself yesterday and hit the same error while decoding the
vacuum full scenario. Thank you for pointing out. I was not aware of DDL
case with logical replication.

Regards,
Takeshi Ideriha

On Tue, Oct 20, 2026 at 11:32:44AM +0900, Takeshi Ideriha wrote:

Regarding logical_rewrite_heap_tuple() part, you are completely right. I
tested by myself yesterday and hit the same error while decoding the
vacuum full scenario. Thank you for pointing out. I was not aware of DDL
case with logical replication.

@@ -574,6 +576,14 @@ rewrite_heap_dead_tuple(RewriteState state, HeapTuple old_tuple)

 	if (unresolved != NULL)
 	{
+		/*
+		 * If the inserter of this tuple aborted, the update never happened
+		 * and the earlier tuple is not dead.  It will be written at the end.
+		 */
+		if (!HeapTupleHeaderXminCommitted(old_tuple->t_data) &&
+			!TransactionIdDidCommit(HeapTupleHeaderGetXmin(old_tuple->t_data)))
+			return false;

Having a recheck of the old tuple's state within
rewrite_heap_dead_tuple() feels very incorrect to me, based on the
bunch of comments at the top of rewriteheap.c. It sounds to me that
rewrite_heap_dead_tuple() should be called when we know that the tuple
is dead, no? Isn't heapam_relation_copy_for_cluster() the bit actually
incorrect here, marking the tuple with "isdead" to start with? Your
solution of a recheck to make sure that an old tuple is actually dead
in a code path where an old tuple is assumed to be absolutely and
definitely dead looks like an API contract violation regarding
rewriteheap.c.
--
Michael

On Tue, Oct 20, 2026 at 04:41:49PM +0900, Michael Paquier wrote:

Having a recheck of the old tuple's state within
rewrite_heap_dead_tuple() feels very incorrect to me, based on the
bunch of comments at the top of rewriteheap.c. It sounds to me that
rewrite_heap_dead_tuple() should be called when we know that the tuple
is dead, no? Isn't heapam_relation_copy_for_cluster() the bit actually
incorrect here, marking the tuple with "isdead" to start with? Your
solution of a recheck to make sure that an old tuple is actually dead
in a code path where an old tuple is assumed to be absolutely and
definitely dead looks like an API contract violation regarding
rewriteheap.c.

Looking a bit more at that before the end of the day, I think that the
suggestion of touching HeapTupleSatisfiesVacuumHorizon() and setting
some hint bits would be the correct thing to do. One case that matters
is the "deleting subtransaction must have aborted" case. I am pretty
sure that we should also worry about the case where we check for
TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple)), which
looks reachable, and set an extra hint bit there..
--
Michael

Hi Michael,

Isn't heapam_relation_copy_for_cluster() the bit actually
incorrect here, marking the tuple with "isdead" to start with?

Thanks for looking at this so closely. You are right that the recheck
broke the contract of rewriteheap.c.

Looking a bit more at that before the end of the day, I think that
the suggestion of touching HeapTupleSatisfiesVacuumHorizon() and
setting some hint bits would be the correct thing to do.

Agreed, and thanks for pointing at both places. v2 does just that and
leaves rewriteheap.c alone.

With the hint set, the old version no longer waits in unresolved_tups,
so the logical mapping change from v1 is not needed anymore.

Thanks,
Shihao

On Tue, Oct 20, 2026 at 09:20:18PM -0400, shihao zhong wrote:

Agreed, and thanks for pointing at both places. v2 does just that and
leaves rewriteheap.c alone.

With the hint set, the old version no longer waits in unresolved_tups,
so the logical mapping change from v1 is not needed anymore.

I can see that you are testing both code paths, at least. The
coverage part looks good, but I'd suggest to make the test more
readable at least especially in the s1_alter and s1_plain that include
a bunch of queries. There two are hard to parse.

+                if (!TransactionIdIsInProgress(xmax) &&
+                    !TransactionIdDidCommit(xmax))
+                    SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+                                InvalidTransactionId)

It's also worth noting that these may make the code less efficient,
with more SLRU traffic. What could be the class of worst case
possible? A combination of an in-progress transaction with some
VACUUM activity while the in-progress transaction is still opened?
VACUUM would need to do more work with the patch in place, where I
guess it could be slower if we have a bunch of tuples that need to
have this hint bit set.

Or perhaps this is too narrow to worry about? Anyway, we should be
super careful with such changes. That can bite hard.

If others have any ideas and/or comments, please feel free..

Thanks,
--
Michael

Hi,

On Wed, Oct 21, 2026 at 3:13 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Oct 20, 2026 at 09:20:18PM -0400, shihao zhong wrote:

Agreed, and thanks for pointing at both places. v2 does just that and
leaves rewriteheap.c alone.

With the hint set, the old version no longer waits in unresolved_tups,
so the logical mapping change from v1 is not needed anymore.

I can see that you are testing both code paths, at least. The
coverage part looks good, but I'd suggest to make the test more
readable at least especially in the s1_alter and s1_plain that include
a bunch of queries. There two are hard to parse.

+                if (!TransactionIdIsInProgress(xmax) &&
+                    !TransactionIdDidCommit(xmax))
+                    SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+                                InvalidTransactionId)

It's also worth noting that these may make the code less efficient,
with more SLRU traffic. What could be the class of worst case
possible? A combination of an in-progress transaction with some
VACUUM activity while the in-progress transaction is still opened?
VACUUM would need to do more work with the patch in place, where I
guess it could be slower if we have a bunch of tuples that need to
have this hint bit set.

Or perhaps this is too narrow to worry about? Anyway, we should be
super careful with such changes. That can bite hard.

If others have any ideas and/or comments, please feel free..

I haven't thought of a better idea yet, but I noticed a potential race condition
for MultiXacts in the current fix:

+ if (!(tuple->t_infomask & HEAP_XMAX_INVALID) &&
+ !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
+ !HeapTupleHeaderIsOnlyLocked(tuple))
+ {
+ TransactionId xmax = HeapTupleHeaderGetUpdateXid(tuple);
+
+ if (!TransactionIdIsInProgress(xmax) &&
+ !TransactionIdDidCommit(xmax))
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);

If a tuple's Xmax is a MultiXact and the updating subtransaction is still in
progress when HeapTupleHeaderIsOnlyLocked() is called, it returns false.
However, if that updating subtransaction aborts before the second
TransactionIdIsInProgress() check, the second check returns false and sets
HEAP_XMAX_INVALID on the tuple even though the MultiXact still has active
members, which I think violates the MultiXact hint-bit standard.

This can cause a problem if another transaction locked the tuple (e.g., FOR KEY
SHARE) beforehand. Once HEAP_XMAX_INVALID is set on the tuple, another backend
can lock the tuple FOR UPDATE even though the FOR KEY SHARE lock has not been
released.

I can reproduce this if a transaction updates a tuple, a second session locks
it FOR KEY SHARE, the first transaction updates it again in a subtransaction,
and a concurrent backend runs a SELECT that triggers on-access pruning
(heap_prune_satisfies_vacuum() -> HeapTupleSatisfiesVacuumHorizon()).

See the attached patch (which applies on top of the v2 patches and adds an
injection point) to reproduce this.

Best Regards,
Zhijie Hou

On Wed, Oct 21, 2026 at 06:01:28PM +0800, Zhijie Hou wrote:

+ if (!(tuple->t_infomask & HEAP_XMAX_INVALID) &&
+ !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
+ !HeapTupleHeaderIsOnlyLocked(tuple))
+ {
+ TransactionId xmax = HeapTupleHeaderGetUpdateXid(tuple);
+
+ if (!TransactionIdIsInProgress(xmax) &&
+ !TransactionIdDidCommit(xmax))
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);

If a tuple's Xmax is a MultiXact and the updating subtransaction is still in
progress when HeapTupleHeaderIsOnlyLocked() is called, it returns false.
However, if that updating subtransaction aborts before the second
TransactionIdIsInProgress() check, the second check returns false and sets
HEAP_XMAX_INVALID on the tuple even though the MultiXact still has active
members, which I think violates the MultiXact hint-bit standard.

Perhaps also check for HEAP_XMAX_IS_MULTI to skip the multixacts and
make sure that active members are not lost, then? I am not completely
sure if this would be always right, though, these code paths are the
hard-level definition of tricky.

What I am really wondering is why do we actually need to fix *two*
problems at once? The first SetHintBits() looks like a no-brainer to
me, and links to the second permutation in the test "s1_plain
s1_count" where we do a CLUSTER after an UPDATE rollbacked to a
previous savepoint. I don't think that there is a need for an
isolation test on this one, either, wouldn't a plain SQL sequence be
sufficient?

That's not to mention that the first one can be hit with DDLs that do
some update on catalogs, as well. Say, an ANALYZE with a CLUSTER
(this SQL sequence is a courtesy of Claude, I've just pointed to it
the possibility and the agent has confirmed the sequence):
BEGIN;
-- force the index-scan path
SET LOCAL random_page_cost = 0.0001;
SET LOCAL seq_page_cost = 1000;
CREATE TABLE s (a int);
INSERT INTO s VALUES (1);
ANALYZE s;
SAVEPOINT sp;
INSERT INTO s SELECT g FROM generate_series(2, 1000) g;
-- updates pg_statistic row
ANALYZE s;
ROLLBACK TO sp;
RELEASE sp;
CLUSTER VERBOSE pg_statistic USING pg_statistic_relid_att_inh_index;
COMMIT;
-- one row expected, none on HEAD.
SELECT count(*) FROM pg_statistic WHERE starelid = 's'::regclass;

The VACUUM FULL on pg_class is less exciting, points to the second
SetHintBits(). And as far as I can see, this one is actually the
source of a measurable regression. To prove my point, see the SQL
scripts attached. Use them like that:
1) Run setup.sql, for some relations.
2) Run holders.sql through a bunch of sessions (64 here, could be
more). This is important to stress TransactionIdIsInProgress():
for i in $(seq 64); do
psql -X -f holders.sql > /dev/null 2>&1 &
done
3) In a new session, run either subxact_updates.sql, delete.sql or
insert_only.sql.
3) Run vacuum.sql, measure its runtime.

Then with 500k rows, here are some VACUUM average runtimes in ms from
one of my laptops:
HEAD Shihao's v2
subxact_updates 1408 2123
delete 686 1369
insert_only 689 693

The subxact_updates is an edge case, fine, still it regresses badly.
The DELETE case is a much more common case. It regresses even more
badly, and that does not look acceptable to me. That should be
optimizable, of course, I don't really know yet how to do that. I
know however that regressions like these ones are never good.

So the first step to me would be a "Divide and Conquer" strategy,
where we should look at each problem independently instead of trying
to do too much at once.
--
Michael

Hi

What I am really wondering is why do we actually need to fix *two*
problems at once? The first SetHintBits() looks like a no-brainer to
me, and links to the second permutation in the test "s1_plain
s1_count" where we do a CLUSTER after an UPDATE rollbacked to a
previous savepoint. I don't think that there is a need for an
isolation test on this one, either, wouldn't a plain SQL sequence be
sufficient?

So the first step to me would be a "Divide and Conquer" strategy,
where we should look at each problem independently instead of trying
to do too much at once.

Thank you for sorting the issue out and setting the direction. I was
thinking of leaving first SetHintBits() in
HeapTupleSatisfiesVacuumHorizon() and moving the second SetHintBits() to
heapam_relation_copy_for_cluster() to solve the performance issue.
However, it would still be another API violation, wouldn't it? We also
needs to figure out the multixact race issue. I'm trying to look into
multixact part as much as possible.

Regards,
Takeshi Ideriha

On Thu, Oct 22, 2026 at 11:53:29AM +0900, Takeshi Ideriha wrote:

Thank you for sorting the issue out and setting the direction. I was
thinking of leaving first SetHintBits() in
HeapTupleSatisfiesVacuumHorizon() and moving the second SetHintBits() to
heapam_relation_copy_for_cluster() to solve the performance issue.

Yeah, I don't think that we should begin using SetHintBits() outside
of heapam_visibility.c when it comes down to heap as a table AM.

However, it would still be another API violation, wouldn't it? We also
needs to figure out the multixact race issue. I'm trying to look into
multixact part as much as possible.

What do you think about the first SetHintBits() added for the
subtransaction aborted case? This one seems acceptable here, by
itself..
--
Michael

On 2026/10/22 12:13, Michael Paquier wrote:

Yeah, I don't think that we should begin using SetHintBits() outside> of

heapam_visibility.c when it comes down to heap as a table AM.

Sure, thank you for the reply.

However, it would still be another API violation, wouldn't it? We also
needs to figure out the multixact race issue. I'm trying to look into
multixact part as much as possible.

What do you think about the first SetHintBits() added for the
subtransaction aborted case? This one seems acceptable here, by
itself..

+1 for this. Setting the hint bit for the aborted subtransaction inside
HeapTupleSatisfiesVacuumHorizon is safe and not harmful for the
performance.

Regards,
Takeshi Ideriha

Hi

On 2026/10/22 11:22, Michael Paquier wrote:

On Wed, Oct 21, 2026 at 06:01:28PM +0800, Zhijie Hou wrote:

+ if (!(tuple->t_infomask & HEAP_XMAX_INVALID) &&
+ !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
+ !HeapTupleHeaderIsOnlyLocked(tuple))
+ {
+ TransactionId xmax = HeapTupleHeaderGetUpdateXid(tuple);
+
+ if (!TransactionIdIsInProgress(xmax) &&
+ !TransactionIdDidCommit(xmax))
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);

If a tuple's Xmax is a MultiXact and the updating subtransaction is still in
progress when HeapTupleHeaderIsOnlyLocked() is called, it returns false.
However, if that updating subtransaction aborts before the second
TransactionIdIsInProgress() check, the second check returns false and sets
HEAP_XMAX_INVALID on the tuple even though the MultiXact still has active
members, which I think violates the MultiXact hint-bit standard.

Perhaps also check for HEAP_XMAX_IS_MULTI to skip the multixacts and
make sure that active members are not lost, then? I am not completely
sure if this would be always right, though, these code paths are the
hard-level definition of tricky.

I'm +1 for fixing the subtransaction issue first but let me add a
comment regarding this multixact race issue.

It should be OK not to set the hint bit for a tuple whose xmax is
MultiXact containing a key locker and an aborted updating
subtransaction. This is because rewrite_heap_tuple filters out this
tuple by HeapTupleHeaderIsOnlyLocked and doesn't put it into unresolved
table. I'm not sure if there is another issue we need to consider in
addition to the performance issue.

Regards,
Takeshi Ideriha

Hi,

On Thu, Oct 22, 2026 at 10:22 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Oct 21, 2026 at 06:01:28PM +0800, Zhijie Hou wrote:

+ if (!(tuple->t_infomask & HEAP_XMAX_INVALID) &&
+ !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) &&
+ !HeapTupleHeaderIsOnlyLocked(tuple))
+ {
+ TransactionId xmax = HeapTupleHeaderGetUpdateXid(tuple);
+
+ if (!TransactionIdIsInProgress(xmax) &&
+ !TransactionIdDidCommit(xmax))
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);

If a tuple's Xmax is a MultiXact and the updating subtransaction is still in
progress when HeapTupleHeaderIsOnlyLocked() is called, it returns false.
However, if that updating subtransaction aborts before the second
TransactionIdIsInProgress() check, the second check returns false and sets
HEAP_XMAX_INVALID on the tuple even though the MultiXact still has active
members, which I think violates the MultiXact hint-bit standard.

Perhaps also check for HEAP_XMAX_IS_MULTI to skip the multixacts and
make sure that active members are not lost, then? I am not completely
sure if this would be always right, though, these code paths are the
hard-level definition of tricky.

Right, I think we can skip setting flag for HEAP_XMAX_IS_MULTI tuples,
since they're
handled correctly later in rewrite_heap_tuple() by HeapTupleHeaderIsOnlyLocked()
and won't be added to unresolved_tups. (I see Takeshi San also mentioned that).

What I am really wondering is why do we actually need to fix *two*
problems at once? The first SetHintBits() looks like a no-brainer to
me, and links to the second permutation in the test "s1_plain
s1_count" where we do a CLUSTER after an UPDATE rollbacked to a
previous savepoint.

+1 to fix that part first. That part of the fix (patch) looks clean to me. It
also covers more common cases, which can happen for both user tables and
catalogs, while the other part (the complicated, performance-sensitive one) only
covers catalog cases IIUC, as user table DML holds locks until transaction end,
so concurrent CLUSTER can't touch it in that case.

Then with 500k rows, here are some VACUUM average runtimes in ms from
one of my laptops:
HEAD Shihao's v2
subxact_updates 1408 2123
delete 686 1369
insert_only 689 693

The subxact_updates is an edge case, fine, still it regresses badly.
The DELETE case is a much more common case. It regresses even more
badly, and that does not look acceptable to me. That should be
optimizable, of course, I don't really know yet how to do that. I
know however that regressions like these ones are never good.

One idea to improve is to do these expensive checks only when xmin is not equal
to xmax. If xmin is still in progress, xmax cannot be aborted, so it's safe to
return early in that case. The DELETE test wouldn't regress after this change,
since xmin and xmax differ there. The subxact_update test would still regress,
but that's likely a rare case.

So the first step to me would be a "Divide and Conquer" strategy,
where we should look at each problem independently instead of trying
to do too much at once.

Agreed.

Best Regards,
Zhijie Hou

Hi Michael,

On Thu, Oct 22, 2026 at 11:22:08AM +0900, Michael Paquier wrote:

The DELETE case is a much more common case. It regresses even more
badly, and that does not look acceptable to me. That should be
optimizable, of course, I don't really know yet how to do that.

I ran your scripts as attached upthread, on master 871dee0ccbe with
v2, 500k rows and 64 holders, 12 interleaved rounds per build. The
ratios match yours: VACUUM takes 1.98x HEAD with delete.sql (95% CI
1.96..2.02) and 1.53x with subxact_updates.sql (1.51..1.55), and
insert_only.sql does not move. Method and tables are in the attached
measurements.txt.

The two cases cost for different reasons. In delete.sql xmax is the
inserter's own xid, so the new TransactionIdIsInProgress(xmax) is a
second proc array scan, and pg_stat_slru shows no extra access. It
grows with the number of open transactions: 1.51x with no holder,
1.95x with 16, 2.02x with 128. In subxact_updates.sql xmax is one of
500k subxids, the subxid cache has overflowed, and v2 doubles the
pg_subtrans and clog accesses of each VACUUM (999872 to 1999744 for
pg_subtrans, 499936 to 999872 for the clog).

Zhijie's idea, doing the check only when xmin != xmax, removes the
delete.sql regression: 1.00x (0.99..1.01). In delete.sql xmax and
xmin are the same xid, so the check is skipped there. subxact_updates
stays at 1.53x, as he expected. The condition the other way around
loses the pg_class row again. On its own the idea keeps the
MultiXact race of his repro; with the HEAP_XMAX_IS_MULTI skip you
suggested, the repro passes. Both together are nocfbot-0001.

For subxact_updates, nocfbot-0002 on top of it. In that branch xmin
is in progress, so xmax is the inserter's xid or one of its
subtransactions, and RecordTransactionAbort() writes a subtransaction
abort to the clog before the xids leave the proc array. So
TransactionIdDidAbort(xmax) gives the same answer there as
!TransactionIdIsInProgress() && !TransactionIdDidCommit(), with one
clog lookup and no proc array or pg_subtrans access. With both
diffs, subxact_updates is at 1.01x (1.00..1.03) with 64 holders and
1.00x with 128. What 0002 costs is that clog lookup per tuple:
1.14x with no holder at all (50 ms to 57 ms).

With 0001 alone and with 0001 + 0002, these pass: the v2-0002 spec,
your pg_statistic sequence, VACUUM FULL pg_class with the CREATE
TABLE transaction open, logical decoding across it, Zhijie's
injection point repro, make check, isolation, and recovery 035.
Since 0002 changes when the hint is set, I added five cases where
the pg_class row must survive VACUUM FULL pg_class exactly once
(released, nested and prepared subtransactions). A build that sets
the hint unconditionally passes the original SQL cases and fails
three of the new ones, with two pg_class rows for the table.

The diffs apply on top of v2-0001 and v2-0002. Shihao, if they hold
up, feel free to fold them into a v3.

Regards,
Manu