Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. - Mailing list pgsql-hackers

>
> I looked at your patch closely.  You added code to track dependencies on
> FDW-related objects to createplan.c, but I think it would be more
> appropriate to put that code in setrefs.c like the attached.  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.  (Also some regression tests for that were added.)

Thanks for fixing the inefficiency.

+   /*
+    * 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."

+ * 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.
>
> 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?

> 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 updated some comments also.

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.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company