On 2016/09/30 1:09, Ashutosh Bapat wrote: You wrote:
The attached patch adds the dependencies from create_foreignscan_plan() which is called for any foreign access. I have also added testcases to test the functionality. Let me know your comments on the patch.
I wrote:
Hmm. I'm not sure that that's a good idea. I was thinking the changes to setrefs.c proposed by Amit to collect that dependencies would be probably OK, but I wasn't sure that it's a good idea that he used PlanCacheFuncCallback as the syscache inval callback function for the foreign object caches because it invalidates not only generic plans but query trees, as you mentioned downthread. So, I was thinking to modify his patch so that we add a new syscache inval callback function for the caches that is much like PlanCacheFuncCallback but only invalidates generic plans.
PlanCacheFuncCallback() invalidates the query tree only when invalItems are added to the plan source. The patch adds the dependencies in root->glob->invalItems, which standard_planner() copies into PlannedStmt::invalItems. This is then copied into the gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the query tree. I have verified this under the debugger. Am I missing something?
I think you are right. Maybe my explanation was not enough, sorry for that. I just wanted to mention that Amit's patch would invalidate the generic plan as well as the rewritten query tree.
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.)
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. 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.
Best regards, Etsuro Fujita
foreign_plan_cache_inval_v2.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
