From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Etsuro Fujita <fujita(dot)etsuro(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-07 01:26:04 |
Message-ID: | bade3546-e70e-2bb6-1a8d-50aaab8d23e1@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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:
>>> On 2016/10/05 14:09, Ashutosh Bapat wrote:
>>>> 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.
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.
> 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.
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.
> 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).
Agreed. I didn't know at the time that extract_query_dependencies is only
able to capture dependencies using the RTEs in the *rewritten* query tree;
it wouldn't have gone through the planner at that point.
I think this (v4) patch is in the best shape so far.
Thanks,
Amit
From | Date | Subject | |
---|---|---|---|
Next Message | Etsuro Fujita | 2016-10-07 01:50:38 | Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. |
Previous Message | Jeff Janes | 2016-10-07 00:40:35 | Re: VACUUM's ancillary tasks |