Re: REINDEX CONCURRENTLY unexpectedly fails

--oyqLL/JqMvClXZi1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 07, 2020 at 05:33:23PM +0200, Heikki Linnakangas wrote:
> The comment says "this is used as a sanity check". "Sanity check" implies
> that it should never happen, but it is perfectly normal for it to return
> false.

Fixed, thanks.

> The caller in DefineIndex() calls RelationSupportsConcurrentIndexing() on=
ly
> after choosing the lock mode. That's fine for temporary tables, but if
> wouldn't work right if RelationSupportsConcurrentIndexing() started to
> return false for some other tables. Maybe it would be clearer to just che=
ck
> "relpersistence =3D=3D RELPERSISTENCE_TEMP" directly in the callers, and =
not
> have the RelationSupportsConcurrentIndexing() function.

The routine has the advantage of avoiding extra duplication of
comments to justify the choice of enforcing the non-concurrent path as
mentioned upthread.  So I'd rather keep it.

> The new text in drop_index.sgml talks about index creation, copy-pasted f=
rom
> create_index.sgml.

Thanks.  Fixed.

I have spent a couple of hours poking at this code, and found two
problems:
1) The error reporting for PROGRESS_CREATEIDX_COMMAND would report a
concurrent build, but that's not the case if the work happens for a
temporary table in DefineIndex(), so the call to
RelationSupportsConcurrentIndexing needs to happen before any look at
the concurrent flag is done.  That's easy enough to fix.
2) The handling of the patch within index_drop is too weak.  As
presented, the patch first locks the OID using a RangeVar.  However
for a temporary relation we would first take ShareUpdateExclusiveLock
RemoveRelations() and then upgrade to a AccessExclusiveLock in=20
index_drop().  I think that actually the check in index_drop() is not
necessary, and that instead we had better do three things:
a) In RangeVarCallbackForDropRelation(), if the relation is temporary,
use AccessExclusiveLock all the time, and we know the OID of the
relation here.
b) After locking the OID with the RangeVar, re-check if the relation
is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is.
c) Add an assertion in index_drop() to be sure that this code path is
never invoked concurrently with a temporary relation.

I am lacking of time today, I'll continue tomorrow.
--
Michael

--oyqLL/JqMvClXZi1
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4VkJIACgkQnvQgOdby
QH0HvRAAikI0r2Kx3Kn8qqSiL/kZM5pIBZNuhaQz2swsme+a/bhvWFDrfwhfNzot
pT8qCr+b5SaQ0x7RLk2/4V2CStSWZRpMT7d2XL9+HDKvAJ7XoZfJsMoilGTwfNSQ
nDsv92xsEy+vCEHHS0M+icCRTZBLyfXE4bndm2w6HUspI28HnXWw07mP/MQN8/PB
9yRbvWXrwLnThF67a8x0+Rc7S8NKIqrQvvJor4NkULrTy+Ffa7cVULSQtGfqcfHg
BszndD7SW1xFF8O7UJZGP5qjTnc9kEuSCU/n1mZANnng9Pw62J02RfRpus9j7+i2
D0YnCoygkFpdlzXoju+MF3slOLRyHB80WLIUGC+CzCAvan96R50l4+EWpCkoSCWL
Se4lG2fmacDurI/Nv95RiZJB+T/tibIwO41uKyeacOy35p6q9BnH47vVwFtWy0xO
ymbRZgdMu4Ge/XXFc7EK6TOISQAGy4VrTBN4NbYb/mSySuXKkTFVbKOyaIqCsMXJ
oYAn1IFo2624J/2DEr5oCYZ6/6vJdwG0Yjrco0MsmghlpWh48Z3uNg46BALodtz0
iTNLZSTPJnWF+llP1WyU8YcfhkibxnP+AsNhvDos5RppJWhE2x6o6eTnH6zt58fV
9QPJiCN7GVugmL2tVxSwGu17/dXjg1ILaYaTmAfYS6DA5RKmDLE=
=Iuj2
-----END PGP SIGNATURE-----

--oyqLL/JqMvClXZi1--