Re: Inadequate executor locking of indexes

On Wed, 3 Apr 2019 at 06:03, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > On Thu, 14 Mar 2019 at 21:52, Amit Langote
> > Maybe you're right about being able to use rellockmode for indexes,
> > but I assume that we lowered the lock level for indexes for some
> > reason, and this would reverse that.
>
> I kind of think that we *should* use rellockmode for indexes too.
> To the extent that we're doing differently from that now, I think
> it's accidental not intentional. It would perhaps have been difficult
> to clean that up completely before we added rellockmode, but now that
> we've got that we should use it. I feel that adding a second field
> for index lock mode would just be perpetuating some accidental
> inconsistencies.

Okay. That certainly makes the patch a good bit more simple.

> In short, I think we should take the parts of this patch that modify
> the index_open calls, but make them use rte->rellockmode; and forget
> all the other parts.

Done.

> BTW, I'd also suggest expanding the header comment for
> ExecRelationIsTargetRelation to explain that it's no longer
> used in the core code, but we keep it around because FDWs
> might want it.

Also done.

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.

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.

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