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

On Fri, Oct 7, 2016 at 7:20 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/10/07 10:26, Amit Langote wrote:
>>
>> On 2016/10/06 21:55, Etsuro Fujita wrote:
>>>
>>> On 2016/10/06 20:17, Amit Langote wrote:
>>>>
>>>> On 2016/10/05 20:45, Etsuro Fujita wrote:
>
>
>>> I noticed that we were wrong. Your patch was modified so that
>>> dependencies on FDW-related objects would be extracted from a given plan
>>> in create_foreignscan_plan (by Ashutosh) and then in
>>> set_foreignscan_references by me, but that wouldn't work well for INSERT
>>> cases. To fix that, I'd like to propose that we collect the dependencies
>>> from the given rte in add_rte_to_flat_rtable, instead.
>
>
>> I see. So, doing it from set_foreignscan_references() fails to capture
>> plan dependencies in case of INSERT because it won't be invoked at all
>> unlike the UPDATE/DELETE case.
>
>
> Right.
>
>>>>> If some writable FDW consulted foreign table/server/FDW options in
>>>>> AddForeignUpdateTarget, which adds the extra junk columns for
>>>>> UPDATE/DELETE to the targetList of the given query tree, the rewritten
>>>>> query tree would also need to be invalidated. But I don't think such
>>>>> an
>>>>> FDW really exists because that routine in a practical FDW wouldn't
>>>>> change
>>>>> such columns depending on those options.
>
>
>>> I had second thoughts about that; since the possibility wouldn't be zero,
>>> I added to extract_query_dependencies_walker the same code I added to
>>> add_rte_to_flat_rtable.
>
>
>> And here, since AddForeignUpdateTargets() could possibly utilize foreign
>> options which would cause *query tree* dependencies. It's possible that
>> add_rte_to_flat_rtable may not be called before an option change, causing
>> invalidation of any cached objects created based on the changed options.
>> So, must record dependencies from extract_query_dependencies as well.
>
>
> Right.
>
>> 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".

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

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company