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

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

Attachment: foreign_plan_cache_inval_v3.patch
Description: binary/octet-stream

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to