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

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Amit Langote <amitlangote09(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan.
Date: 2016-10-18 12:48:51
Message-ID: b597199a-bbab-0005-b752-4dd554a054b6@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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.

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

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

Best regards,
Etsuro Fujita

Attachment Content-Type Size
foreign_plan_cache_inval_v6.patch binary/octet-stream 13.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-10-18 12:53:26 Re: Re: [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
Previous Message Gilles Darold 2016-10-18 12:18:36 Re: Patch to implement pg_current_logfile() function