On 2016/10/05 14:09, Ashutosh Bapat wrote: I wrote:
I think record_foreignscan_plan_dependencies in your patch would be a bit inefficient because that tracks such dependencies redundantly in the case where the given ForeignScan has an outer subplan, so I optimized that in the attached.
+ /* + * Record dependencies on FDW-related objects. If an outer subplan + * exists, that would be done in the processing of its baserels, so skip + * that. + */ I think, we need a bit more explanation here. How about rewording it as "Record dependencies on FDW-related objects. If an outer subplan exists, the dependencies would be recorded while processing the foreign plans for the base foreign relations in the subplan. Hence skip that here."
Agreed. I updated the comments as proposed.
+ * Currently, we track exactly the dependencies of plans on relations, + * user-defined functions and FDW-related objects. On relcache invalidation + * events or pg_proc syscache invalidation events, we invalidate just those + * plans that depend on the particular object being modified. (Note: this + * scheme assumes that any table modification that requires replanning will + * generate a relcache inval event.) We also watch for inval events on + * certain other system catalogs, such as pg_namespace; but for them, our + * response is just to invalidate all plans. We expect updates on those + * catalogs to be infrequent enough that more-detailed tracking is not worth + * the effort. Just like you have added FDW-related objects in the first sentence, reference to those needs to be added in the second sentence as well. Or you want to start the second sentence as "When the relevant caches are invalidated, we invalidate ..." so that those two sentences will remain in sync even after additions/deletions to the first sentence.
Good catch! I like the second one. How about starting the second sentence as "On relcache invalidation events or the relevant syscache invalidation events, we invalidate ..."?
I think it would be a bit inefficient to use PlanCacheFuncCallback as the inval callback function for those caches, because that checks the inval-item list for the rewritten query tree, but any updates on such catalogs wouldn't affect the query tree.
I am not sure about that. Right now it seems that only the plans are affected, but can we say that for all FDWs?
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.
So, I added a new callback function for those caches that is much like PlanCacheFuncCallback but skips checking the list for the query tree.
I am not sure that the inefficiency because of an extra loop on plansource->invalItems is a good reason to duplicate most of the code in PlanCacheFuncCallback(). 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 name of function may be changed to be more generic, instead of current one referring Func.
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.
Attached is the next version of the patch. Thanks for the comments! Best regards, Etsuro Fujita
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers