Re: REINDEX INDEX results in a crash for an index of pg_class since 9.6

On 2019-04-30 11:51:10 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2019-04-30 00:50:20 -0400, Tom Lane wrote:
> > I suspect the problem isn't REINDEX INDEX in general, it's REINDEX INDEX
> > over catalog tables modified during reindex.
>
> So far, every one of the failures in the buildfarm looks like the REINDEX
> is deciding that it needs to wait for some other transaction, eg here
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2019-04-30%2014%3A43%3A11
>
> the relevant bit of postmaster log is
>
> 2019-04-30 14:44:13.478 UTC [16135:450] pg_regress/create_index LOG: statement: REINDEX TABLE pg_class;
> 2019-04-30 14:44:14.478 UTC [16137:430] pg_regress/create_view LOG: process 16137 detected deadlock while waiting for AccessShareLock on relation 2662 of database 16384 after 1000.148 ms
> 2019-04-30 14:44:14.478 UTC [16137:431] pg_regress/create_view DETAIL: Process holding the lock: 16135. Wait queue: .
> 2019-04-30 14:44:14.478 UTC [16137:432] pg_regress/create_view STATEMENT: DROP SCHEMA temp_view_test CASCADE;
> 2019-04-30 14:44:14.478 UTC [16137:433] pg_regress/create_view ERROR: deadlock detected
> 2019-04-30 14:44:14.478 UTC [16137:434] pg_regress/create_view DETAIL: Process 16137 waits for AccessShareLock on relation 2662 of database 16384; blocked by process 16135.
> Process 16135 waits for ShareLock on transaction 2875; blocked by process 16137.
> Process 16137: DROP SCHEMA temp_view_test CASCADE;
> Process 16135: REINDEX TABLE pg_class;
> 2019-04-30 14:44:14.478 UTC [16137:435] pg_regress/create_view HINT: See server log for query details.
> 2019-04-30 14:44:14.478 UTC [16137:436] pg_regress/create_view STATEMENT: DROP SCHEMA temp_view_test CASCADE;
>
> I haven't been able to reproduce this locally yet, but my guess is that
> the REINDEX wants to update some row that was already updated by the
> concurrent transaction, so it has to wait to see if the latter commits
> or not. And, of course, waiting while holding AccessExclusiveLock on
> any index of pg_class is a Bad Idea (TM). But I can't quite see why
> we'd be doing something like that during the reindex ...

I've reproduced something similar locally by running "REINDEX INDEX
pg_class_oid_index;" via pgbench. Fails over pretty much immediately.

It's the lock-upgrade problem I theorized about
upthread. ReindexIndex(), via RangeVarCallbackForReindexIndex(), takes a
ShareLock on pg_class, and then goes on to upgrade to RowExclusiveLock
in RelationSetNewRelfilenode(). But at that time another session
obviously can already have the ShareLock and would also want to upgrade.

The same problem exists with reindexing indexes on pg_index.

ReindexTable is also affected. It locks the table with ShareLock, but
then subsidiary routines upgrade to RowExclusiveLock. The way to fix it
would be a bit different than for ReindexIndex(), as the locking happens
via RangeVarGetRelidExtended() directly, rather than in the callback.

There's a somewhat related issue in the new REINDEX CONCURRENTLY. See
https://www.postgresql.org/message-id/20190430151735.wi52sxjvxsjvaxxt%40alap3.anarazel.de

Attached is a *hacky* prototype patch that fixes the issues for me. This
is *not* meant as an actual fix, just a demonstration.

Turns out it's not even sufficient to take a ShareRowExclusive for
pg_class. That prevents issues of concurrent REINDEX INDEX
pg_class_oid_index blowing up, but if one runs REINDEX INDEX
pg_class_oid_index; and REINDEX TABLE pg_class; (or just the latter)
concurrently it still blows up, albeit taking longer to do so.

