On 2016/10/17 20:12, Ashutosh Bapat wrote:

On 2016/10/07 10:26, Amit Langote wrote:
I think this (v4) patch is in the best shape so far.
+1, except one small change.

The comment "... On relcache invalidation events or the relevant
syscache invalidation events, ..." specifies relcache separately. I
think, it should either needs to specify all the syscaches or just
mention "On corresponding syscache invalidation events, ...".

Attached patch uses the later version. Let me know if this looks good.
If you are fine, I think we should mark this as "Ready for committer".

I'm not sure that is a good idea because ISTM the comments there use the words "syscache" and "relcache" properly. I'd like to leave that for committers.

The patch compiles cleanly, and make check in regress and that in
postgres_fdw shows no failures.

Thanks for the review and the updated patch!

One thing I'd like to propose to improve the patch is to update the following comments for PlanCacheFuncCallback: "Note that the coding would support use for multiple caches, but right now only user-defined functions are tracked this way.". We now use this for tracking FDW-related objects as well, so the comments needs to be updated. Please find attached an updated version.

Sorry for speaking this now, but one thing I'm not sure about the patch is whether we should track the dependencies on FDW-related objects more accurately; we modified extract_query_dependencies so that the query tree's dependencies are tracked, regardless of the command type, but the query tree would be only affected by those objects in AddForeignUpdateTargets, so it would be enough to collect the dependencies for the case where the given query is UPDATE/DELETE. But I thought the patch would be probably fine as proposed, because we expect updates on such catalogs to be infrequent. (I guess the changes needed for the accuracy would be small, though.) Besides that, I modified add_rte_to_flat_rtable so that the plan's dependencies are tracked, but that would lead to tracking the dependencies of unreferenced foreign tables in dead subqueries or the dependencies of foreign tables excluded from the plan by eg, constraint exclusion. But I thought that would be also OK by the same reason as above. (Another reason for that was it seemed better to me to collect the dependencies in the same place as for relation OIDs.)

Best regards,
Etsuro Fujita

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

-- 
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