Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. |
Date | |
Msg-id | bade3546-e70e-2bb6-1a8d-50aaab8d23e1@lab.ntt.co.jp Whole thread Raw |
In response to | Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
Responses |
Re: postgres_fdw : altering foreign table not invalidating
prepare statement execution plan.
(Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
|
List | pgsql-hackers |
On 2016/10/06 21:55, Etsuro Fujita wrote: > On 2016/10/06 20:17, Amit Langote wrote: >> On 2016/10/05 20:45, Etsuro Fujita wrote: >>> On 2016/10/05 14:09, Ashutosh Bapat wrote: >>>> IMO, maintaining that extra function and >>>> the risk of bugs because of not keeping those two functions in sync >>>> outweigh the small not-so-frequent gain. >>> The inefficiency wouldn't be negligible in the case where there are large >>> numbers of cached plans. So, I'd like to introduce a helper function that >>> checks the dependency list for the generic plan, to eliminate most of the >>> duplication. > >> I too am inclined to have a PlanCacheForeignCallback(). >> >> Just one minor comment: > > Thanks for the comments! > > I noticed that we were wrong. Your patch was modified so that > dependencies on FDW-related objects would be extracted from a given plan > in create_foreignscan_plan (by Ashutosh) and then in > set_foreignscan_references by me, but that wouldn't work well for INSERT > cases. To fix that, I'd like to propose that we collect the dependencies > from the given rte in add_rte_to_flat_rtable, instead. I see. So, doing it from set_foreignscan_references() fails to capture plan dependencies in case of INSERT because it won't be invoked at all unlike the UPDATE/DELETE case. > Attached is an updated version, in which I removed the > PlanCacheForeignCallback and adjusted regression tests a bit. > >>> If some writable FDW consulted foreign table/server/FDW options in >>> AddForeignUpdateTarget, which adds the extra junk columns for >>> UPDATE/DELETE to the targetList of the given query tree, the rewritten >>> query tree would also need to be invalidated. But I don't think such an >>> FDW really exists because that routine in a practical FDW wouldn't change >>> such columns depending on those options. > > I had second thoughts about that; since the possibility wouldn't be zero, > I added to extract_query_dependencies_walker the same code I added to > add_rte_to_flat_rtable. And here, since AddForeignUpdateTargets() could possibly utilize foreign options which would cause *query tree* dependencies. It's possible that add_rte_to_flat_rtable may not be called before an option change, causing invalidation of any cached objects created based on the changed options. So, must record dependencies from extract_query_dependencies as well. > After all, the updated patch is much like your version, but I think your > patch, which modified extract_query_dependencies_walker only, is not > enough because a generic plan can have more dependencies than its query > tree (eg, consider inheritance). Agreed. I didn't know at the time that extract_query_dependencies is only able to capture dependencies using the RTEs in the *rewritten* query tree; it wouldn't have gone through the planner at that point. I think this (v4) patch is in the best shape so far. Thanks, Amit
pgsql-hackers by date: