On Fri, 20 Jan 2023 at 08:39, Tom Lane <t...@sss.pgh.pa.us> wrote: > I spent some time re-reading this whole thread, and the more I read > the less happy I got. We are adding a lot of complexity and introducing > coding hazards that will surely bite somebody someday. And after awhile > I had what felt like an epiphany: the whole problem arises because the > system is wrongly factored. We should get rid of AcquireExecutorLocks > altogether, allowing the plancache to hand back a generic plan that > it's not certain of the validity of, and instead integrate the > responsibility for acquiring locks into executor startup. It'd have > to be optional there, since we don't need new locks in the case of > executing a just-planned plan; but we can easily add another eflags > bit (EXEC_FLAG_GET_LOCKS or so). Then there has to be a convention > whereby the ExecInitNode traversal can return an indicator that > "we failed because the plan is stale, please make a new plan".
I also reread the entire thread up to this point yesterday. I've also been thinking about this recently as Amit has mentioned it to me a few times over the past few months. With the caveat of not yet having looked at the latest patch, my thoughts are that having the executor startup responsible for taking locks is a bad idea and I don't think we should go down this path. My reasons are: 1. No ability to control the order that the locks are obtained. The order in which the locks are taken will be at the mercy of the plan the planner chooses. 2. It introduces lots of complexity regarding how to cleanly clean up after a failed executor startup which is likely to make exec startup slower and the code more complex 3. It puts us even further down the path of actually needing an executor startup phase. For #1, the locks taken for SELECT queries are less likely to conflict with other locks obtained by PostgreSQL, but at least at the moment if someone is getting deadlocks with a DDL type operation, they can change their query or DDL script so that locks are taken in the same order. If we allowed executor startup to do this then if someone comes complaining that PG18 deadlocks when PG17 didn't we'd just have to tell them to live with it. There's a comment at the bottom of find_inheritance_children_extended() just above the qsort() which explains about the deadlocking issue. I don't have much extra to say about #2. As mentioned, I've not looked at the patch. On paper, it sounds possible, but it also sounds bug-prone and ugly. For #3, I've been thinking about what improvements we can do to make the executor more efficient. In [1], Andres talks about some very interesting things. In particular, in his email items 3) and 5) are relevant here. If we did move lots of executor startup code into the planner, I think it would be possible to one day get rid of executor startup and have the plan record how much memory is needed for the non-readonly part of the executor state and tag each plan node with the offset in bytes they should use for their portion of the executor working state. This would be a single memory allocation for the entire plan. The exact details are not important here, but I feel like if we load up executor startup with more responsibilities, it'll just make doing something like this harder. The init run-time pruning code that I worked on likely already has done that, but I don't think it's closed the door on it as it might just mean allocating more executor state memory than we need to. Providing the plan node records the offset into that memory, I think it could be made to work, just with the inefficiency of having a (possibly) large unused hole in that state memory. As far as I understand it, your objection to the original proposal is just on the grounds of concerns about introducing hazards that could turn into bugs. I think we could come up with some way to make the prior method of doing pruning before executor startup work. I think what Amit had before your objection was starting to turn into something workable and we should switch back to working on that. David [1] https://www.postgresql.org/message-id/20180525033538.6ypfwcqcxce6zkjj%40alap3.anarazel.de
