pgsql: Make queries' locking of indexes more consistent. - Mailing list pgsql-committers

From Tom Lane
Subject pgsql: Make queries' locking of indexes more consistent.
Date
Msg-id E1hC7nU-00005g-F7@gemulon.postgresql.org
Whole thread Raw
List pgsql-committers
Make queries' locking of indexes more consistent.

The assertions added by commit b04aeb0a0 exposed that there are some
code paths wherein the executor will try to open an index without
holding any lock on it.  We do have some lock on the index's table,
so it seems likely that there's no fatal problem with this (for
instance, the index couldn't get dropped from under us).  Still,
it's bad practice and we should fix it.

To do so, remove the optimizations in ExecInitIndexScan and friends
that tried to avoid taking a lock on an index belonging to a target
relation, and just take the lock always.  In non-bug cases, this
will result in no additional shared-memory access, since we'll find
in the local lock table that we already have a lock of the desired
type; hence, no significant performance degradation should occur.

Also, adjust the planner and executor so that the type of lock taken
on an index is always identical to the type of lock taken for its table,
by relying on the recently added RangeTblEntry.rellockmode field.
This avoids some corner cases where that might not have been true
before (possibly resulting in extra locking overhead), and prevents
future maintenance issues from having multiple bits of logic that
all needed to be in sync.  In addition, this change removes all core
calls to ExecRelationIsTargetRelation, which avoids a possible O(N^2)
startup penalty for queries with large numbers of target relations.
(We'd probably remove that function altogether, were it not that we
advertise it as something that FDWs might want to use.)

Also adjust some places in selfuncs.c to not take any lock on indexes
they are transiently opening, since we can assume that plancat.c
did that already.

In passing, change gin_clean_pending_list() to take RowExclusiveLock
not AccessShareLock on its target index.  Although it's not clear that
that's actually a bug, it seemed very strange for a function that's
explicitly going to modify the index to use only AccessShareLock.

David Rowley, reviewed by Julien Rouhaud and Amit Langote,
a bit of further tweaking by me

Discussion: https://postgr.es/m/19465.1541636036@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/9c703c169a872d144f2f79d2fb211c82587adfa7

Modified Files
--------------
src/backend/access/gin/ginfast.c           |  4 ++--
src/backend/executor/execUtils.c           |  4 ++++
src/backend/executor/nodeBitmapIndexscan.c | 15 ++++-----------
src/backend/executor/nodeIndexonlyscan.c   | 15 ++++-----------
src/backend/executor/nodeIndexscan.c       | 15 ++++-----------
src/backend/optimizer/plan/planner.c       |  1 +
src/backend/optimizer/util/plancat.c       | 25 ++++++++++---------------
src/backend/utils/adt/selfuncs.c           | 19 ++++++++++---------
8 files changed, 39 insertions(+), 59 deletions(-)


pgsql-committers by date:

Previous
From: Robert Haas
Date:
Subject: pgsql: Allow VACUUM to be run with index cleanup disabled.
Next
From: Alvaro Herrera
Date:
Subject: pgsql: pg_restore: Require "-f -" to mean stdout