Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan.

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