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

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

Reply via email to