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-04 11:30:18 |
Message-ID: | 3e5f6df3-9052-270f-ff7e-b92435c194e4@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2016/09/30 1:09, Ashutosh Bapat wrote:
You wrote:
>>> The attached patch adds the
>>> dependencies from create_foreignscan_plan() which is called for any
>>> foreign access. I have also added testcases to test the functionality.
>>> Let me know your comments on the patch.
I wrote:
>> Hmm. I'm not sure that that's a good idea.
>>
>> I was thinking the changes to setrefs.c proposed by Amit to collect that
>> dependencies would be probably OK, but I wasn't sure that it's a good idea
>> that he used PlanCacheFuncCallback as the syscache inval callback function
>> for the foreign object caches because it invalidates not only generic plans
>> but query trees, as you mentioned downthread. So, I was thinking to modify
>> his patch so that we add a new syscache inval callback function for the
>> caches that is much like PlanCacheFuncCallback but only invalidates generic
>> plans.
> PlanCacheFuncCallback() invalidates the query tree only when
> invalItems are added to the plan source. The patch adds the
> dependencies in root->glob->invalItems, which standard_planner()
> copies into PlannedStmt::invalItems. This is then copied into the
> gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
> query tree. I have verified this under the debugger. Am I missing
> something?
I think you are right. Maybe my explanation was not enough, sorry for
that. I just wanted to mention that Amit's patch would invalidate the
generic plan as well as the rewritten query tree.
I looked at your patch closely. You added code to track dependencies on
FDW-related objects to createplan.c, but I think it would be more
appropriate to put that code in setrefs.c like the attached. I think
record_foreignscan_plan_dependencies in your patch would be a bit
inefficient because that tracks such dependencies redundantly in the
case where the given ForeignScan has an outer subplan, so I optimized
that in the attached. (Also some regression tests for that were added.)
I think it would be a bit inefficient to use PlanCacheFuncCallback as
the inval callback function for those caches, because that checks the
inval-item list for the rewritten query tree, but any updates on such
catalogs wouldn't affect the query tree. So, I added a new callback
function for those caches that is much like PlanCacheFuncCallback but
skips checking the list for the query tree. I updated some comments also.
Best regards,
Etsuro Fujita
Attachment | Content-Type | Size |
---|---|---|
foreign_plan_cache_inval_v2.patch | binary/octet-stream | 23.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Julian Markwort | 2016-10-04 11:42:26 | Re: [PATCH] pgpassfile connection option |
Previous Message | Ashutosh Bapat | 2016-10-04 11:29:48 | Re: Transactions involving multiple postgres foreign servers |