Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

On Mon, Dec 21, 2020 at 12:24:55PM +0500, Andrey Borodin wrote:
> > 21 дек. 2020 г., в 10:40, Michael Paquier <michael(at)paquier(dot)xyz> написал(а):
> >> In general, I wonder whether WaitForLockersMultiple and GetLockConflicts
> >> need to gain an additional parameter indicating whether to consider
> >> prepared xacts. It's not clear to me that their current behavior is wrong
> >> for all possible uses.
> >
> > WaitForLockers is used only by REINDEX and CREATE/DROP CONCURRENTLY,
> > where it seems to me we need to care about all the cases related to
> > concurrent build, validation and index drop. The other caller of
> > GetLockConflicts() is for conflict resolution in standbys where it is
> > fine to ignore 2PC transactions as these cannot be cancelled.
>
> I don't think that fact that we cannot cancel transaction is sufficient here to ignore prepared transaction. I think there should not exist any prepared transaction that we need to cancel in standby conflict resolution. And if it exists - it's a sign of corruption, we could emit warning or something like that.

Agreed. Based on src/backend/storage/lmgr/README section "Locking during Hot
Standby" and RecoverPreparedTransactions(), prepared-transaction PGPROC
entries get locks at end of recovery, not earlier. The conflict resolution
callers won't see prepared-transaction locks, and they could assert that,
raise an error, or just assume that implicitly.

> But I'm really not an expert here, just a common sense that prepared transaction is just like regular transaction that survives crash. If we wait for any transaction - probably we should wait for prepared too. I'm not insisting on anything though.

In the startup process of a standby, waiting for a primary-side transaction
(regular or prepared) would succeed instantly, rendering the wait meaningless.
I'm not too worried about it; several parts of the system would change if
these standby-side lock invariants ceased to hold.

On Mon, Dec 21, 2020 at 01:19:59PM +0500, Andrey Borodin wrote:
> --- /dev/null
> +++ b/src/test/isolation/expected/prepared-transactions-cic.out
> @@ -0,0 +1,18 @@
> +Parsed test spec with 2 sessions
> +
> +starting permutation: w1 p1 cic2 c1 r2
> +step w1: BEGIN; INSERT INTO cic_test VALUES (1);
> +step p1: PREPARE TRANSACTION 's1';
> +step cic2:
> + CREATE INDEX CONCURRENTLY on cic_test(a);
> +
> +ERROR: canceling statement due to lock timeout

I wondered if a slow server could ever print "<waiting ...>" in here. It
can't; this is fine. pg_isolation_test_session_is_blocked() never reports the
prepared transaction, because that transaction's locks have no pid associated.

> --- a/src/backend/storage/lmgr/lock.c
> +++ b/src/backend/storage/lmgr/lock.c
> @@ -2931,15 +2929,17 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
>
> /*
> * Allocate memory to store results, and fill with InvalidVXID. We only
> - * need enough space for MaxBackends + a terminator, since prepared xacts
> - * don't count. InHotStandby allocate once in TopMemoryContext.
> + * need enough space for MaxBackends + max_prepared_xacts + a
> + * terminator. InHotStandby allocate once in TopMemoryContext.
> */
> if (InHotStandby)
> {
> if (vxids == NULL)
> vxids = (VirtualTransactionId *)
> MemoryContextAlloc(TopMemoryContext,
> - sizeof(VirtualTransactionId) * (MaxBackends + 1));
> + sizeof(VirtualTransactionId) * (MaxBackends
> + + max_prepared_xacts
> + + 1));

PostgreSQL typically puts the operator before the newline. Also, please note
the whitespace error that "git diff --check origin/master" reports.

> }
> else
> vxids = (VirtualTransactionId *)

This is updating only the InHotStandby branch. Both branches need the change.

> @@ -4461,9 +4462,21 @@ bool
> VirtualXactLock(VirtualTransactionId vxid, bool wait)
> {
> LOCKTAG tag;
> - PGPROC *proc;
> + PGPROC *proc = NULL;
>
> - Assert(VirtualTransactionIdIsValid(vxid));
> + Assert(VirtualTransactionIdIsValid(vxid) ||
> + VirtualTransactionIdIsPreparedXact(vxid));
> +
> + if (VirtualTransactionIdIsPreparedXact(vxid))
> + {
> + LockAcquireResult lar;
> + /* If it's prepared xact - just wait for it */
> + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
> + lar = LockAcquire(&tag, ShareLock, false, !wait);
> + if (lar == LOCKACQUIRE_OK)

This should instead test "!= LOCKACQUIRE_NOT_AVAIL", lest
LOCKACQUIRE_ALREADY_HELD happen. (It perhaps can't happen, but skipping the
LockRelease() would be wrong if it did.)

> + LockRelease(&tag, ShareLock, false);
> + return lar != LOCKACQUIRE_NOT_AVAIL;
> + }
>
> SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
>
> diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
> index 1c3e9c1999..cedb9d6d2a 100644
> --- a/src/include/storage/lock.h
> +++ b/src/include/storage/lock.h
> @@ -70,6 +70,8 @@ typedef struct
> #define VirtualTransactionIdIsValid(vxid) \
> (((vxid).backendId != InvalidBackendId) && \
> LocalTransactionIdIsValid((vxid).localTransactionId))
> +#define VirtualTransactionIdIsPreparedXact(vxid) \
> + ((vxid).backendId == InvalidBackendId)

Your patch is introducing VirtualTransactionId values that represent prepared
xacts, and it has VirtualTransactionIdIsValid() return false for those values.
Let's make VirtualTransactionIdIsValid() return true for them, since they're
as meaningful as any other value. The GetLockConflicts() header comment says
"The result array is palloc'd and is terminated with an invalid VXID." Patch
v4 falsifies that comment. The array can contain many of these new "invalid"
VXIDs, and an all-zeroes VXID terminates the array. Rather than change the
comment, let's change VirtualTransactionIdIsValid() to render the comment true
again. Most (perhaps all) VirtualTransactionIdIsValid() callers won't need to
distinguish the prepared-transaction case.

An alternative to redefining VXID this way would be to have some new type,
each instance of which holds either a valid VXID or a valid
prepared-transaction XID. That alternative feels inferior to me, though.
What do you think?

Thanks,
nm