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

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