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

<div dir="ltr"><span style="font-size:small">Hi,</span><br /><div class="gmail_quote"><div dir="ltr"><font face="arial,
helvetica,sans-serif"><span style="font-size:small"><br />I observed below in postgres_fdw</span></font><u> .<br /><br
/>Observation:</u>Prepare statement execution plan is not getting changed even after altering foreign table to point to
newschema.<br /><div class="gmail_extra"><div class="gmail_quote"><div
style="color:rgb(0,0,0);font-family:Helvetica;font-size:12px"><br/>CREATE EXTENSION postgres_fdw;<br />CREATE SCHEMA
s1;<br/>create table <a href="http://s1.lt">s1.lt</a> (c1 integer, c2 varchar);<br />insert into <a
href="http://s1.lt">s1.lt</a>values (1, '<a href="http://s1.lt">s1.lt</a>');<br />CREATE SCHEMA s2;<br />create table
<ahref="http://s2.lt">s2.lt</a> (c1 integer, c2 varchar);<br />insert into <a href="http://s2.lt">s2.lt</a> values (1,
'<ahref="http://s2.lt">s2.lt</a>');<br />CREATE SERVER link_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
'postgres',port '5447', use_remote_estimate 'true');<br />CREATE USER MAPPING FOR public SERVER link_server;<br /><br
/>createforeign table ft (c1 integer, c2 varchar) server link_server options (schema_name 's1',table_name 'lt');<br
/><br/>ANALYZE ft;<br />PREPARE stmt_ft AS select c1,c2 from ft;<br /><br />EXECUTE stmt_ft;<br /> c1 |  c2   <br
/>----+-------<br/>  1 | <a href="http://s1.lt">s1.lt</a><br />(1 row)<br /><br />--changed foreign table ft pointing
schemafrom s1 to s2<br />ALTER foreign table ft options (SET schema_name 's2', SET table_name 'lt');<br />ANALYZE
ft;<br/><br />EXPLAIN (COSTS OFF, VERBOSE) EXECUTE stmt_ft;<br />               QUERY PLAN               <br
/>----------------------------------------<br/> Foreign Scan on public.ft<br />   Output: c1, c2<br />   Remote SQL:
SELECTc1, c2 FROM <a href="http://s1.lt">s1.lt</a><br />(3 rows)<br /><br />EXECUTE stmt_ft;<br /> c1 |  c2   <br
/>----+-------<br/>  1 | <a href="http://s1.lt">s1.lt</a><br />(1 row)<br /><br /><span
style="font-size:13px"></span><spanstyle="color:rgb(0,0,0);font-family:Helvetica;font-size:12px">Thanks &
Regards,</span><br/>Rajkumar Raghuwanshi<br />QMG, EnterpriseDB Corporation<br /></div><blockquote class="gmail_quote"
style="margin:0px0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div
class="gmail_quote"><divdir="ltr"><div class="gmail_quote"><div
dir="ltr"></div></div></div></div></div></blockquote></div></div></div></div></div>

Hi,

Thanks for the report.

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:
> Hi,
>
> I observed below in postgres_fdw
>
> * .Observation: *Prepare statement execution plan is not getting changed
> even after altering foreign table to point to new schema.
>

[ ... ]

> PREPARE stmt_ft AS select c1,c2 from ft;
>
> EXECUTE stmt_ft;
>  c1 |  c2
> ----+-------
>   1 | s1.lt
> (1 row)
>
> --changed foreign table ft pointing schema from s1 to s2
> ALTER foreign table ft options (SET schema_name 's2', SET table_name 'lt');
> ANALYZE ft;
>
> EXPLAIN (COSTS OFF, VERBOSE) EXECUTE stmt_ft;
>                QUERY PLAN
> ----------------------------------------
>  Foreign Scan on public.ft
>    Output: c1, c2
>    Remote SQL: SELECT c1, c2 FROM s1.lt
> (3 rows)

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem.  The attached patch fixes the
issue for me.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:
>> * .Observation: *Prepare statement execution plan is not getting changed
>> even after altering foreign table to point to new schema.

> I wonder if performing relcache invalidation upon ATExecGenericOptions()
> is the correct solution for this problem.  The attached patch fixes the
> issue for me.

A forced relcache inval will certainly fix it, but I'm not sure if that's
the best (or only) place to put it.

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either.  I'm
not sure there's any very good fix for that.  Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.
        regards, tom lane

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:
>>> * .Observation: *Prepare statement execution plan is not getting changed
>>> even after altering foreign table to point to new schema.
>
>> I wonder if performing relcache invalidation upon ATExecGenericOptions()
>> is the correct solution for this problem.  The attached patch fixes the
>> issue for me.
>
> A forced relcache inval will certainly fix it, but I'm not sure if that's
> the best (or only) place to put it.
>
> A related issue, now that I've seen this example, is that altering
> FDW-level or server-level options won't cause a replan either.  I'm
> not sure there's any very good fix for that.  Surely we don't want
> to try to identify all tables belonging to the FDW or server and
> issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe? Or
some solution on lines of recent PlanCacheUserMappingCallback() added
by fbe5a3fb, wherein I see this comment which sounds like a similar
solution could be built for servers and FDWs:

+       /*
+        * If the plan has pushed down foreign joins, those join may become
+        * unsafe to push down because of user mapping changes. Invalidate only
+        * the generic plan, since changes to user mapping do not invalidate the
+        * parse tree.
+        */

Missing something am I?

Thanks,
Amit

Amit Langote <amitlangote09@gmail.com> writes:
> On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A related issue, now that I've seen this example, is that altering
>> FDW-level or server-level options won't cause a replan either.  I'm
>> not sure there's any very good fix for that.  Surely we don't want
>> to try to identify all tables belonging to the FDW or server and
>> issue relcache invals on all of them.

> Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them?  That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.
        regards, tom lane

At Mon, 04 Apr 2016 11:23:34 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <9798.1459783414@sss.pgh.pa.us>
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> A related issue, now that I've seen this example, is that altering
> >> FDW-level or server-level options won't cause a replan either.  I'm
> >> not sure there's any very good fix for that.  Surely we don't want
> >> to try to identify all tables belonging to the FDW or server and
> >> issue relcache invals on all of them.
> 
> > Hm, some kind of PlanInvalItem-based solution could work maybe?
> 
> Hm, so we'd expect that whenever an FDW consulted the options while
> making a plan, it'd have to record a plan dependency on them?  That
> would be a clean fix maybe, but I'm worried that third-party FDWs
> would fail to get the word about needing to do this.

If the "recording" means recoding oids to CachedPlanSource like
relationOids, it seems to be able to be recorded automatically by
the core, not by fdw side, we already know the server id for
foreign tables.

