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: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: 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-06 12:55:20
Message-ID: 09bd593a-7fe2-34a2-9bad-3e4d091f1e11@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2016/10/06 20:17, Amit Langote wrote:
> On 2016/10/05 20:45, Etsuro Fujita wrote:
>> On 2016/10/05 14:09, Ashutosh Bapat wrote:

I wrote:
>>>> So, I added a new callback function for those caches
>>>> that is much like PlanCacheFuncCallback but skips checking the list for
>>>> the
>>>> query tree.

>>> IMO, maintaining that extra function and
>>> the risk of bugs because of not keeping those two functions in sync
>>> outweigh the small not-so-frequent gain.

>> The inefficiency wouldn't be negligible in the case where there are large
>> numbers of cached plans. So, I'd like to introduce a helper function that
>> checks the dependency list for the generic plan, to eliminate most of the
>> duplication.

> I too am inclined to have a PlanCacheForeignCallback().
>
> Just one minor comment:

Thanks for the comments!

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.

Attached is an updated version, in which I removed the
PlanCacheForeignCallback and adjusted regression tests a bit.

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

After all, the updated patch is much like your version, but I think your
patch, which modified extract_query_dependencies_walker only, is not
enough because a generic plan can have more dependencies than its query
tree (eg, consider inheritance).

Best regards,
Etsuro Fujita

Attachment Content-Type Size
foreign_plan_cache_inval_v4.patch binary/octet-stream 14.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-10-06 13:46:33 Re: Switch to unnamed POSIX semaphores as our preferred sema code?
Previous Message Michael Paquier 2016-10-06 12:50:12 Re: psql autocomplete works not good in USER command in 9.6