Re: Inadequate executor locking of indexes

On Thu, 4 Apr 2019 at 15:35, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> BTW, get_actual_variable_range is static to selfuncs.c and other public
> functions that are entry point to get_actual_variable_range's
> functionality appear to require having built a RelOptInfo first, which
> means (even for third party code) having gone through get_relation_info
> and opened the indexes.  That may be too much wishful thinking though.

I think we should do this. We have the CheckRelationLockedByMe Asserts
to alert us if this ever gets broken. I think even if something added
a get_relation_info_hook to alter the indexes somehow, say to remove
one then it should still be okay as get_actual_variable_range() only
looks at index that are in that list and the other two functions are
dealing with Paths, which couldn't exist for an index that was removed
from RelOptInfo->indexlist.

> >> 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.
>
> Yeah, maybe it'd be a good idea, if only for consistency, to fetch the
> rellockmode of the resultRelation RTE and use it for index_open.

I've changed infer_arbiter_indexes() too, but decided to use
rellockmode rather than NoLock since we're not using
RelOptInfo->indexlist.  If anything uses get_relation_info_hook to
remove indexes and then closes removed ones releasing the lock, then
we could end up with problems here.

v2 attached.

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