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-19 03:20:35 |
Message-ID: | 21f2cf27-9dd8-22ce-bd6a-33e2c4feb8d6@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2016/10/18 22:04, Ashutosh Bapat wrote:
> 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.
OK
>> 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.
OK
>> 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.)
> 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.
Right, but as I said before, some FDW might consult FDW options stored
in those objects during AddForeignUpdateTargets, so we should do that.
>> 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.
I mean plan dependencies here, not query dependencies.
Having said that, I like the latest version (v6), so I'd vote for
marking this as Ready For Committer.
Best regards,
Etsuro Fujita
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2016-10-19 04:01:01 | Change of schedule for the next batch of PG update releases |
Previous Message | Peter Eisentraut | 2016-10-19 02:35:14 | Re: "make check" and pg_hba.conf |