I'm missing something?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2016/04/05 0:23, Tom Lane wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
>> On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> A related issue, now that I've seen this example, is that altering
>>> FDW-level or server-level options won't cause a replan either.  I'm
>>> not sure there's any very good fix for that.  Surely we don't want
>>> to try to identify all tables belonging to the FDW or server and
>>> issue relcache invals on all of them.
>
>> Hm, some kind of PlanInvalItem-based solution could work maybe?
>
> Hm, so we'd expect that whenever an FDW consulted the options while
> making a plan, it'd have to record a plan dependency on them?  That
> would be a clean fix maybe, but I'm worried that third-party FDWs
> would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options.  I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors.  How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems.  Also, it adds plan cache
callbacks for respective caches.

One thing that I observed that may not be all that surprising is that we
may need a similar mechanism for postgres_fdw's connection cache, which
doesn't drop connections using older server connection info after I alter
them.  I was trying to test my patch by altering dbaname option of a
foreign server but that was silly, ;).  Although, I did confirm that the
patch works by altering use_remote_estimates server option. I could not
really test for FDW options though.

Thoughts?

Thanks,
Amit

At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<57034C24.1000203@lab.ntt.co.jp>
> On 2016/04/05 0:23, Tom Lane wrote:
> > Amit Langote <amitlangote09@gmail.com> writes:
> >> On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> A related issue, now that I've seen this example, is that altering
> >>> FDW-level or server-level options won't cause a replan either.  I'm
> >>> not sure there's any very good fix for that.  Surely we don't want
> >>> to try to identify all tables belonging to the FDW or server and
> >>> issue relcache invals on all of them.
> > 
> >> Hm, some kind of PlanInvalItem-based solution could work maybe?
> > 
> > Hm, so we'd expect that whenever an FDW consulted the options while
> > making a plan, it'd have to record a plan dependency on them?  That
> > would be a clean fix maybe, but I'm worried that third-party FDWs
> > would fail to get the word about needing to do this.
> 
> I would imagine that that level of granularity may be a little too much; I
> mean tracking dependencies at the level of individual FDW/foreign
> table/foreign server options.  I think it should really be possible to do
> the entire thing in core instead of requiring this to be made a concern of
> FDW authors.  How about the attached that teaches
> extract_query_dependencies() to add a foreign table and associated foreign
> data wrapper and foreign server to invalItems.  Also, it adds plan cache
> callbacks for respective caches.

It seems to work but some renaming of functions would needed.

> One thing that I observed that may not be all that surprising is that we
> may need a similar mechanism for postgres_fdw's connection cache, which
> doesn't drop connections using older server connection info after I alter
> them.  I was trying to test my patch by altering dbaname option of a
> foreign server but that was silly, ;).  Although, I did confirm that the
> patch works by altering use_remote_estimates server option. I could not
> really test for FDW options though.
> 
> Thoughts?

It seems a issue of FDW drivers but notification can be issued by
the core. The attached ultra-POC patch does it.


Disconnecting of a fdw connection with active transaction causes
an unexpected rollback on the remote side. So disconnection
should be allowed only when xact_depth == 0 in
pgfdw_subxact_callback().

There are so many parameters that affect connections, which are
listed as PQconninfoOptions. It is a bit too complex to detect
changes of one or some of them. So, I try to use object access
hook even though using hook is not proper as fdw interface. An
additional interface would be needed to do this anyway.

With this patch, making any change on foreign servers or user
mappings corresponding to exiting connection causes
disconnection. This could be moved in to core, with the following
API like this.

reoutine->NotifyObjectModification(ObjectAccessType access,             Oid classId, Oid objId);

- using object access hook (which is put by the core itself) is not bad?

- If ok, the API above looks bad. Any better API?


By the way, AlterUserMapping seems missing calling
InvokeObjectPostAlterHook. Is this a bug?


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 189f290..3d1b4fa 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -47,6 +47,7 @@ typedef struct ConnCacheEntry                                 * one level of subxact open, etc */
bool       have_prep_stmt; /* have we prepared any stmts in this xact? */    bool        have_error;        /* have any
subxactsaborted in this xact? */
 
+    bool        to_be_disconnected;} ConnCacheEntry;/*
@@ -137,6 +138,7 @@ GetConnection(UserMapping *user, bool will_prep_stmt)        entry->xact_depth = 0;
entry->have_prep_stmt= false;        entry->have_error = false;
 
+        entry->to_be_disconnected = false;    }    /*
@@ -145,6 +147,14 @@ GetConnection(UserMapping *user, bool will_prep_stmt)     * connection is actually used.     */
+    if (entry->conn != NULL &&
+        entry->to_be_disconnected && entry->xact_depth == 0)
+    {
+        elog(LOG, "disconnected");
+        PQfinish(entry->conn);
+        entry->conn = NULL;
+        entry->to_be_disconnected = false;
+    }    /*     * If cache entry doesn't have a connection, we have to establish a new     * connection.  (If
connect_pg_serverthrows an error, the cache entry
 
@@ -270,6 +280,26 @@ connect_pg_server(ForeignServer *server, UserMapping *user)    return conn;}
+void
+reserve_pg_disconnect(Oid umid)
+{
+    bool        found;
+    ConnCacheEntry *entry;
+    ConnCacheKey key;
+
+    if (ConnectionHash == NULL)
+        return;
+
+    /* Find existing connectionentry */
+    key = umid;
+    entry = hash_search(ConnectionHash, &key, HASH_ENTER, &found);
+
+    if (!found || entry->conn == NULL)
+        return;
+
+    entry->to_be_disconnected = true;
+}
+/* * For non-superusers, insist that the connstr specify a password.  This * prevents a password from being picked up
from.pgpass, a service file,
 
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4fbbde1..7777875 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -16,6 +16,9 @@#include "access/htup_details.h"#include "access/sysattr.h"
+#include "catalog/pg_foreign_server.h"
+#include "catalog/pg_user_mapping.h"
+#include "catalog/objectaccess.h"#include "commands/defrem.h"#include "commands/explain.h"#include
"commands/vacuum.h"
@@ -33,7 +36,9 @@#include "optimizer/tlist.h"#include "parser/parsetree.h"#include "utils/builtins.h"
+#include "utils/fmgroids.h"#include "utils/guc.h"
+#include "utils/syscache.h"#include "utils/lsyscache.h"#include "utils/memutils.h"#include "utils/rel.h"
@@ -407,7 +412,16 @@ static List *get_useful_pathkeys_for_relation(PlannerInfo *root,static List
*get_useful_ecs_for_relation(PlannerInfo*root, RelOptInfo *rel);static void add_paths_with_pathkeys_for_rel(PlannerInfo
*root,RelOptInfo *rel,                                Path *epq_path);
 
+static void postgresObjectAccessHook(ObjectAccessType access,
+                                                     Oid classId,
+                                                     Oid objectId,
+                                                     int subId,
+                                                     void *arg);
+                                     
+static object_access_hook_type previous_object_access_hook;
+void        _PG_init(void);
+void        _PG_fini(void);/* * Foreign-data wrapper handler function: return a struct with pointers
@@ -460,6 +474,19 @@ postgres_fdw_handler(PG_FUNCTION_ARGS)    PG_RETURN_POINTER(routine);}
+void
+_PG_init(void)
+{
+    previous_object_access_hook = object_access_hook;
+    object_access_hook = postgresObjectAccessHook;
+}
+
+void
+_PG_fini(void)
+{
+    object_access_hook = previous_object_access_hook;
+}
+/* * postgresGetForeignRelSize *        Estimate # of rows and width of the result of the scan
@@ -4492,3 +4519,43 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)    /* We didn't find any suitable
equivalenceclass expression */    return NULL;}
 