The problem is that other codepaths expect to be able to hold an
AccessShareLock on pg_class, and multiple pg_class indexes
(e.g. catcache initialization which is easy to hit with -C, [1]). If we
were to want this concurrency safe, I think it requires an AEL on at
least pg_class for reindex (I think ShareRowExclusiveLock might suffice
for pg_index).

I'm not sure it's worth fixing this. It's crummy and somewhat fragile
that we'd have we'd have special locking rules for catalog tables. OTOH,
it really also sucks that a lone REINDEX TABLE pg_class; can deadlock
with another session doing nothing more than establishing a connection.

I guess it's not that common, and can be fixed by users by doing an
explicit BEGIN;LOCK pg_class;REINDEX TABLE pg_class;COMMIT;, but that's
not something anybody will know to do.

Pragmatically I don't think there's a meaningful difference between
holding a ShareLock on pg_class + AEL on one or more indexes, to holding
an AEL on pg_class. Just about every pg_class access is through an
index.

Greetings,

Andres Freund

[1]
#6 0x0000561dac7f9a36 in WaitOnLock (locallock=0x561dae101878, owner=0x561dae112ee8) at /home/andres/src/postgresql/src/backend/storage/lmgr/lock.c:1768
#7 0x0000561dac7f869e in LockAcquireExtended (locktag=0x7ffd7a128650, lockmode=1, sessionLock=false, dontWait=false, reportMemoryError=true,
locallockp=0x7ffd7a128648) at /home/andres/src/postgresql/src/backend/storage/lmgr/lock.c:1050
#8 0x0000561dac7f5c15 in LockRelationOid (relid=2662, lockmode=1) at /home/andres/src/postgresql/src/backend/storage/lmgr/lmgr.c:116
#9 0x0000561dac3a3aa2 in relation_open (relationId=2662, lockmode=1) at /home/andres/src/postgresql/src/backend/access/common/relation.c:56
#10 0x0000561dac422560 in index_open (relationId=2662, lockmode=1) at /home/andres/src/postgresql/src/backend/access/index/indexam.c:156
#11 0x0000561dac421bbe in systable_beginscan (heapRelation=0x561dae14af80, indexId=2662, indexOK=true, snapshot=0x561dacd26f80 <CatalogSnapshotData>,
nkeys=1, key=0x7ffd7a128760) at /home/andres/src/postgresql/src/backend/access/index/genam.c:364
#12 0x0000561dac982362 in ScanPgRelation (targetRelId=2663, indexOK=true, force_non_historic=false)
at /home/andres/src/postgresql/src/backend/utils/cache/relcache.c:360
#13 0x0000561dac983b18 in RelationBuildDesc (targetRelId=2663, insertIt=true) at /home/andres/src/postgresql/src/backend/utils/cache/relcache.c:1058
#14 0x0000561dac985d24 in RelationIdGetRelation (relationId=2663) at /home/andres/src/postgresql/src/backend/utils/cache/relcache.c:2037
#15 0x0000561dac3a3aac in relation_open (relationId=2663, lockmode=1) at /home/andres/src/postgresql/src/backend/access/common/relation.c:59
#16 0x0000561dac422560 in index_open (relationId=2663, lockmode=1) at /home/andres/src/postgresql/src/backend/access/index/indexam.c:156
#17 0x0000561dac976116 in InitCatCachePhase2 (cache=0x561dae13e400, touch_index=true) at /home/andres/src/postgresql/src/backend/utils/cache/catcache.c:1050
--Type <RET> for more, q to quit, c to continue without paging--
#18 0x0000561dac990134 in InitCatalogCachePhase2 () at /home/andres/src/postgresql/src/backend/utils/cache/syscache.c:1078
#19 0x0000561dac988955 in RelationCacheInitializePhase3 () at /home/andres/src/postgresql/src/backend/utils/cache/relcache.c:3960
#20 0x0000561dac9acdac in InitPostgres (in_dbname=0x561dae111320 "postgres", dboid=0, username=0x561dae0dbaf8 "andres", useroid=0, out_dbname=0x0,
override_allow_connections=false) at /home/andres/src/postgresql/src/backend/utils/init/postinit.c:1034