Amit Langote <amitlangote09@gmail.com> writes:
> On Fri, Jan 20, 2023 at 4:39 AM Tom Lane <tgl@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.
It might be possible to incorporate this pointer into PlannedStmt
instead of passing it separately.
>> * 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.
>> * 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?
> 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.
regards, tom lane