+
+static void
+postgresObjectAccessHook(ObjectAccessType access,
+                         Oid classId,
+                         Oid objectId,
+                         int subId,
+                         void *arg)
+{
+    if (access == OAT_POST_ALTER)
+    {
+        if (classId == ForeignServerRelationId)
+        {
+            Relation umrel;
+            ScanKeyData skey;
+            SysScanDesc scan;
+            HeapTuple umtup;
+
+            umrel = heap_open(UserMappingRelationId, AccessShareLock);
+            ScanKeyInit(&skey,
+                        Anum_pg_user_mapping_umserver,
+                        BTEqualStrategyNumber, F_OIDEQ,
+                        ObjectIdGetDatum(objectId));
+            scan = systable_beginscan(umrel, InvalidOid, false,
+                                      NULL, 1, &skey);
+            while(HeapTupleIsValid(umtup = systable_getnext(scan)))
+                reserve_pg_disconnect(HeapTupleGetOid(umtup));
+
+            systable_endscan(scan);
+            heap_close(umrel, AccessShareLock);
+        }
+        if (classId == UserMappingRelationId)
+        {
+            reserve_pg_disconnect(objectId);
+        }
+    }
+        
+
+    if (previous_object_access_hook)
+        previous_object_access_hook(access, classId, objectId, subId, arg);
+}
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 3a11d99..7e8ea31 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -100,6 +100,7 @@ extern void reset_transmission_modes(int nestlevel);/* in connection.c */extern PGconn
*GetConnection(UserMapping*user, bool will_prep_stmt);
 
+extern void reserve_pg_disconnect(Oid umid);extern void ReleaseConnection(PGconn *conn);extern unsigned int
GetCursorNumber(PGconn*conn);extern unsigned int GetPrepStmtNumber(PGconn *conn);
 
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 804bab2..ebae253 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1312,6 +1312,9 @@ AlterUserMapping(AlterUserMappingStmt *stmt)    ObjectAddressSet(address, UserMappingRelationId,
umId);
+    /* Post alter hook for new user mapping */
+    InvokeObjectPostAlterHook(UserMappingRelationId, umId, 0);
+    heap_freetuple(tp);    heap_close(rel, RowExclusiveLock);

On 2016/04/05 18:44, Kyotaro HORIGUCHI wrote:
> At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote wrote:
>> On 2016/04/05 0:23, Tom Lane wrote:
>>> Amit Langote <amitlangote09@gmail.com> writes:
>>>> Hm, some kind of PlanInvalItem-based solution could work maybe?
>>>
>>> Hm, so we'd expect that whenever an FDW consulted the options while
>>> making a plan, it'd have to record a plan dependency on them?  That
>>> would be a clean fix maybe, but I'm worried that third-party FDWs
>>> would fail to get the word about needing to do this.
>>
>> I would imagine that that level of granularity may be a little too much; I
>> mean tracking dependencies at the level of individual FDW/foreign
>> table/foreign server options.  I think it should really be possible to do
>> the entire thing in core instead of requiring this to be made a concern of
>> FDW authors.  How about the attached that teaches
>> extract_query_dependencies() to add a foreign table and associated foreign
>> data wrapper and foreign server to invalItems.  Also, it adds plan cache
>> callbacks for respective caches.
> 
> It seems to work but some renaming of functions would needed.

Yeah, I felt the names were getting quite long, too :)

>> One thing that I observed that may not be all that surprising is that we
>> may need a similar mechanism for postgres_fdw's connection cache, which
>> doesn't drop connections using older server connection info after I alter
>> them.  I was trying to test my patch by altering dbaname option of a
>> foreign server but that was silly, ;).  Although, I did confirm that the
>> patch works by altering use_remote_estimates server option. I could not
>> really test for FDW options though.
>>
>> Thoughts?
> 
> It seems a issue of FDW drivers but notification can be issued by
> the core. The attached ultra-POC patch does it.
> 
> 
> Disconnecting of a fdw connection with active transaction causes
> an unexpected rollback on the remote side. So disconnection
> should be allowed only when xact_depth == 0 in
> pgfdw_subxact_callback().
>
> There are so many parameters that affect connections, which are
> listed as PQconninfoOptions. It is a bit too complex to detect
> changes of one or some of them. So, I try to use object access
> hook even though using hook is not proper as fdw interface. An
> additional interface would be needed to do this anyway.
> 
> With this patch, making any change on foreign servers or user
> mappings corresponding to exiting connection causes
> disconnection. This could be moved in to core, with the following
> API like this.
> 
> reoutine->NotifyObjectModification(ObjectAccessType access,
>                  Oid classId, Oid objId);
> 
> - using object access hook (which is put by the core itself) is not bad?
> 
> - If ok, the API above looks bad. Any better API?

Although helps achieve our goal here, object access hook stuff seems to be
targeted at different users:

/** Hook on object accesses.  This is intended as infrastructure for security* and logging plugins.*/
object_access_hook_type object_access_hook = NULL;


I just noticed the following comment above GetConnection():
** XXX Note that caching connections theoretically requires a mechanism to* detect change of FDW objects to invalidate
alreadyestablished connections.* We could manage that by watching for invalidation events on the relevant* syscaches.
Forthe moment, though, it's not clear that this would really* be useful and not mere pedantry.  We could not flush any
activeconnections* mid-transaction anyway.
 


So, the hazard of flushing the connection mid-transaction sounds like it
may be a show-stopper here, even if we research some approach based on
syscache invalidation.  Although I see that your patch takes care of it.

> By the way, AlterUserMapping seems missing calling
> InvokeObjectPostAlterHook. Is this a bug?

