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

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-06 11:17:47
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Thanks to both of you for taking this up and sorry about the delay in

On 2016/10/05 20:45, Etsuro Fujita wrote:
> On 2016/10/05 14:09, Ashutosh Bapat wrote:
>>> 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.
>> I am not sure about that. Right now it seems that only the plans are
>> affected, but can we say that for all FDWs?
> 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.
>>> 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 am not sure that the inefficiency because of an extra loop on
>> plansource->invalItems is a good reason to duplicate most of the code
>> in PlanCacheFuncCallback(). 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 name of function may be
>> changed to be more generic, instead of current one referring Func.
> 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:

+static void
+CheckGenericPlanDependencies(CachedPlanSource *plansource,
+ int cacheid, uint32 hashvalue)
+ if (plansource->gplan && plansource->gplan->is_valid)
+ {

How about move the if() to the callers so that:

+ /*
+ * Check the dependency list for the generic plan.
+ */
+ if (plansource->gplan && plansource->gplan->is_valid)
+ CheckGenericPlanDependencies(plansource, cacheid, hashvalue);

That will reduce the indentation level within the function definition.

By the way, one wild idea may be to have a fixed number of callbacks based
on their behavior, rather than which catalogs they are meant for. For
example, have 2 suitably named callbacks: first that invalidates both
CachedPlanSource and CachedPlan, second that invalidates only CachedPlan.
Use the appropriate one whenever a new case arises where we decide to mark
plans dependent on still other types of objects. Just an idea...


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2016-10-06 11:19:48 Re: Fast AT ADD COLUMN with DEFAULTs
Previous Message Heikki Linnakangas 2016-10-06 10:37:16 Re: pg_rewind hangs if --source-server is used and syncrep is enabled