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:
I wrote:
So, I added a new callback function for those caches that is much like PlanCacheFuncCallback but skips checking the list for the query tree.
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.
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.
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).
Best regards, Etsuro Fujita
foreign_plan_cache_inval_v4.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