Probably, yes.

Thanks,
Amit

Hi,

At Tue, 5 Apr 2016 19:46:04 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<5703976C.30308@lab.ntt.co.jp>
> On 2016/04/05 18:44, Kyotaro HORIGUCHI wrote:
> > At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote wrote:
> > With this patch, making any change on foreign servers or user
> > mappings corresponding to exiting connection causes
> > disconnection. This could be moved in to core, with the following
> > API like this.
> > 
> > reoutine->NotifyObjectModification(ObjectAccessType access,
> >                  Oid classId, Oid objId);
> > 
> > - using object access hook (which is put by the core itself) is not bad?
> > 
> > - If ok, the API above looks bad. Any better API?
> 
> Although helps achieve our goal here, object access hook stuff seems to be
> targeted at different users:
> 
> /*
>  * Hook on object accesses.  This is intended as infrastructure for security
>  * and logging plugins.
>  */
> object_access_hook_type object_access_hook = NULL;

Yeah, furthermore, it doesn't notify to other backends, that is
what I forgotten at the time:(

So, it seems reasonable that all stuffs ride on the cache
invalidation mechanism.

Object class id and object id should be necessary to be
adequitely notificated to fdws but the current invalidation
mechanism donesn't convey the latter. It is hidden in hash
code. This is resolved just by additional member in PlanInvalItem
or resolving oid from hash code(this would need catalog scan..).

PlanCache*Callback() just do invalidtion and throw the causeal
PlanInvalItem away. If plancache had oid lists of foreign
servers, foreign tables, user mappings or FDWS, we can notify
FDWs of invalidation with the causal object.


- Adding CachedPlanSource with some additional oid lists, which will be built by extract_query_dependencies.

- In PlanCache*Callback(), class id and object id of the causal object is notified to FDW.


> I just noticed the following comment above GetConnection():
> 
>  *
>  * XXX Note that caching connections theoretically requires a mechanism to
>  * detect change of FDW objects to invalidate already established connections.
>  * We could manage that by watching for invalidation events on the relevant
>  * syscaches.  For the moment, though, it's not clear that this would really
>  * be useful and not mere pedantry.  We could not flush any active connections
>  * mid-transaction anyway.
> 
> 
> So, the hazard of flushing the connection mid-transaction sounds like it
> may be a show-stopper here, even if we research some approach based on
> syscache invalidation.  Although I see that your patch takes care of it.

Yeah. Altough cache invalidation would accur amid transaction..

> > By the way, AlterUserMapping seems missing calling
> > InvokeObjectPostAlterHook. Is this a bug?
> 
> Probably, yes.

But we dont' see a proper way to "fix" this since we here don't
use this.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2016/04/05 14:24, Amit Langote wrote:
> On 2016/04/05 0:23, Tom Lane wrote:
>> Amit Langote <amitlangote09@gmail.com> writes:
>>> Hm, some kind of PlanInvalItem-based solution could work maybe?
>>
>> Hm, so we'd expect that whenever an FDW consulted the options while
>> making a plan, it'd have to record a plan dependency on them?  That
>> would be a clean fix maybe, but I'm worried that third-party FDWs
>> would fail to get the word about needing to do this.
> 
> I would imagine that that level of granularity may be a little too much; I
> mean tracking dependencies at the level of individual FDW/foreign
> table/foreign server options.  I think it should really be possible to do
> the entire thing in core instead of requiring this to be made a concern of
> FDW authors.  How about the attached that teaches
> extract_query_dependencies() to add a foreign table and associated foreign
> data wrapper and foreign server to invalItems.  Also, it adds plan cache
> callbacks for respective caches.
> 
> One thing that I observed that may not be all that surprising is that we
> may need a similar mechanism for postgres_fdw's connection cache, which
> doesn't drop connections using older server connection info after I alter
> them.  I was trying to test my patch by altering dbaname option of a
> foreign server but that was silly, ;).  Although, I did confirm that the
> patch works by altering use_remote_estimates server option. I could not
> really test for FDW options though.
> 
> Thoughts?

I added this to next CF, just in case:
https://commitfest.postgresql.org/10/609/

Thanks,
Amit

On 2016/04/04 23:24, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:
>>> * .Observation: *Prepare statement execution plan is not getting changed
>>> even after altering foreign table to point to new schema.

>> I wonder if performing relcache invalidation upon ATExecGenericOptions()
>> is the correct solution for this problem.  The attached patch fixes the
>> issue for me.

> A forced relcache inval will certainly fix it, but I'm not sure if that's
> the best (or only) place to put it.

> A related issue, now that I've seen this example, is that altering
> FDW-level or server-level options won't cause a replan either.  I'm
> not sure there's any very good fix for that.  Surely we don't want
> to try to identify all tables belonging to the FDW or server and
> issue relcache invals on all of them.

While improving join pushdown in postgres_fdw, I happened to notice that  
the fetch_size option in 9.6 has the same issue.  A forced replan  
discussed above would fix that issue, but I'm not sure that that's a  
good idea, because the fetch_size option, which postgres_fdw gets at  
GetForeignRelSize, is not used for anything in creating a plan.  So, as  
far as the fetch_size option is concerned, a better solution is (1) to  
consult that option at execution time, probably at BeginForeignScan, not  
at planning time such as GetForiegnRelSize (and (2) to not cause that  
replan when altering that option.)  Maybe I'm missing something, though.

Best regards,
Etsuro Fujita

On Tue, Apr 5, 2016 at 10:54 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2016/04/05 0:23, Tom Lane wrote:
>> Amit Langote <amitlangote09@gmail.com> writes:
>>> On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> A related issue, now that I've seen this example, is that altering
>>>> FDW-level or server-level options won't cause a replan either.  I'm
>>>> not sure there's any very good fix for that.  Surely we don't want
>>>> to try to identify all tables belonging to the FDW or server and
>>>> issue relcache invals on all of them.
>>
>>> Hm, some kind of PlanInvalItem-based solution could work maybe?
>>
>> Hm, so we'd expect that whenever an FDW consulted the options while
>> making a plan, it'd have to record a plan dependency on them?  That
>> would be a clean fix maybe, but I'm worried that third-party FDWs
>> would fail to get the word about needing to do this.
>
> I would imagine that that level of granularity may be a little too much; I
> mean tracking dependencies at the level of individual FDW/foreign
> table/foreign server options.  I think it should really be possible to do
> the entire thing in core instead of requiring this to be made a concern of
> FDW authors.  How about the attached that teaches
> extract_query_dependencies() to add a foreign table and associated foreign
> data wrapper and foreign server to invalItems.  Also, it adds plan cache
> callbacks for respective caches.

