Re: Inadequate executor locking of indexes

On Thu, 4 Apr 2019 at 15:01, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> 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)?

I put that Assert there to ensure that everything that calls it has
properly set RangeTblEntry's rellockmode. If there's some valid reason
for that to be NoLock then it's the wrong thing to do, but I can't
think of a valid reason.

> 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 skimmed over that and thought that the rellockmode would always be
RowExclusiveLock anyway, so didn't change it. However, it would make
sense to use rellockmode for consistency. We're already looking up the
RangeTblEntry to get the relid, so getting the rellockmode is about
free.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services