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

On Tue, Oct 18, 2016 at 6:18 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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.

>
>> The patch compiles cleanly, and make check in regress and that in
>> postgres_fdw shows no failures.
>
>
> Thanks for the review and the updated patch!
>
> 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.

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

If we do as you suggest, we will have to add separate code for
tracking plan dependencies for SELECT queries. 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.

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

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