Although this is a better solution than the previous one, it
invalidates the query tree along with the generic plan. Invalidating
the query tree and the generic plan required parsing the query again
and generating the plan. Instead I think, we should invalidate only
the generic plan and not the query tree. 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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Wed, Aug 24, 2016 at 1:55 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/04/04 23:24, Tom Lane wrote:
>>
>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>>
>>> On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:
>>>>
>>>> * .Observation: *Prepare statement execution plan is not getting changed
>>>> even after altering foreign table to point to new schema.
>
>
>>> I wonder if performing relcache invalidation upon ATExecGenericOptions()
>>> is the correct solution for this problem.  The attached patch fixes the
>>> issue for me.
>
>
>> A forced relcache inval will certainly fix it, but I'm not sure if that's
>> the best (or only) place to put it.
>
>
>> A related issue, now that I've seen this example, is that altering
>> FDW-level or server-level options won't cause a replan either.  I'm
>> not sure there's any very good fix for that.  Surely we don't want
>> to try to identify all tables belonging to the FDW or server and
>> issue relcache invals on all of them.
>
>
> While improving join pushdown in postgres_fdw, I happened to notice that the
> fetch_size option in 9.6 has the same issue.  A forced replan discussed
> above would fix that issue, but I'm not sure that that's a good idea,
> because the fetch_size option, which postgres_fdw gets at GetForeignRelSize,
> is not used for anything in creating a plan.  So, as far as the fetch_size
> option is concerned, a better solution is (1) to consult that option at
> execution time, probably at BeginForeignScan, not at planning time such as
> GetForiegnRelSize (and (2) to not cause that replan when altering that
> option.)  Maybe I'm missing something, though.

As explained in my latest patch, an FDW knows which of the options,
when changed, render a plan invalid. If we have to track the changes
for each option, an FDW will need to consulted to check whether that
options affects the plan or not. That may be an overkill and there is
high chance that the FDW authors wouldn't bother implementing that
hook. Let me know your views on my latest patch on this thread.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/09/29 20:06, Ashutosh Bapat wrote:
> On Wed, Aug 24, 2016 at 1:55 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2016/04/04 23:24, Tom Lane wrote:
>>> A related issue, now that I've seen this example, is that altering
>>> FDW-level or server-level options won't cause a replan either.  I'm
>>> not sure there's any very good fix for that.  Surely we don't want
>>> to try to identify all tables belonging to the FDW or server and
>>> issue relcache invals on all of them.

>> While improving join pushdown in postgres_fdw, I happened to notice that the
>> fetch_size option in 9.6 has the same issue.  A forced replan discussed
>> above would fix that issue, but I'm not sure that that's a good idea,
>> because the fetch_size option, which postgres_fdw gets at GetForeignRelSize,
>> is not used for anything in creating a plan.  So, as far as the fetch_size
>> option is concerned, a better solution is (1) to consult that option at
>> execution time, probably at BeginForeignScan, not at planning time such as
>> GetForiegnRelSize (and (2) to not cause that replan when altering that
>> option.)  Maybe I'm missing something, though.

> As explained in my latest patch, an FDW knows which of the options,
> when changed, render a plan invalid. If we have to track the changes
> for each option, an FDW will need to consulted to check whether that
> options affects the plan or not. That may be an overkill and there is
> high chance that the FDW authors wouldn't bother implementing that
> hook.

I thought it would be better to track that dependencies to avoid useless 
replans, but I changed my mind; I agree with Amit's approach.  In 
addition to what you mentioned, ISTM that users don't change such 
options frequently, so I think Amit's approach is much practical.

> Let me know your views on my latest patch on this thread.

OK, will do.

Best regards,
Etsuro Fujita

On 2016/09/29 20:03, Ashutosh Bapat wrote:
> On Tue, Apr 5, 2016 at 10:54 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> How about the attached that teaches
>> extract_query_dependencies() to add a foreign table and associated foreign
>> data wrapper and foreign server to invalItems.  Also, it adds plan cache
>> callbacks for respective caches.

> Although this is a better solution than the previous one, it
> invalidates the query tree along with the generic plan. Invalidating
> the query tree and the generic plan required parsing the query again
> and generating the plan. Instead I think, we should invalidate only
> the generic plan and not the query tree.

Agreed.

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

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.

Best regards,
Etsuro Fujita

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

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Fri, Sep 30, 2016 at 1:09 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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.
>>
>>
>> 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?

Moved to next CF. Feel free to continue the work on this item.
-- 
Michael

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

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

Thanks for fixing the inefficiency.

+   /*
+    * Record dependencies on FDW-related objects.  If an outer subplan
+    * exists, that would be done in the processing of its baserels, so skip
+    * that.
+    */
I think, we need a bit more explanation here. How about rewording it
as "Record dependencies on FDW-related objects. If an outer subplan
exists, the dependencies would be recorded while processing the
foreign plans for the base foreign relations in the subplan. Hence
skip that here."

+ * Currently, we track exactly the dependencies of plans on relations,
+ * user-defined functions and FDW-related objects.  On relcache invalidation
+ * events or pg_proc syscache invalidation events, we invalidate just those
+ * plans that depend on the particular object being modified.  (Note: this
+ * scheme assumes that any table modification that requires replanning will
+ * generate a relcache inval event.)  We also watch for inval events on
+ * certain other system catalogs, such as pg_namespace; but for them, our
+ * response is just to invalidate all plans.  We expect updates on those
+ * catalogs to be infrequent enough that more-detailed tracking is not worth
+ * the effort.

Just like you have added FDW-related objects in the first sentence,
reference to those needs to be added in the second sentence as well.
Or you want to start the second sentence as "When the relevant caches
are invalidated, we invalidate ..." so that those two sentences will
remain in sync even after additions/deletions to the first sentence.
>
> 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?

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

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.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/10/05 14:09, Ashutosh Bapat wrote:

I wrote:
>> 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.

> +   /*
> +    * Record dependencies on FDW-related objects.  If an outer subplan
> +    * exists, that would be done in the processing of its baserels, so skip
> +    * that.
> +    */
> I think, we need a bit more explanation here. How about rewording it
> as "Record dependencies on FDW-related objects. If an outer subplan
> exists, the dependencies would be recorded while processing the
> foreign plans for the base foreign relations in the subplan. Hence
> skip that here."

Agreed.  I updated the comments as proposed.

