Re: Inadequate executor locking of indexes

Sorry for jumping in late on this.

On 2018/11/24 1:25, Tom Lane wrote:
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> Thinking more about this, the problem I noted previously about two of
> these solutions not working if the index scan node is not physically
> underneath the ModifyTable node actually applies to all three :-(.
> It's a slightly different issue for #2, namely that what we risk is
> first taking AccessShareLock and then upgrading to RowExclusiveLock.
> Since there are places (not many) that take ShareLock on indexes,
> this would pose a deadlock risk.
>
> Now, after more thought, I believe that it's probably impossible
> to have a plan tree in which ExecRelationIsTargetRelation would
> return true at an indexscan node that's not underneath the ModifyTable
> node. What *is* possible is that we have such an indexscan on a
> different RTE for the same table. I constructed this admittedly
> artificial example in the regression database:
>
> # explain with x1 as (select * from tenk1 t1 where unique1 = 42),
> x2 as (update tenk1 t2 set two = 2 where unique2 = 11 returning *)
> select * from x1,x2 where x1.ten = x2.ten;
> QUERY PLAN
> ----------------------------------------------------------------------------------------------
> Nested Loop (cost=16.61..16.66 rows=1 width=488)
> Join Filter: (x1.ten = x2.ten)
> CTE x1
> -> Index Scan using tenk1_unique1 on tenk1 t1 (cost=0.29..8.30 rows=1 width=244)
> Index Cond: (unique1 = 42)
> CTE x2
> -> Update on tenk1 t2 (cost=0.29..8.30 rows=1 width=250)
> -> Index Scan using tenk1_unique2 on tenk1 t2 (cost=0.29..8.30 rows=1 width=250)
> Index Cond: (unique2 = 11)
> -> CTE Scan on x1 (cost=0.00..0.02 rows=1 width=244)
> -> CTE Scan on x2 (cost=0.00..0.02 rows=1 width=244)
> (11 rows)
>
> Because the CTEs will be initialized in order, this presents a case
> where the lock-upgrade hazard exists today: the x1 indexscan will
> open tenk1_unique1 with AccessShareLock and then x2's ModifyTable
> node will upgrade that to RowExclusiveLock. None of the proposed
> fixes improve this.

Provided we want to keep ExecRelationIsTargetRelation going forward, how
about modifying it such that we compare the scan relation's OID with that
of the result relations, not the RT index? Like in the attached.

> I'm beginning to think that postponing target-index locking to
> ExecInitModifyTable was a damfool idea and we should undo it.

+1

Also as already proposed, InitPlan should lock result relation indexes
even for DELETEs.

>> For partitioned
>> table updates, there may be many result relations which can cause
>> ExecRelationIsTargetRelation() to become very slow, in such a case any
>> additional redundant lock would be cheap by comparison.
>
> Yeah, it'd be nice to get rid of the need for that.

As David mentioned elsewhere, can we add the ResultRelInfos to a hash
table if there are at least a certain number of result relations?
3f2393edefa did that for UPDATE tuple routing efficiency.

Thanks,
Amit