Thanks to both of you for taking this up and sorry about the delay in responding.
On 2016/10/05 20:45, Etsuro Fujita wrote: > On 2016/10/05 14:09, Ashutosh Bapat wrote: >>> 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. I too am inclined to have a PlanCacheForeignCallback(). Just one minor comment: +static void +CheckGenericPlanDependencies(CachedPlanSource *plansource, + int cacheid, uint32 hashvalue) +{ + if (plansource->gplan && plansource->gplan->is_valid) + { How about move the if() to the callers so that: + /* + * Check the dependency list for the generic plan. + */ + if (plansource->gplan && plansource->gplan->is_valid) + CheckGenericPlanDependencies(plansource, cacheid, hashvalue); That will reduce the indentation level within the function definition. By the way, one wild idea may be to have a fixed number of callbacks based on their behavior, rather than which catalogs they are meant for. For example, have 2 suitably named callbacks: first that invalidates both CachedPlanSource and CachedPlan, second that invalidates only CachedPlan. Use the appropriate one whenever a new case arises where we decide to mark plans dependent on still other types of objects. Just an idea... 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