> + * Currently, we track exactly the dependencies of plans on relations,
> + * user-defined functions and FDW-related objects.  On relcache invalidation
> + * events or pg_proc syscache invalidation events, we invalidate just those
> + * plans that depend on the particular object being modified.  (Note: this
> + * scheme assumes that any table modification that requires replanning will
> + * generate a relcache inval event.)  We also watch for inval events on
> + * certain other system catalogs, such as pg_namespace; but for them, our
> + * response is just to invalidate all plans.  We expect updates on those
> + * catalogs to be infrequent enough that more-detailed tracking is not worth
> + * the effort.
>
> Just like you have added FDW-related objects in the first sentence,
> reference to those needs to be added in the second sentence as well.
> Or you want to start the second sentence as "When the relevant caches
> are invalidated, we invalidate ..." so that those two sentences will
> remain in sync even after additions/deletions to the first sentence.

Good catch!  I like the second one.  How about starting the second
sentence as "On relcache invalidation events or the relevant syscache
invalidation events, we invalidate ..."?

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

Attached is the next version of the patch.

Thanks for the comments!

Best regards,
Etsuro Fujita

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

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

Thanks,
Amit

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

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

On 2016/10/07 10:26, Amit Langote wrote:
> 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:

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

Right.

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

Right.

> I think this (v4) patch is in the best shape so far.

Thanks for the review!

Best regards,
Etsuro Fujita

On Fri, Oct 7, 2016 at 7:20 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/10/07 10:26, Amit Langote wrote:
>>
>> 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:
>
>
>>> 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.
>
>
> Right.
>
>>>>> 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.
>
>
> Right.
>
>> 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".

The patch compiles cleanly, and make check in regress and that in
postgres_fdw shows no failures.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

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.

> The patch compiles cleanly, and make check in regress and that in
> postgres_fdw shows no failures.

Thanks for the review and the updated patch!

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.

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

Best regards,
Etsuro Fujita

On Tue, Oct 18, 2016 at 6:18 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.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.

>
>> The patch compiles cleanly, and make check in regress and that in
>> postgres_fdw shows no failures.
>
>
> Thanks for the review and the updated patch!
>
> 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.

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

If we do as you suggest, we will have to add separate code for
tracking plan dependencies for SELECT queries. 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.

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

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/10/18 22:04, Ashutosh Bapat wrote:
> On Tue, Oct 18, 2016 at 6:18 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.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

On 2016/10/19 12:20, Etsuro Fujita wrote:
> Having said that, I like the latest version (v6), so I'd vote for marking
> this as Ready For Committer.

+1, thanks a lot!

Regards,
Amit

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

A query dependency also implies plan dependency since changing query
implies plan changes. So, if query depends upon something, so does the
plan.

>
> Having said that, I like the latest version (v6), so I'd vote for marking
> this as Ready For Committer.
>

Marked that way.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/10/20 16:27, Ashutosh Bapat wrote:

I wrote:
>>>> 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.

You wrote:
>>> 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 wrote:
>> I mean plan dependencies here, not query dependencies.

> A query dependency also implies plan dependency since changing query
> implies plan changes. So, if query depends upon something, so does the
> plan.

Right.

Sorry, my explanation was not enough, but what I just wanted to mention 
is: for unreferenced relations in the plan (eg, foreign tables excluded 
from the plan by constraint exclusion), we don't need to record the 
dependencies of those relations on FDW-related objects in the plan 
dependency list (ie, glob->invalItems), because the changes to 
attributes of the FDW-related objects for those relations wouldn't 
affect the plan.

I wrote:
>> Having said that, I like the latest version (v6), so I'd vote for marking
>> this as Ready For Committer.

> Marked that way.

Thanks!

Best regards,
Etsuro Fujita

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> [ foreign_plan_cache_inval_v6.patch ]

I looked through this a bit.  A point that doesn't seem to have been
discussed anywhere in the thread is why foreign tables should deserve
exact tracking of dependencies, when we have not bothered with that
for most other object types.  Schemas, for example, we're happy to just
zap the whole plan cache for.  Attempting to do exact tracking is
somewhat expensive and bug-prone --- the length of this thread speaks
to the development cost and bug hazard.  Meanwhile, nobody has questioned
the cost of attaching more PlanInvalItems to a plan (and the cost of the
extra catalog lookups this patch does to create them).  For most plans,
that's nothing but overhead because no invalidation will actually occur
in the life of the plan.

I think there's a very good argument that we should just treat any inval
in pg_foreign_data_wrapper as a reason to blow away the whole plan cache.
People aren't going to alter FDW-level options often enough to make it
worth tracking things more finely.  Personally I'd put pg_foreign_server
changes in the same boat, though maybe those are changed slightly more
often.  If it's worth doing anything more than that, it would be to note
which plans mention foreign tables at all, and not invalidate those that
don't.  We could do that with, say, a hasForeignTables bool in
PlannedStmt.

That leaves the question of whether it's worth detecting table-level
option changes this way, or whether we should just handle those by forcing
a relcache inval in ATExecGenericOptions, as in Amit's original patch in
<5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
patch was short and sweet, and it put the cost on the unusual path (ALTER
TABLE) not the common path (every time we create a query plan).

