Re: Inadequate executor locking of indexes

David Rowley <david.rowley@2ndquadrant.com> writes:
> On Thu, 14 Mar 2019 at 21:52, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> If the correct lock is taken in both cases by the current code, then maybe
>> there's no need to change anything?

> I'm aiming to fix the problem described by Tom when he started this
> thread. It's pretty easy to get an assert failure in master.

Yeah, removing the Assert failure is a minimum requirement.  Whether
we need to do more than that can be debated.

> I think this borderline impossible to fix completely.

After further review I concur with that position.  The cases where
we have lock upgrade hazards are where some subquery uses a table
in a way that requires a stronger lock than is needed for some other
reference that was processed earlier.  It's kind of pointless to
guarantee that we avoid that in the planner or executor if the parser
already hit the problem; and it seems darn near impossible, and
certainly impractical, to avoid the problem while parsing.

(Actually, even if we fixed the parser, I'm not sure we'd be out of
the woods.  Consider a case where a subquery requires AccessShareLock
on some table, but then when we come to expand inheritance in the
planner, we discover that that same table is a child or partition of
some target table, so now we need a stronger lock on it.)

I think therefore that we should forget about the idea of avoiding
lock upgrade hazards, at least for situations where the hazard is
caused by conflicting table references that are all written by the
user.  That's not common, and since none of these lock types are
exclusive, the odds of an actual deadlock are low anyway.

> 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.

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.

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.

            regards, tom lane