Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. - Mailing list pgsql-hackers

On 2016/10/18 22:04, Ashutosh Bapat wrote:
> On Tue, Oct 18, 2016 at 6:18 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> 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.

> Fine with me.

OK

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

> Fine with me.

OK

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

> In fact, I am not in
> favour of tracking the query dependencies for UPDATE/DELETE since we
> don't have any concrete example as to when that would be needed.

Right, but as I said before, some FDW might consult FDW options stored 
in those objects during AddForeignUpdateTargets, so we should do that.

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

> If those unreferenced relations become references because of the
> changes in options, we will require those query dependencies to be
> recorded. So, if we are recording query dependencies, we should record
> the ones on unreferenced relations as well.

I mean plan dependencies here, not query dependencies.

Having said that, I like the latest version (v6), so I'd vote for 
marking this as Ready For Committer.

Best regards,
Etsuro Fujita