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

Reply via email to