Date: Wed, 18 Nov 2020 10:43:52 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	James Coleman <jtc331@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: remove spurious CREATE INDEX CONCURRENTLY wait
Message-ID: <20201118014352.GI19692@paquier.xyz>
References: <20201117002341.GA23972@alvherre.pgsql>
 <20201117171453.GA19701@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/8E7gjuj425jZz9t"
Content-Disposition: inline
In-Reply-To: <20201117171453.GA19701@alvherre.pgsql>
Precedence: bulk


--/8E7gjuj425jZz9t
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--/8E7gjuj425jZz9t
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl+0fFgACgkQnvQgOdby
QH3+ig//bQoh3mpMoOIL5dzsuSoE9RBFPRV5PM95y2AsxwvsNclc8G2eYsKulVa2
OHMURfB4f4r1iSI6QPeK2iwzHINImRmwFuAiG+BhfWewjWKI7YZICHWL6qyRreTF
1J2QhuY/HJgH2pyollJ8ihSw4iGCFYnIb/FaolyfOjF+XHh/Jdi3rJMA6XmMYzSJ
IrvD6ohwDLuQC7tB6FWTTC6smgJdZjTOZw+n8Mep9+PeOvxdRTb80LY7htljHwDP
dJ1hpPwTqbF80Jad4yYC+LF/WORToBhD7JeKeMGLmzBzw0RN41rDkHFm0sesf5Oi
EEiBBmOawgkTRkiTnxDDSwax6v5cWJqYEG8NzuBaoEkbPahIwBaZzFxadMEzuqOb
Zlb+D8I6hspu5HqhdGZkVX2/5bcT6gvSLQNJ+atDDG+1vNok+Uvy88ltopfGvBm7
/pr0ODPOFxHxrdjpHxZ2KobEPNBRkRiZ7RSM9U+XE1FWgXKAM0XYLUH6/YoNZ7fA
6eovvq2R4oQWNjhM4Nk3nBG2r1GH0XwoLCb/njsAzzun9O8hyYmpgWH4XM7gTIww
beftLEUYbwa/It9ILCvUYhQbLF6NEGdN7zVFZinHY/F49Kpd6bA4Sp3bOQ3zeUJ/
JOyWUZ8goSYTQTCqXPTUhndrHZ8yEAddK1QLiSFNkNIQpn73GP0=
=MdPs
-----END PGP SIGNATURE-----

--/8E7gjuj425jZz9t--