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? > + /* 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. > + * Set the PROC_IN_SAFE_IC flag in my PGPROC entry. > + * > + * When doing concurrent index builds, we can set this flag > + * to tell other processes concurrently running CREATE > + * INDEX CONCURRENTLY to ignore us when > + * doing their waits for concurrent snapshots. On one hand it > + * avoids pointlessly waiting for a process that's not interesting > + * anyway, but more importantly it avoids deadlocks in some cases. > + * > + * This can only be done for indexes that don't execute any expressions. > + * Caller is responsible for only calling this routine when that > + * assumption holds true. > + * > + * (The flag is reset automatically at transaction end, so it must be > + * set for each transaction.) 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); > + */ > +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? -- Michael
signature.asc
Description: PGP signature
