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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
