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

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