Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data - Mailing list pgsql-bugs

Hi,

On 2021-08-15 16:09:37 +0500, Andrey Borodin wrote:
> From 929736512ebf8eb9ac6ddaaf49b9e6148d72cfbb Mon Sep 17 00:00:00 2001
> From: Andrey Borodin <amborodin@acm.org>
> Date: Fri, 30 Jul 2021 14:40:16 +0500
> Subject: [PATCH v12 2/6] PoC fix for race in RelationBuildDesc() and relcache
>  invalidation
> 
> ---
>  src/backend/utils/cache/relcache.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
> index 13d9994af3..7eec7b1f41 100644
> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -997,9 +997,16 @@ equalRSDesc(RowSecurityDesc *rsdesc1, RowSecurityDesc *rsdesc2)
>   *        (suggesting we are trying to access a just-deleted relation).
>   *        Any other error is reported via elog.
>   */
> +typedef struct InProgressRels {
> +    Oid relid;
> +    bool invalidated;
> +} InProgressRels;
> +static InProgressRels inProgress[100];
> +
>  static Relation
>  RelationBuildDesc(Oid targetRelId, bool insertIt)
>  {
> +    int in_progress_offset;
>      Relation    relation;
>      Oid            relid;
>      HeapTuple    pg_class_tuple;
> @@ -1033,6 +1040,14 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
>      }
>  #endif
>  
> +    for (in_progress_offset = 0;
> +         OidIsValid(inProgress[in_progress_offset].relid);
> +         in_progress_offset++)
> +        ;
> +    inProgress[in_progress_offset].relid = targetRelId;
> +retry:
> +    inProgress[in_progress_offset].invalidated = false;
> +
>      /*
>       * find the tuple in pg_class corresponding to the given relation id
>       */
> @@ -1213,6 +1228,12 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
>       */
>      heap_freetuple(pg_class_tuple);
>  
> +    if (inProgress[in_progress_offset].invalidated)
> +        goto retry;                /* TODO free old one */
> +    /* inProgress is in fact the stack, we can safely remove it's top */
> +    inProgress[in_progress_offset].relid = InvalidOid;
> +    Assert(inProgress[in_progress_offset + 1].relid == InvalidOid);
> +
>      /*
>       * Insert newly created relation into relcache hash table, if requested.
>       *
> @@ -2805,6 +2826,13 @@ RelationCacheInvalidateEntry(Oid relationId)
>          relcacheInvalsReceived++;
>          RelationFlushRelation(relation);
>      }
> +    else
> +    {
> +        int i;
> +        for (i = 0; OidIsValid(inProgress[i].relid); i++)
> +            if (inProgress[i].relid == relationId)
> +                inProgress[i].invalidated = true;
> +    }
>  }

Desperately needs comments. Without a commit message and without
comments it's hard to review this without re-reading the entire thread -
which approximately nobody will do.


> From 7e47dae2828d88ddb2161fda0c3b08a158c6cf37 Mon Sep 17 00:00:00 2001
> From: Andrey Borodin <amborodin@acm.org>
> Date: Sat, 7 Aug 2021 20:27:14 +0500
> Subject: [PATCH v12 5/6] PoC fix clear xid
> 
> ---
>  src/backend/access/transam/xact.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
> index 387f80419a..9b19d939eb 100644
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -2500,7 +2500,6 @@ PrepareTransaction(void)
>       * done *after* the prepared transaction has been marked valid, else
>       * someone may think it is unlocked and recyclable.
>       */
> -    ProcArrayClearTransaction(MyProc);
>  
>      /*
>       * In normal commit-processing, this is all non-critical post-transaction
> @@ -2535,6 +2534,8 @@ PrepareTransaction(void)
>      PostPrepare_MultiXact(xid);
>  
>      PostPrepare_Locks(xid);
> +
> +    ProcArrayClearTransaction(MyProc);
>      PostPrepare_PredicateLocks(xid);

The comment above ProcArrayClearTransaction would need to be moved and
updated...

> From 6db9cafd146db1a645bb6885157b0e1f032765e0 Mon Sep 17 00:00:00 2001
> From: Andrey Borodin <amborodin@acm.org>
> Date: Mon, 19 Jul 2021 11:50:02 +0500
> Subject: [PATCH v12 4/6] Fix CREATE INDEX CONCURRENTLY in precence of vxids
>  converted to 2pc
...

> +/*
> + * TwoPhaseGetXidByVXid
> + *        Try to lookup for vxid among prepared xacts
> + */
> +XidListEntry
> +TwoPhaseGetXidByVXid(VirtualTransactionId vxid)
> +{
> +    int                i;
> +    XidListEntry    result;
> +    result.next = NULL;
> +    result.xid = InvalidTransactionId;
> +
> +    LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
> +
> +    for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
> +    {
> +        GlobalTransaction    gxact = TwoPhaseState->prepXacts[i];
> +        PGPROC               *proc = &ProcGlobal->allProcs[gxact->pgprocno];
> +        VirtualTransactionId proc_vxid;
> +        GET_VXID_FROM_PGPROC(proc_vxid, *proc);
> +
> +        if (VirtualTransactionIdEquals(vxid, proc_vxid))
> +        {
> +            if (result.xid != InvalidTransactionId)
> +            {
> +                /* Already has candidate - need to alloc some space */
> +                XidListEntry *copy = palloc(sizeof(XidListEntry));
> +                copy->next = result.next;
> +                copy->xid = result.xid;
> +                result.next = copy;
> +            }
> +            result.xid = gxact->xid;
> +        }
> +    }
> +
> +    LWLockRelease(TwoPhaseStateLock);
> +
> +    return result;
> +}

