Re: Adding REPACK [concurrently]

Antonin Houska <ah(at)cybertec(dot)at>:

> 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.

Hm... Maybe, but at the same time we already have SnapshotDirty used
in that way and it even deals with the same race....
But I agree - a special kind of snapshot is a more accurate solution.

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

Do you mean some kind of snapshot which only uses
TransactionIdDidCommit/Abort ignoring
TransactionIdIsCurrentTransactionId/TransactionIdIsInProgress?
Actually it behaves like SnapshotBelieveEverythingCommitted in that
particular case, but TransactionIdDidCommit/Abort may be used as some
kind of assert/error source to be sure everything is going as
designed.
And, yes, for the new snapshot we need to have
HeapTupleSatisfiesUpdate to be modified.

Also, to deal with that particular race we may just use
XactLockTableWait(xid, NULL, NULL, XLTW_None) before starting
transaction replay.

> No rush. First, the MVCC safety is not likely to be included in v19 [1].

That worries me - it is not the behaviour someone expects from a
database by default. At least the warning should be much more visible
and obvious.
I think most of user will expect the same guarantees as [CREATE|RE]
INDEX CONCURRENTLY provides.