Re: Inadequate executor locking of indexes

Hi David,

Thanks for updating the patch.

On 2019/04/04 9:14, David Rowley wrote:
> I also looked over other index_open() calls in the planner and found a
> bunch of places in selfuncs.c that we open an index to grab some
> information then close it again releasing the lock.  At this stage
> get_relation_info() should have already grabbed what it needs and
> stored it into an IndexOptInfo, so we might have no need to access the
> index again. However, if any code was added that happened to assume
> the index was already locked then we'd get the same Assert failure
> that we're fixing here. I've ended up changing these calls so that
> they also use rellockmode, which may make the lock just a trip to the
> local lock table for relations that have rellockmode >
> AccessShareLock.  I also changed the index_close to use NoLock so we
> hold the lock.

Sorry, I didn't understand why it wouldn't be OK to pass NoLock to
index_open, for example, here:

@@ -5191,7 +5191,14 @@ get_actual_variable_range(PlannerInfo *root,
VariableStatData *vardata,
 			 * necessarily on the index.
 			 */
 			heapRel = table_open(rte->relid, NoLock);
-			indexRel = index_open(index->indexoid, AccessShareLock);
+
+			/*
+			 * We use the same lock level as the relation as it may have
+			 * already been locked with that level.  Using the same lock level
+			 * can save a trip to the shared lock manager.
+			 */
+			Assert(rte->rellockmode != NoLock);
+			indexRel = index_open(index->indexoid, rte->rellockmode);

Especially seeing that the table itself is opened without lock.  If there
are any Assert failures, wouldn't that need to be fixed in the upstream
code (such as get_relation_info)?

Also, I noticed that there is infer_arbiter_indexes() too, which opens the
target table's indexes with RowExclusiveLock.  I thought for a second
that's a index-locking site in the planner that you may have missed, but
turns out it might very well be the first time those indexes are locked in
a given insert query's processing, because query_planner doesn't need to
plan access to the result relation, so get_relation_info is not called.

> I scanned around other usages of index_open() and saw that
> gin_clean_pending_list() uses an AccessShareLock. That seems strange
> since it modifies the index.

Yeah, other maintenance tasks modifying an index, such as
brin_summarize_range(), take ShareUpdateExclusiveLock.

Thanks,
Amit