Dynamic memory allocations while holding a fairly central lock - one
which is now going to be more contended - doesn't seem great.

Is the memory context this is called in guaranteed to be of a proper
duration?  Including being reset in case there's an error at some point
before the memory is freed?


> +/*
> + *        WaitXact
> + *
> + *        Wait for xid completition if have xid. Otherwise try to find xid among
> + *        two-phase procarray entries.
> + */
> +static bool WaitXact(VirtualTransactionId vxid, TransactionId xid, bool wait)
> +{
> +    LockAcquireResult    lar;
> +    LOCKTAG                tag;
> +    XidListEntry        xidlist;
> +    XidListEntry       *xidlist_ptr = NULL; /* pointer to TwoPhaseGetXidByVXid()s pallocs */
> +    bool                result;
> +
> +    if (TransactionIdIsValid(xid))
> +    {
> +        /* We know exact xid - no need to search in 2PC state */
> +        xidlist.xid = xid;
> +        xidlist.next = NULL;
> +    }
> +    else
> +    {
> +        /* You cant have vxid among 2PCs if you have no 2PCs */
> +        if (max_prepared_xacts == 0)
> +            return true;
> +
> +        /*
> +         * We must search for vxids in 2pc state
> +         * XXX: O(N*N) complexity where N is number of prepared xacts
> +         */
> +        xidlist = TwoPhaseGetXidByVXid(vxid);
> +        /* Return if transaction is gone entirely */
> +        if (!TransactionIdIsValid(xidlist.xid))
> +            return true;
> +        xidlist_ptr = xidlist.next;
> +    }

Perhaps I missed this - but won't we constantly enter this path for
non-2pc transactions? E.g.

> @@ -4573,7 +4649,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
>       */
>      proc = BackendIdGetProc(vxid.backendId);
>      if (proc == NULL)
> -        return true;
> +        return WaitXact(vxid, InvalidTransactionId, wait);
>  
>      /*
>       * We must acquire this lock before checking the backendId and lxid
> @@ -4587,9 +4663,12 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
>          || proc->fpLocalTransactionId != vxid.localTransactionId)
>      {
>          LWLockRelease(&proc->fpInfoLock);
> -        return true;
> +        return WaitXact(vxid, InvalidTransactionId, wait);
>      }

It seems like it's going to add a substantial amount of work even when
no 2PC xacts are involved?


> diff --git a/src/include/access/twophase.h b/src/include/access/twophase.h
> index e27e1a8fe8..a5f28d3a80 100644
> --- a/src/include/access/twophase.h
> +++ b/src/include/access/twophase.h
> @@ -25,6 +25,17 @@
>   */
>  typedef struct GlobalTransactionData *GlobalTransaction;
>  
> +/* 
> + * XidListEntry is expected to be used as list very rarely. Under normal
> + * circumstances TwoPhaseGetXidByVXid() returns only one xid.
> + * But under certain conditions can return many xids or nothing.
> + */
> +typedef struct XidListEntry
> +{
> +    TransactionId xid;
> +    struct XidListEntry* next;
> +} XidListEntry;

I don't think we should invent additional ad-hoc list types.

Greetings,

Andres Freund