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.1531674...@sss.pgh.pa.us -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
