On Fri, Jan 20, 2023 at 12:31 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Amit Langote <amitlangot...@gmail.com> writes: > > On Fri, Jan 20, 2023 at 4:39 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > >> 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. > > > Interesting. The current implementation relies on > > PlanCacheRelCallback() marking a generic CachedPlan as invalid, so > > perhaps there will have to be some sharing of state between the > > plancache and the executor for this to work? > > Yeah. Thinking a little harder, I think this would have to involve > passing a CachedPlan pointer to the executor, and what the executor > would do after acquiring each lock is to ask the plancache "hey, do > you still think this CachedPlan entry is valid?". In the case where > there's a problem, the AcceptInvalidationMessages call involved in > lock acquisition would lead to a cache inval that clears the validity > flag on the CachedPlan entry, and this would provide an inexpensive > way to check if that happened.
OK, thanks, this is useful. > It might be possible to incorporate this pointer into PlannedStmt > instead of passing it separately. Yeah, that would be less churn. Though, I wonder if you still hold that PlannedStmt should not be scribbled upon outside the planner as you said upthread [1]? > >> * In a successfully built execution state tree, there will simply > >> not be any nodes corresponding to pruned-away, never-locked subplans. > > > I think this is true with the patch as proposed too, but I was still a > > bit worried about what an ExecutorStart_hook may be doing with an > > uninitialized plan tree. Maybe we're mandating that the hook must > > call standard_ExecutorStart() and only work with the finished > > PlanState tree? > > It would certainly be incumbent on any such hook to not touch > not-yet-locked parts of the plan tree. I'm not particularly concerned > about that sort of requirements change, because we'd be breaking APIs > all through this area in any case. OK. Perhaps something that should be documented around ExecutorStart(). > >> * In some cases (views, at least) we need to acquire lock on relations > >> that aren't directly reflected anywhere in the plan tree. So there'd > >> have to be a separate mechanism for getting those locks and rechecking > >> validity afterward. A list of relevant relation OIDs might be enough > >> for that. > > > Hmm, a list of only the OIDs wouldn't preserve the lock mode, > > Good point. I wonder if we could integrate this with the > RTEPermissionInfo data structure? You mean adding a rellockmode field to RTEPermissionInfo? > > Would you like me to hack up a PoC or are you already on that? > > I'm not planning to work on this myself, I was hoping you would. Alright, I'll try to get something out early next week. Thanks for all the pointers. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/922566.1648784745%40sss.pgh.pa.us
