Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com> wrote:

> Hello, Antonin!
>
> Antonin Houska <ah(at)cybertec(dot)at>:
> >
> > Where exactly should HeapTupleSatisfiesDirty() conclude that the tuple is
> > visible? TransactionIdIsCurrentTransactionId() will not do w/o the
> > modifications that you proposed earlier [1] and TransactionIdIsInProgress() is
> > not suitable as I explained in [2].
>
> HeapTupleSatisfiesDirty is designed to respect even not-yet-committed
> transactions and provides additional related information.
>
> else if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmin(tuple)))
> {
> /*
> * Return the speculative token to caller. Caller can worry about
> * xmax, since it requires a conclusively locked row version, and
> * a concurrent update to this tuple is a conflict of its
> * purposes.
> */
> if (HeapTupleHeaderIsSpeculative(tuple))
> {
> snapshot->speculativeToken =
> HeapTupleHeaderGetSpeculativeToken(tuple);
>
> Assert(snapshot->speculativeToken != 0);
> }
>
> snapshot->xmin = HeapTupleHeaderGetRawXmin(tuple);
> /* XXX shouldn't we fall through to look at xmax? */
> return true; /* in insertion by other */
> }
>
> So, it returns true when TransactionIdIsInProgress is true.
> However, that alone is not sufficient to trust the result in the common case.
>
> You may check check_exclusion_or_unique_constraint or
> RelationFindReplTupleByIndex for that pattern:
> if xmin is set in the snapshot (a special hack in SnapshotDirty to
> provide additional information from the check), we wait for the
> ongoing transaction (or one that is actually committed but not yet
> properly reflected in the proc array), and then retry the entire tuple
> search.
>
> So, the race condition you explained in [2] will be resolved by a
> retry, and the changes to TransactionIdIsInProgress described in [1]
> are not necessary.

I insist that this is a misuse of TransactionIdIsInProgress(). When dealing
with logical decoding, only WAL should tell whether particular transaction is
still running. AFAICS this is how reorderbuffer.c works.

A new kind of snapshot seems like (much) cleaner solution at the moment.

> I'll try to make some kind of prototype this weekend + cover race
> condition you mentioned in specs.
> Maybe some corner cases will appear.

No rush. First, the MVCC safety is not likely to be included in v19
[1]. Second, I think it's good to let others propose their ideas before
writing code.

> By the way, there's one more optimization we could apply in both
> MVCC-safe and non-MVCC-safe cases: setting the HEAP_XMIN_COMMITTED /
> HEAP_XMAX_COMMITTED bit in the new table:
> * in the MVCC-safe approach, the transaction is already committed.
> * in the non-MVCC-safe case, it isn’t committed yet - but no one will
> examine that bit before it commits (though this approach does feel
> more fragile).
>
> This could help avoid potential storms of full-page writes caused by
> SetHintBit after the table switch.

Good idea, thanks.

[1] https://www.postgresql.org/message-id/202504040733.ysuy5gad55md%40alvherre.pgsql

--
Antonin Houska
Web: https://www.cybertec-postgresql.com