That leaves not much of this patch :-( though maybe we could salvage some
of the test cases.
        regards, tom lane

On 2016/11/10 5:17, Tom Lane wrote:
> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>> [ foreign_plan_cache_inval_v6.patch ]

> I looked through this a bit.

Thank you for working on this!

> A point that doesn't seem to have been
> discussed anywhere in the thread is why foreign tables should deserve
> exact tracking of dependencies, when we have not bothered with that
> for most other object types.  Schemas, for example, we're happy to just
> zap the whole plan cache for.  Attempting to do exact tracking is
> somewhat expensive and bug-prone --- the length of this thread speaks
> to the development cost and bug hazard.  Meanwhile, nobody has questioned
> the cost of attaching more PlanInvalItems to a plan (and the cost of the
> extra catalog lookups this patch does to create them).  For most plans,
> that's nothing but overhead because no invalidation will actually occur
> in the life of the plan.
>
> I think there's a very good argument that we should just treat any inval
> in pg_foreign_data_wrapper as a reason to blow away the whole plan cache.
> People aren't going to alter FDW-level options often enough to make it
> worth tracking things more finely.  Personally I'd put pg_foreign_server
> changes in the same boat, though maybe those are changed slightly more
> often.  If it's worth doing anything more than that, it would be to note
> which plans mention foreign tables at all, and not invalidate those that
> don't.  We could do that with, say, a hasForeignTables bool in
> PlannedStmt.

I agree on that point.

> That leaves the question of whether it's worth detecting table-level
> option changes this way, or whether we should just handle those by forcing
> a relcache inval in ATExecGenericOptions, as in Amit's original patch in
> <5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
> patch was short and sweet, and it put the cost on the unusual path (ALTER
> TABLE) not the common path (every time we create a query plan).

I think it'd be better to detect table-level option changes because that 
could allow us to do more; we could avoid invalidating cached plans that 
need not to be invalidated, by tracking the plan dependencies more 
exactly, ie, avoid collecting the dependencies of foreign tables in a 
plan that are in dead subqueries or excluded by constraint exclusion. 
The proposed patch doesn't do that, though.  I think updates on 
pg_foreign_table would be more frequent, so it might be worth 
complicating the code.  But the relcache-inval approach couldn't do 
that, IIUC.

Best regards,
Etsuro Fujita

>
> That leaves the question of whether it's worth detecting table-level
> option changes this way, or whether we should just handle those by forcing
> a relcache inval in ATExecGenericOptions, as in Amit's original patch in
> <5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
> patch was short and sweet, and it put the cost on the unusual path (ALTER
> TABLE) not the common path (every time we create a query plan).
>

You seemed not sure about this solution per [1]. Do you think we
should add similar cache invalidation when foreign server and FDW
options are modified?

> That leaves not much of this patch :-( though maybe we could salvage some
> of the test cases.
>

If that's the best way, we can add testcases to that patch.

[1] https://www.postgresql.org/message-id/29681.1459779873@sss.pgh.pa.us

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

[ apologies for not responding sooner ]

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> On 2016/11/10 5:17, Tom Lane wrote:
>> I think there's a very good argument that we should just treat any inval
>> in pg_foreign_data_wrapper as a reason to blow away the whole plan cache.
>> People aren't going to alter FDW-level options often enough to make it
>> worth tracking things more finely.  Personally I'd put pg_foreign_server
>> changes in the same boat, though maybe those are changed slightly more
>> often.  If it's worth doing anything more than that, it would be to note
>> which plans mention foreign tables at all, and not invalidate those that
>> don't.  We could do that with, say, a hasForeignTables bool in
>> PlannedStmt.

> I agree on that point.

OK, please update the patch to handle those catalogs that way.

>> That leaves the question of whether it's worth detecting table-level
>> option changes this way, or whether we should just handle those by forcing
>> a relcache inval in ATExecGenericOptions, as in Amit's original patch in
>> <5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
>> patch was short and sweet, and it put the cost on the unusual path (ALTER
>> TABLE) not the common path (every time we create a query plan).

> I think it'd be better to detect table-level option changes because that 
> could allow us to do more; we could avoid invalidating cached plans that 
> need not to be invalidated, by tracking the plan dependencies more 
> exactly, ie, avoid collecting the dependencies of foreign tables in a 
> plan that are in dead subqueries or excluded by constraint exclusion. 
> The proposed patch doesn't do that, though.  I think updates on 
> pg_foreign_table would be more frequent, so it might be worth 
> complicating the code.  But the relcache-inval approach couldn't do 
> that, IIUC.

Why not?  But the bigger picture here is that relcache inval is the
established practice for forcing replanning due to table-level changes,
and I have very little sympathy for inventing a new mechanism for that
just for foreign tables.  If it were worth bypassing replanning for
changes in tables in dead subqueries, for instance, it would surely be
worth making that happen for all table types not just foreign tables.
        regards, tom lane

On 2016/11/22 4:49, Tom Lane wrote:
> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>> On 2016/11/10 5:17, Tom Lane wrote:
>>> I think there's a very good argument that we should just treat any inval
>>> in pg_foreign_data_wrapper as a reason to blow away the whole plan cache.
>>> People aren't going to alter FDW-level options often enough to make it
>>> worth tracking things more finely.  Personally I'd put pg_foreign_server
>>> changes in the same boat, though maybe those are changed slightly more
>>> often.  If it's worth doing anything more than that, it would be to note
>>> which plans mention foreign tables at all, and not invalidate those that
>>> don't.  We could do that with, say, a hasForeignTables bool in
>>> PlannedStmt.

>> I agree on that point.

> OK, please update the patch to handle those catalogs that way.

Will do.

>>> That leaves the question of whether it's worth detecting table-level
>>> option changes this way, or whether we should just handle those by forcing
>>> a relcache inval in ATExecGenericOptions, as in Amit's original patch in
>>> <5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
>>> patch was short and sweet, and it put the cost on the unusual path (ALTER
>>> TABLE) not the common path (every time we create a query plan).

>> I think it'd be better to detect table-level option changes because that
>> could allow us to do more; we could avoid invalidating cached plans that
>> need not to be invalidated, by tracking the plan dependencies more
>> exactly, ie, avoid collecting the dependencies of foreign tables in a
>> plan that are in dead subqueries or excluded by constraint exclusion.
>> The proposed patch doesn't do that, though.  I think updates on
>> pg_foreign_table would be more frequent, so it might be worth
>> complicating the code.  But the relcache-inval approach couldn't do
>> that, IIUC.

> Why not?  But the bigger picture here is that relcache inval is the
> established practice for forcing replanning due to table-level changes,
> and I have very little sympathy for inventing a new mechanism for that
> just for foreign tables.  If it were worth bypassing replanning for
> changes in tables in dead subqueries, for instance, it would surely be
> worth making that happen for all table types not just foreign tables.

My point here is that FDW-option changes wouldn't affect replanning by 
core; even if forcing a replan, we would have the same foreign tables 
excluded by constraint exclusion, for example.  So, considering updates 
on pg_foreign_table to be rather frequent, it might be better to avoid 
replanning for the pg_foreign_table changes to foreign tables excluded 
by constraint exclusion, for example.  My concern about the 
relcache-inval approach is: that approach wouldn't allow us to do that, 
IIUC.

Best regards,
Etsuro Fujita

On 2016/11/22 15:24, Etsuro Fujita wrote:
> On 2016/11/22 4:49, Tom Lane wrote:
>> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>>> On 2016/11/10 5:17, Tom Lane wrote:
>>>> I think there's a very good argument that we should just treat any
>>>> inval
>>>> in pg_foreign_data_wrapper as a reason to blow away the whole plan
>>>> cache.
>>>> People aren't going to alter FDW-level options often enough to make it
>>>> worth tracking things more finely.  Personally I'd put
>>>> pg_foreign_server
>>>> changes in the same boat, though maybe those are changed slightly more
>>>> often.  If it's worth doing anything more than that, it would be to
>>>> note
>>>> which plans mention foreign tables at all, and not invalidate those
>>>> that
>>>> don't.  We could do that with, say, a hasForeignTables bool in
>>>> PlannedStmt.

>>> I agree on that point.

>> OK, please update the patch to handle those catalogs that way.

> Will do.

Done.  I modified the patch so that any inval in pg_foreign_server also
blows the whole plan cache.

>>>> That leaves the question of whether it's worth detecting table-level
>>>> option changes this way, or whether we should just handle those by
>>>> forcing
>>>> a relcache inval in ATExecGenericOptions, as in Amit's original
>>>> patch in
>>>> <5702298D.4090903@lab.ntt.co.jp>.  I kind of like that approach; that
>>>> patch was short and sweet, and it put the cost on the unusual path
>>>> (ALTER
>>>> TABLE) not the common path (every time we create a query plan).

>>> I think it'd be better to detect table-level option changes because that
>>> could allow us to do more;

>> Why not?  But the bigger picture here is that relcache inval is the
>> established practice for forcing replanning due to table-level changes,
>> and I have very little sympathy for inventing a new mechanism for that
>> just for foreign tables.  If it were worth bypassing replanning for
>> changes in tables in dead subqueries, for instance, it would surely be
>> worth making that happen for all table types not just foreign tables.

> My point here is that FDW-option changes wouldn't affect replanning by
> core; even if forcing a replan, we would have the same foreign tables
> excluded by constraint exclusion, for example.  So, considering updates
> on pg_foreign_table to be rather frequent, it might be better to avoid
> replanning for the pg_foreign_table changes to foreign tables excluded
> by constraint exclusion, for example.  My concern about the
> relcache-inval approach is: that approach wouldn't allow us to do that,
> IIUC.

I thought updates on pg_foreign_table would be rather frequent, but we
don't prove that that is enough that more-detailed tracking is worth the
effort.  Yes, as you mentioned, it wouldn't be a good idea to invent the
new mechanism just for foreign tables.  So, +1 for relcache inval.
Attached is an updated version of the patch, in which I also modified
regression tests; re-created tests to check to see if execution as well
as explain work properly and added some tests for altering server-level
options.

Sorry for the delay.

Best regards,
Etsuro Fujita

Fujita-san,

On 2016/11/30 17:25, Etsuro Fujita wrote:
> On 2016/11/22 15:24, Etsuro Fujita wrote:
>> On 2016/11/22 4:49, Tom Lane wrote:
> 
>>> OK, please update the patch to handle those catalogs that way.
> 
>> Will do.
> 
> Done.  I modified the patch so that any inval in pg_foreign_server also
> blows the whole plan cache.

Thanks for updating the patch and sorry that I didn't myself.

I noticed the following addition:

+     CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID,
PlanCacheSysCallback, (Datum) 0);

Is that intentional?  I thought you meant only to add for pg_foreign_server.

Thanks,
Amit

On 2016/11/30 17:53, Amit Langote wrote:
> On 2016/11/30 17:25, Etsuro Fujita wrote:
>> Done.  I modified the patch so that any inval in pg_foreign_server also
>> blows the whole plan cache.

> I noticed the following addition:
>
> +     CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID,
> PlanCacheSysCallback, (Datum) 0);
>
> Is that intentional?  I thought you meant only to add for pg_foreign_server.

