Re: remove spurious CREATE INDEX CONCURRENTLY wait

On 2020-Nov-18, Michael Paquier wrote:

> On Tue, Nov 17, 2020 at 02:14:53PM -0300, Alvaro Herrera wrote:
> > diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
> > index f1f4df7d70..4324e32656 100644
> > --- a/src/backend/replication/logical/logical.c
> > +++ b/src/backend/replication/logical/logical.c
> > @@ -181,7 +181,7 @@ StartupDecodingContext(List *output_plugin_options,
> > */
> > if (!IsTransactionOrTransactionBlock())
> > {
> > - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
> > + LWLockAcquire(ProcArrayLock, LW_SHARED);
> > MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING;
> > ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
> > LWLockRelease(ProcArrayLock);
>
> We don't really document that it is safe to update statusFlags while
> holding a shared lock on ProcArrayLock, right? Could this be
> clarified at least in proc.h?

Pushed that part with a comment addition. This stuff is completely
undocumented ...

> > + /* Determine whether we can call set_safe_index_flag */
> > + safe_index = indexInfo->ii_Expressions == NIL &&
> > + indexInfo->ii_Predicate == NIL;
>
> This should tell why we check after expressions and predicates, in
> short giving an explanation about the snapshot issues with build and
> validation when executing those expressions and/or predicates.

Fair. It seems good to add a comment to the new function, and reference
that in each place where we set the flag.

> > + * Set the PROC_IN_SAFE_IC flag in my PGPROC entry.

> Would it be better to document that this function had better be called
> after beginning a transaction? I am wondering if we should not
> enforce some conditions here, say this one or similar:
> Assert(GetTopTransactionIdIfAny() == InvalidTransactionId);

I went with checking MyProc->xid and MyProc->xmin, which is the same in
practice but notionally closer to what we're doing.

> > +static inline void
> > +set_safe_index_flag(void)
>
> This routine name is rather close to index_set_state_flags(), could it
> be possible to finish with a better name?

I went with set_indexsafe_procflags(). Ugly ...