Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan.

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