While reviewing Amit's 0001 patch in [1] I noticed Amit pulled out the
code that inits the ResultRelInfos during InitPlan() but didn't update
the comment which says:
/*
* initialize result relation stuff, and open/lock the result rels.
*
* We must do this before initializing the plan tree, else we might try to
* do a lock upgrade if a result rel is also a source rel.
*/
This got me thinking about what Tom pointed out in [2].
Tom wrote:
> It struck me this morning that a whole lot of the complication here is
> solely due to needing to identify the right type of relation lock to take
> during executor startup, and that THAT WORK IS TOTALLY USELESS. In every
> case, we must already hold a suitable lock before we ever get to the
> executor; either one acquired during the parse/plan pipeline, or one
> re-acquired by AcquireExecutorLocks in the case of a cached plan.
> Otherwise it's entirely possible that the plan has been invalidated by
> concurrent DDL --- and it's not the executor's job to detect that and
> re-plan; that *must* have been done upstream.
This seems to mean that the above code comment was never true. How can
we possibly be preventing a lock upgrade hazard if the locks are
already all obtained?
Amit's change to delay the ResultRelInfo creation until the node is
initialized seems to not make the problem worse as the locks are
already taken, but I do wonder if there is something I'm not seeing
here. I do have a case here that does deadlock because of the lock
upgrade, but it feels pretty fabricated.
S1: set plan_cache_mode = 'force_generic_plan';
S1: prepare q1 as select * from t1 cross join t2 cross join t1 t1a for
update of t2,t1a;
S1: execute q1;
-- break after AccessShareLock on t1. (the 1st lock that's obtained)
S2: BEGIN;
S2: LOCK TABLE t1 IN EXCLUSIVE MODE;
-- continue S1 until RowShareLock on t2 (the 2nd lock that's obtained)
S2: LOCK TABLE t2 IN EXCLUSIVE MODE;
-- unattach S1 from debugger.
S1: ERROR: deadlock detected
I think we're also protected to some degree because result relations
come first in the rangetable. This is why I used FOR UPDATE in my
example as I can control the range table index based on where I put
the rel in the query.
Is it safe to remove the 2nd part of the code comment? Or is there
something I'm not seeing here?
[1] https://www.postgresql.org/message-id/a20151ff-9d3b-bec8-8c64-5336676cfda3%40lab.ntt.co.jp
[2] https://www.postgresql.org/message-id/4114.1531674142@sss.pgh.pa.us
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services