Yes, that's intentional; we would need that as well, because cached 
plans might reference FDW-level options, not only server/table-level 
options.  I couldn't come up with regression tests for FDW-level options 
in postgres_fdw, though.

Best regards,
Etsuro Fujita

On Wed, Nov 30, 2016 at 2:35 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/11/30 17:53, Amit Langote wrote:
>>
>> On 2016/11/30 17:25, Etsuro Fujita wrote:
>>>
>>> Done.  I modified the patch so that any inval in pg_foreign_server also
>>> blows the whole plan cache.
>
>
>> I noticed the following addition:
>>
>> +       CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID,
>> PlanCacheSysCallback, (Datum) 0);
>>
>> Is that intentional?  I thought you meant only to add for
>> pg_foreign_server.
>
>
> Yes, that's intentional; we would need that as well, because cached plans
> might reference FDW-level options, not only server/table-level options.  I
> couldn't come up with regression tests for FDW-level options in
> postgres_fdw, though.


The patch looks good to me, but I feel there are too many testscases.
Now that we have changed the approach to invalidate caches in all
cases, should we just include cases for SELECT or UPDATE or INSERT or
DELETE instead of each statement?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2017/01/03 15:57, Ashutosh Bapat wrote:
> The patch looks good to me, but I feel there are too many testscases.
> Now that we have changed the approach to invalidate caches in all
> cases, should we just include cases for SELECT or UPDATE or INSERT or
> DELETE instead of each statement?

I don't object to that, but (1) the tests I added wouldn't be that 
time-consuming, and (2) they would be more expected to help find bugs, 
in general, so I'd vote for keeping them.  How about leaving that for 
the committer's judge?

Thanks for the review!

Best regards,
Etsuro Fujita

On Thu, Jan 5, 2017 at 2:49 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2017/01/03 15:57, Ashutosh Bapat wrote:
>>
>> The patch looks good to me, but I feel there are too many testscases.
>> Now that we have changed the approach to invalidate caches in all
>> cases, should we just include cases for SELECT or UPDATE or INSERT or
>> DELETE instead of each statement?
>
>
> I don't object to that, but (1) the tests I added wouldn't be that
> time-consuming, and (2) they would be more expected to help find bugs, in
> general, so I'd vote for keeping them.  How about leaving that for the
> committer's judge?
>

Ok. Marking this as ready for committer.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2017/01/06 21:25, Ashutosh Bapat wrote:
> On Thu, Jan 5, 2017 at 2:49 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2017/01/03 15:57, Ashutosh Bapat wrote:
>>> The patch looks good to me, but I feel there are too many testscases.
>>> Now that we have changed the approach to invalidate caches in all
>>> cases, should we just include cases for SELECT or UPDATE or INSERT or
>>> DELETE instead of each statement?

>> I don't object to that, but (1) the tests I added wouldn't be that
>> time-consuming, and (2) they would be more expected to help find bugs, in
>> general, so I'd vote for keeping them.  How about leaving that for the
>> committer's judge?

> Ok. Marking this as ready for committer.

Thanks!

Best regards,
Etsuro Fujita

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> On 2017/01/06 21:25, Ashutosh Bapat wrote:
>> On Thu, Jan 5, 2017 at 2:49 PM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp> wrote:
>>> On 2017/01/03 15:57, Ashutosh Bapat wrote:
>>>> The patch looks good to me, but I feel there are too many testscases.

>>> I don't object to that, but (1) the tests I added wouldn't be that
>>> time-consuming, and (2) they would be more expected to help find bugs, in
>>> general, so I'd vote for keeping them.  How about leaving that for the
>>> committer's judge?

>> Ok. Marking this as ready for committer.

> Thanks!

Pushed.  I ended up simplifying the tests some, partly because I agreed it
seemed like overkill, but mostly because they weren't testing the bug.
The prepared statements that had parameters would have been replanned
anyway, because plancache.c wouldn't have generated enough plans to decide
if a generic plan would be ok.
        regards, tom lane