postgres_fdw : altering foreign table not invalidating prepare statement execution plan.
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.
CREATE EXTENSION postgres_fdw;
CREATE SCHEMA s1;
create table s1.lt (c1 integer, c2 varchar);
insert into s1.lt values (1, 's1.lt');
CREATE SCHEMA s2;
create table s2.lt (c1 integer, c2 varchar);
insert into s2.lt values (1, 's2.lt');
CREATE SERVER link_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
'postgres', port '5447', use_remote_estimate 'true');
CREATE USER MAPPING FOR public SERVER link_server;
create foreign table ft (c1 integer, c2 varchar) server link_server options
(schema_name 's1',table_name 'lt');
ANALYZE ft;
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)
EXECUTE stmt_ft;
c1 | c2
----+-------
1 | s1.lt
(1 row)
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
Show quoted text
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
Attachments:
cache-inval-ft-options-1.patchtext/x-diff; name=cache-inval-ft-options-1.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9e9082d..6a4e1d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11642,6 +11642,13 @@ ATExecGenericOptions(Relation rel, List *options)
simple_heap_update(ftrel, &tuple->t_self, tuple);
CatalogUpdateIndexes(ftrel, tuple);
+ /*
+ * Invalidate the relcache for the table, so that after this commit
+ * all sessions will refresh any cached plans that are based on older
+ * values of the options.
+ */
+ CacheInvalidateRelcache(rel);
+
InvokeObjectPostAlterHook(ForeignTableRelationId,
RelationGetRelid(rel), 0);
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
fdw-plancache-inval-1.patchtext/x-diff; name=fdw-plancache-inval-1.patchDownload
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index dd2b9ed..e7ddc14 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -16,7 +16,9 @@
#include "postgres.h"
#include "access/transam.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_type.h"
+#include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
@@ -150,6 +152,12 @@ static List *set_returning_clause_references(PlannerInfo *root,
static bool fix_opfuncids_walker(Node *node, void *context);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
+static void record_plan_foreign_table_dependency(PlannerInfo *root,
+ Oid ftid);
+static void record_plan_foreign_data_wrapper_dependency(PlannerInfo *root,
+ Oid fdwid);
+static void record_plan_foreign_server_dependency(PlannerInfo *root,
+ Oid serverid);
/*****************************************************************************
*
@@ -2652,6 +2660,54 @@ record_plan_function_dependency(PlannerInfo *root, Oid funcid)
}
/*
+ * record_plan_foreign_table_dependency
+ * Mark the current plan as depending on a particular foreign table.
+ */
+static void
+record_plan_foreign_table_dependency(PlannerInfo *root, Oid ftid)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = FOREIGNTABLEREL;
+ inval_item->hashValue = GetSysCacheHashValue1(FOREIGNTABLEREL,
+ ObjectIdGetDatum(ftid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+}
+
+/*
+ * record_plan_foreign_data_wrapper_dependency
+ * Mark the current plan as depending on a particular FDW.
+ */
+static void
+record_plan_foreign_data_wrapper_dependency(PlannerInfo *root, Oid fdwid)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = FOREIGNDATAWRAPPEROID;
+ inval_item->hashValue = GetSysCacheHashValue1(FOREIGNDATAWRAPPEROID,
+ ObjectIdGetDatum(fdwid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+}
+
+/*
+ * record_plan_foreign_server_dependency
+ * Mark the current plan as depending on a particular FDW.
+ */
+static void
+record_plan_foreign_server_dependency(PlannerInfo *root, Oid serverid)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = FOREIGNSERVEROID;
+ inval_item->hashValue = GetSysCacheHashValue1(FOREIGNSERVEROID,
+ ObjectIdGetDatum(serverid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+}
+
+/*
* extract_query_dependencies
* Given a not-yet-planned query or queries (i.e. a Query node or list
* of Query nodes), extract dependencies just as set_plan_references
@@ -2723,6 +2779,22 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context)
if (rte->rtekind == RTE_RELATION)
context->glob->relationOids =
lappend_oid(context->glob->relationOids, rte->relid);
+
+ if (rte->relkind == RELKIND_FOREIGN_TABLE)
+ {
+ ForeignTable *ftable;
+ ForeignServer *fserver;
+
+ ftable = GetForeignTable(rte->relid);
+ fserver = GetForeignServer(ftable->serverid);
+
+ record_plan_foreign_table_dependency(context,
+ ftable->relid);
+ record_plan_foreign_server_dependency(context,
+ ftable->serverid);
+ record_plan_foreign_data_wrapper_dependency(context,
+ fserver->fdwid);
+ }
}
/* And recurse into the query's subexpressions */
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 8fd9f2b..a1af0c1 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -124,6 +124,10 @@ InitPlanCache(void)
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
/* User mapping change may invalidate plans with pushed down foreign join */
CacheRegisterSyscacheCallback(USERMAPPINGOID, PlanCacheUserMappingCallback, (Datum) 0);
+ /* Foreign table/data wrapper/server alterations invalidate any related plans */
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheFuncCallback, (Datum) 0);
}
/*
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
Attachments:
fdw_disconn_on_alter_objs.patchtext/x-patch; charset=us-asciiDownload
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 subxacts aborted 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_server throws 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 equivalence class 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 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.
By the way, AlterUserMapping seems missing calling
InvokeObjectPostAlterHook. Is this a bug?
Probably, yes.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
foreign_plan_cache_inval.patchtext/x-patch; charset=US-ASCII; name=foreign_plan_cache_inval.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d97e694..7ca1102 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2480,26 +2480,114 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
Filter: (t1.c8 = $1)
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = $1::integer))
(4 rows)
EXECUTE st5('foo', 1);
c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
----+----+-------+------------------------------+--------------------------+----+------------+-----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+-- changing foreign table option should change the plan
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS UPDATE ft4 SET c1 = c1;
+PREPARE st8 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+(5 rows)
+
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
QUERY PLAN
-------------------------------------------------------------------------------
Limit
Output: c1, c2, c3, c4, c5, c6, c7, c8
-> Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
Filter: (t1.tableoid = '1259'::oid)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 4f68e89..9788606 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -570,27 +570,46 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st4(1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st4(1);
-- value of $1 should not be sent to remote
PREPARE st5(user_enum,int) AS SELECT * FROM ft1 t1 WHERE c8 = $1 and c1 = $2;
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+-- changing foreign table option should change the plan
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS UPDATE ft4 SET c1 = c1;
+PREPARE st8 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'ft1'::regclass LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 47158f6..2d53435 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -16,39 +16,41 @@
*/
#include "postgres.h"
#include <limits.h>
#include <math.h>
#include "access/stratnum.h"
#include "access/sysattr.h"
#include "catalog/pg_class.h"
#include "foreign/fdwapi.h"
+#include "foreign/foreign.h"
#include "miscadmin.h"
#include "nodes/extensible.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/clauses.h"
#include "optimizer/cost.h"
#include "optimizer/paths.h"
#include "optimizer/placeholder.h"
#include "optimizer/plancat.h"
#include "optimizer/planmain.h"
#include "optimizer/planner.h"
#include "optimizer/predtest.h"
#include "optimizer/restrictinfo.h"
#include "optimizer/subselect.h"
#include "optimizer/tlist.h"
#include "optimizer/var.h"
#include "parser/parse_clause.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
+#include "utils/syscache.h"
/*
* Flag bits that can appear in the flags argument of create_plan_recurse().
* These can be OR-ed together.
*
* CP_EXACT_TLIST specifies that the generated plan node must return exactly
* the tlist specified by the path's pathtarget (this overrides both
* CP_SMALL_TLIST and CP_LABEL_TLIST, if those are set). Otherwise, the
* plan node is allowed to return just the Vars and PlaceHolderVars needed
@@ -263,20 +265,24 @@ static SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan *lefttree,
List *distinctList, AttrNumber flagColIdx, int firstFlag,
long numGroups);
static LockRows *make_lockrows(Plan *lefttree, List *rowMarks, int epqParam);
static Result *make_result(List *tlist, Node *resconstantqual, Plan *subplan);
static ModifyTable *make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
Index nominalRelation,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, OnConflictExpr *onconflict, int epqParam);
+static void record_plan_foreign_object_dependency(PlannerInfo *root, Oid oid,
+ int cacheId);
+static void record_foreignscan_plan_dependencies(PlannerInfo *root,
+ ForeignScan *scan);
/*
* create_plan
* Creates the access plan for a query by recursively processing the
* desired tree of pathnodes, starting at the node 'best_path'. For
* every pathnode found, we create a corresponding plan node containing
* appropriate id, target list, and qualification information.
*
* The tlists and quals in the plan tree are still in planner format,
@@ -3310,20 +3316,22 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan->fsSystemCol = true;
break;
}
}
bms_free(attrs_used);
}
+ record_foreignscan_plan_dependencies(root, scan_plan);
+
return scan_plan;
}
/*
* create_custom_plan
*
* Transform a CustomPath into a Plan.
*/
static CustomScan *
create_customscan_plan(PlannerInfo *root, CustomPath *best_path,
@@ -6211,10 +6219,66 @@ is_projection_capable_plan(Plan *plan)
case T_ModifyTable:
case T_Append:
case T_MergeAppend:
case T_RecursiveUnion:
return false;
default:
break;
}
return true;
}
+
+/*
+ * record_foreignscan_plan_dependencies
+ * Mark a plan as dependent upon the properties of foreign server, FDW and
+ * foreign table/s required by given ForeignScan.
+ *
+ * Changes to the foreign server, foreign table/s or FDW might render the plan
+ * invalid. It's not necessary that every change (e.g. changing value of an
+ * option) renders the plan invalid. Only FDW knows which changes render a plan
+ * invalid and may fail to convey the same to the core. Doing it here seems a
+ * mid-way solution.
+ */
+static void
+record_foreignscan_plan_dependencies(PlannerInfo *root, ForeignScan *scan)
+{
+ ForeignServer *server = GetForeignServer(scan->fs_server);
+ int relid;
+
+ /* Record dependency on the foreign server. */
+ record_plan_foreign_object_dependency(root, scan->fs_server,
+ FOREIGNSERVEROID);
+
+ /* Record dependency on the foreign data wrapper. */
+ record_plan_foreign_object_dependency(root, server->fdwid,
+ FOREIGNDATAWRAPPEROID);
+
+ /* Record dependency on each of the foreign tables. */
+ relid = -1;
+ while ((relid = bms_next_member(scan->fs_relids, relid)) >= 0)
+ {
+ RangeTblEntry *rte = planner_rt_fetch(relid, root);
+
+ Assert(rte->rtekind == RTE_RELATION &&
+ rte->relkind == RELKIND_FOREIGN_TABLE);
+ record_plan_foreign_object_dependency(root, rte->relid,
+ FOREIGNTABLEREL);
+ }
+
+}
+
+/*
+ * record_plan_foreign_object_dependency
+ * Records the plan's dependency on given foreign object. A helper
+ * function for record_foreignscan_plan_dependencies().
+ */
+static void
+record_plan_foreign_object_dependency(PlannerInfo *root, Oid oid, int cacheId)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheId;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheId,
+ ObjectIdGetDatum(oid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index c96a865..1d3b6be 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -112,20 +112,24 @@ static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
* All we need to do is hook into inval.c's callback lists.
*/
void
InitPlanCache(void)
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
+ /* Foreign table/data wrapper/server alterations invalidate any related plans */
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheFuncCallback, (Datum) 0);
}
/*
* CreateCachedPlan: initially create a plan cache entry.
*
* Creation of a cached plan is divided into two steps, CreateCachedPlan and
* CompleteCachedPlan. CreateCachedPlan should be called after running the
* query through raw_parser, but before doing parse analysis and rewrite;
* CompleteCachedPlan is called after that. The reason for this arrangement
* is that it can save one round of copying of the raw parse tree, since
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016/09/30 1:09, Ashutosh Bapat wrote:
You wrote:
The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.
I wrote:
Hmm. I'm not sure that that's a good idea.
I was thinking the changes to setrefs.c proposed by Amit to collect that
dependencies would be probably OK, but I wasn't sure that it's a good idea
that he used PlanCacheFuncCallback as the syscache inval callback function
for the foreign object caches because it invalidates not only generic plans
but query trees, as you mentioned downthread. So, I was thinking to modify
his patch so that we add a new syscache inval callback function for the
caches that is much like PlanCacheFuncCallback but only invalidates generic
plans.
PlanCacheFuncCallback() invalidates the query tree only when
invalItems are added to the plan source. The patch adds the
dependencies in root->glob->invalItems, which standard_planner()
copies into PlannedStmt::invalItems. This is then copied into the
gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
query tree. I have verified this under the debugger. Am I missing
something?
I think you are right. Maybe my explanation was not enough, sorry for
that. I just wanted to mention that Amit's patch would invalidate the
generic plan as well as the rewritten query tree.
I looked at your patch closely. You added code to track dependencies on
FDW-related objects to createplan.c, but I think it would be more
appropriate to put that code in setrefs.c like the attached. I think
record_foreignscan_plan_dependencies in your patch would be a bit
inefficient because that tracks such dependencies redundantly in the
case where the given ForeignScan has an outer subplan, so I optimized
that in the attached. (Also some regression tests for that were added.)
I think it would be a bit inefficient to use PlanCacheFuncCallback as
the inval callback function for those caches, because that checks the
inval-item list for the rewritten query tree, but any updates on such
catalogs wouldn't affect the query tree. So, I added a new callback
function for those caches that is much like PlanCacheFuncCallback but
skips checking the list for the query tree. I updated some comments also.
Best regards,
Etsuro Fujita
Attachments:
foreign_plan_cache_inval_v2.patchbinary/octet-stream; name=foreign_plan_cache_inval_v2.patchDownload
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 2487,2498 **** EXECUTE st5('foo', 1);
--- 2487,2685 ----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1);
+ PREPARE st8 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) FOR UPDATE OF t1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st10 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 4" r1 LEFT JOIN "S 1"."T 3" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 4" r1 LEFT JOIN "S 1"."T 3" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+ (5 rows)
+
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 577,582 **** EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
--- 577,608 ----
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1);
+ PREPARE st8 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) FOR UPDATE OF t1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st10 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
-- cleanup
DEALLOCATE st1;
***************
*** 584,589 **** DEALLOCATE st2;
--- 610,620 ----
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 17,28 ****
--- 17,30 ----
#include "access/transam.h"
#include "catalog/pg_type.h"
+ #include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
#include "optimizer/planmain.h"
#include "optimizer/planner.h"
#include "optimizer/tlist.h"
+ #include "parser/parsetree.h"
#include "tcop/utility.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
***************
*** 137,142 **** static List *set_returning_clause_references(PlannerInfo *root,
--- 139,146 ----
Plan *topplan,
Index resultRelation,
int rtoffset);
+ static void record_plan_foreign_dependencies(PlannerInfo *root, ForeignScan *scan);
+ static void add_inval_item(PlannerInfo *root, int cacheid, Oid oid);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
***************
*** 174,181 **** static bool extract_query_dependencies_walker(Node *node,
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations and user-defined functions are the only types of
! * objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
--- 178,185 ----
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations, user-defined functions and FDW-related objects are
! * the only types of objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
***************
*** 1123,1128 **** set_foreignscan_references(PlannerInfo *root,
--- 1127,1140 ----
ForeignScan *fscan,
int rtoffset)
{
+ /*
+ * Record dependencies on FDW-related objects. If an outer subplan
+ * exists, that would be done in the processing of its baserels, so skip
+ * that.
+ */
+ if (fscan->scan.plan.lefttree == NULL)
+ record_plan_foreign_dependencies(root, fscan);
+
/* Adjust scanrelid if it's valid */
if (fscan->scan.scanrelid > 0)
fscan->scan.scanrelid += rtoffset;
***************
*** 2423,2441 **** record_plan_function_dependency(PlannerInfo *root, Oid funcid)
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
- PlanInvalItem *inval_item = makeNode(PlanInvalItem);
-
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! inval_item->cacheId = PROCOID;
! inval_item->hashValue = GetSysCacheHashValue1(PROCOID,
! ObjectIdGetDatum(funcid));
! root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
}
}
/*
--- 2435,2491 ----
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! add_inval_item(root, PROCOID, funcid);
! }
! }
!
! /*
! * record_plan_foreign_dependencies
! * Mark the current plan as depending on FDW-related objects.
! *
! * This is required since modifications to attributes of FDW-related objects,
! * such as FDW options, might require replanning.
! */
! static void
! record_plan_foreign_dependencies(PlannerInfo *root, ForeignScan *scan)
! {
! int relid = -1;
! ForeignServer *server = GetForeignServer(scan->fs_server);
! /* Record dependencies on the foreign tables */
! while ((relid = bms_next_member(scan->fs_relids, relid)) >= 0)
! {
! RangeTblEntry *rte = planner_rt_fetch(relid, root);
!
! add_inval_item(root, FOREIGNTABLEREL, rte->relid);
}
+
+ /* Likewise for the foreign server */
+ add_inval_item(root, FOREIGNSERVEROID, scan->fs_server);
+
+ /* Likewise for the foreign data wrapper */
+ add_inval_item(root, FOREIGNDATAWRAPPEROID, server->fdwid);
+ }
+
+ /*
+ * add_inval_item
+ * Add given dependency to root->glob->invalItems.
+ */
+ static void
+ add_inval_item(PlannerInfo *root, int cacheid, Oid oid)
+ {
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheid;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheid,
+ ObjectIdGetDatum(oid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
}
/*
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
***************
*** 27,41 ****
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * Currently, we track exactly the dependencies of plans on relations and
! * user-defined functions. 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
--- 27,42 ----
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
***************
*** 103,108 **** static bool ScanQueryWalker(Node *node, bool *acquire);
--- 104,110 ----
static TupleDesc PlanCacheComputeResultDesc(List *stmt_list);
static void PlanCacheRelCallback(Datum arg, Oid relid);
static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
+ static void PlanCacheForeignCallback(Datum arg, int cacheid, uint32 hashvalue);
static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
***************
*** 116,121 **** InitPlanCache(void)
--- 118,126 ----
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheForeignCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheForeignCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheForeignCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
***************
*** 1829,1834 **** PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
--- 1834,1903 ----
}
/*
+ * PlanCacheForeignCallback
+ * Syscache inval callback function for FOREIGNTABLEREL, FOREIGNSERVEROID
+ * and FOREIGNDATAWRAPPEROID caches
+ *
+ * Invalidate all plans mentioning the object with the specified hash value,
+ * or all plans mentioning any member of the given cache if hashvalue == 0.
+ *
+ * Note: any updates on those catalogs don't affect the rewritten querytree,
+ * but might require replanning, so we invalidate the generic plan only.
+ */
+ static void
+ PlanCacheForeignCallback(Datum arg, int cacheid, uint32 hashvalue)
+ {
+ CachedPlanSource *plansource;
+
+ for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
+ {
+ ListCell *lc;
+
+ Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
+
+ /* No work if it's already invalidated */
+ if (!plansource->is_valid)
+ continue;
+
+ /* Never invalidate transaction control commands */
+ if (IsTransactionStmtPlan(plansource))
+ continue;
+
+ /*
+ * Check the dependency list for the generic plan.
+ */
+ if (plansource->gplan && plansource->gplan->is_valid)
+ {
+ foreach(lc, plansource->gplan->stmt_list)
+ {
+ PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc);
+ ListCell *lc3;
+
+ Assert(!IsA(plannedstmt, Query));
+ if (!IsA(plannedstmt, PlannedStmt))
+ continue; /* Ignore utility statements */
+ foreach(lc3, plannedstmt->invalItems)
+ {
+ PlanInvalItem *item = (PlanInvalItem *) lfirst(lc3);
+
+ if (item->cacheId != cacheid)
+ continue;
+ if (hashvalue == 0 ||
+ item->hashValue == hashvalue)
+ {
+ /* Invalidate the generic plan only */
+ plansource->gplan->is_valid = false;
+ break; /* out of invalItems scan */
+ }
+ }
+ if (!plansource->gplan->is_valid)
+ break; /* out of stmt_list scan */
+ }
+ }
+ }
+ }
+
+ /*
* PlanCacheSysCallback
* Syscache inval callback function for other caches
*
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
foreign_plan_cache_inval_v3.patchbinary/octet-stream; name=foreign_plan_cache_inval_v3.patchDownload
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 2487,2498 **** EXECUTE st5('foo', 1);
--- 2487,2685 ----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1);
+ PREPARE st8 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) FOR UPDATE OF t1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st10 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 4" r1 LEFT JOIN "S 1"."T 3" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 4" r1 LEFT JOIN "S 1"."T 3" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+ (5 rows)
+
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, r2.c1, r2.c2, r2.c3 FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1))))
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ -> Foreign Scan
+ Output: t1.c1, t1.c2, t1.c3, t2.c1, t2.c2, t2.c3, t1.*, t2.*
+ Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
+ Remote SQL: SELECT r1.c1, r1.c2, r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1.c1, r1.c2, r1.c3) END, r2.c1, r2.c2, r2.c3, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1)))) FOR UPDATE OF r1
+ -> Hash Left Join
+ Output: t1.c1, t1.c2, t1.c3, t1.*, t2.c1, t2.c2, t2.c3, t2.*
+ Hash Cond: (t1.c1 = t2.c1)
+ -> Foreign Scan on public.ft4 t1
+ Output: t1.c1, t1.c2, t1.c3, t1.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3" FOR UPDATE
+ -> Hash
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ -> Foreign Scan on public.ft5 t2
+ Output: t2.c1, t2.c2, t2.c3, t2.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (17 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 577,582 **** EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
--- 577,608 ----
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1);
+ PREPARE st8 AS SELECT * FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) FOR UPDATE OF t1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st10 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ ALTER FOREIGN TABLE ft5 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
-- cleanup
DEALLOCATE st1;
***************
*** 584,589 **** DEALLOCATE st2;
--- 610,620 ----
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 17,28 ****
--- 17,30 ----
#include "access/transam.h"
#include "catalog/pg_type.h"
+ #include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
#include "optimizer/planmain.h"
#include "optimizer/planner.h"
#include "optimizer/tlist.h"
+ #include "parser/parsetree.h"
#include "tcop/utility.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
***************
*** 137,142 **** static List *set_returning_clause_references(PlannerInfo *root,
--- 139,146 ----
Plan *topplan,
Index resultRelation,
int rtoffset);
+ static void record_plan_foreign_dependencies(PlannerInfo *root, ForeignScan *scan);
+ static void add_inval_item(PlannerInfo *root, int cacheid, Oid oid);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
***************
*** 174,181 **** static bool extract_query_dependencies_walker(Node *node,
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations and user-defined functions are the only types of
! * objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
--- 178,185 ----
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations, user-defined functions and FDW-related objects are
! * the only types of objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
***************
*** 1123,1128 **** set_foreignscan_references(PlannerInfo *root,
--- 1127,1140 ----
ForeignScan *fscan,
int rtoffset)
{
+ /*
+ * Record dependencies on FDW-related objects. If an outer subplan
+ * exists, the dependencies would be recorded while processing the foreign
+ * scans for the base relations in the subplan. Hence skip that here.
+ */
+ if (fscan->scan.plan.lefttree == NULL)
+ record_plan_foreign_dependencies(root, fscan);
+
/* Adjust scanrelid if it's valid */
if (fscan->scan.scanrelid > 0)
fscan->scan.scanrelid += rtoffset;
***************
*** 2423,2441 **** record_plan_function_dependency(PlannerInfo *root, Oid funcid)
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
- PlanInvalItem *inval_item = makeNode(PlanInvalItem);
-
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! inval_item->cacheId = PROCOID;
! inval_item->hashValue = GetSysCacheHashValue1(PROCOID,
! ObjectIdGetDatum(funcid));
! root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
}
}
/*
--- 2435,2491 ----
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! add_inval_item(root, PROCOID, funcid);
! }
! }
!
! /*
! * record_plan_foreign_dependencies
! * Mark the current plan as depending on FDW-related objects.
! *
! * This is required since modifications to attributes of FDW-related objects,
! * such as FDW options, might require replanning.
! */
! static void
! record_plan_foreign_dependencies(PlannerInfo *root, ForeignScan *scan)
! {
! int relid = -1;
! ForeignServer *server = GetForeignServer(scan->fs_server);
! /* Record dependencies on the foreign tables */
! while ((relid = bms_next_member(scan->fs_relids, relid)) >= 0)
! {
! RangeTblEntry *rte = planner_rt_fetch(relid, root);
!
! add_inval_item(root, FOREIGNTABLEREL, rte->relid);
}
+
+ /* Likewise for the foreign server */
+ add_inval_item(root, FOREIGNSERVEROID, scan->fs_server);
+
+ /* Likewise for the foreign data wrapper */
+ add_inval_item(root, FOREIGNDATAWRAPPEROID, server->fdwid);
+ }
+
+ /*
+ * add_inval_item
+ * Add given dependency to root->glob->invalItems.
+ */
+ static void
+ add_inval_item(PlannerInfo *root, int cacheid, Oid oid)
+ {
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheid;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheid,
+ ObjectIdGetDatum(oid));
+
+ root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
}
/*
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
***************
*** 27,41 ****
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * Currently, we track exactly the dependencies of plans on relations and
! * user-defined functions. 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
--- 27,42 ----
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * Currently, we track exactly the dependencies of plans on relations,
! * user-defined functions and FDW-related objects. On relcache invalidation
! * events or the relevant 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
***************
*** 103,108 **** static bool ScanQueryWalker(Node *node, bool *acquire);
--- 104,112 ----
static TupleDesc PlanCacheComputeResultDesc(List *stmt_list);
static void PlanCacheRelCallback(Datum arg, Oid relid);
static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
+ static void PlanCacheForeignCallback(Datum arg, int cacheid, uint32 hashvalue);
+ static void CheckGenericPlanDependencies(CachedPlanSource *plansource,
+ int cacheid, uint32 hashvalue);
static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
***************
*** 116,121 **** InitPlanCache(void)
--- 120,128 ----
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheForeignCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheForeignCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheForeignCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
***************
*** 1797,1828 **** PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
* The generic plan, if any, could have more dependencies than the
* querytree does, so we have to check it too.
*/
! if (plansource->gplan && plansource->gplan->is_valid)
{
! foreach(lc, plansource->gplan->stmt_list)
{
! PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc);
! ListCell *lc3;
! Assert(!IsA(plannedstmt, Query));
! if (!IsA(plannedstmt, PlannedStmt))
! continue; /* Ignore utility statements */
! foreach(lc3, plannedstmt->invalItems)
{
! PlanInvalItem *item = (PlanInvalItem *) lfirst(lc3);
!
! if (item->cacheId != cacheid)
! continue;
! if (hashvalue == 0 ||
! item->hashValue == hashvalue)
! {
! /* Invalidate the generic plan only */
! plansource->gplan->is_valid = false;
! break; /* out of invalItems scan */
! }
}
- if (!plansource->gplan->is_valid)
- break; /* out of stmt_list scan */
}
}
}
--- 1804,1882 ----
* The generic plan, if any, could have more dependencies than the
* querytree does, so we have to check it too.
*/
! CheckGenericPlanDependencies(plansource, cacheid, hashvalue);
! }
! }
!
! /*
! * PlanCacheForeignCallback
! * Syscache inval callback function for FOREIGNTABLEREL, FOREIGNSERVEROID
! * and FOREIGNDATAWRAPPEROID caches
! *
! * Invalidate all plans mentioning the object with the specified hash value,
! * or all plans mentioning any member of the given cache if hashvalue == 0.
! *
! * Note: any updates on those catalogs don't affect the rewritten querytree,
! * but might require replanning, so we invalidate the generic plan only.
! */
! static void
! PlanCacheForeignCallback(Datum arg, int cacheid, uint32 hashvalue)
! {
! CachedPlanSource *plansource;
!
! for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
! {
! Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
!
! /* No work if it's already invalidated */
! if (!plansource->is_valid)
! continue;
!
! /* Never invalidate transaction control commands */
! if (IsTransactionStmtPlan(plansource))
! continue;
!
! /*
! * Check the dependency list for the generic plan.
! */
! CheckGenericPlanDependencies(plansource, cacheid, hashvalue);
! }
! }
!
! /*
! * CheckGenericPlanDependencies: invalidate the generic plan, if it meets the
! * condition described in the comments for PlanCacheFuncCallback and
! * PlanCacheForeignCallback.
! */
! static void
! CheckGenericPlanDependencies(CachedPlanSource *plansource,
! int cacheid, uint32 hashvalue)
! {
! if (plansource->gplan && plansource->gplan->is_valid)
! {
! ListCell *lc;
!
! foreach(lc, plansource->gplan->stmt_list)
{
! PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc);
! ListCell *lc3;
!
! Assert(!IsA(plannedstmt, Query));
! if (!IsA(plannedstmt, PlannedStmt))
! continue; /* Ignore utility statements */
! foreach(lc3, plannedstmt->invalItems)
{
! PlanInvalItem *item = (PlanInvalItem *) lfirst(lc3);
! if (item->cacheId != cacheid)
! continue;
! if (hashvalue == 0 ||
! item->hashValue == hashvalue)
{
! /* Invalidate the generic plan only */
! plansource->gplan->is_valid = false;
! return;
}
}
}
}
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
foreign_plan_cache_inval_v4.patchbinary/octet-stream; name=foreign_plan_cache_inval_v4.patchDownload
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 2487,2498 **** EXECUTE st5('foo', 1);
--- 2487,2615 ----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+ PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 4"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+ (5 rows)
+
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ --------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+ -----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+ (4 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+ ----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+ (3 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+ ---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+ (5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 577,582 **** EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
--- 577,602 ----
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+ -- changing foreign table options should change the plans
+ PREPARE st6 AS SELECT * FROM ft4;
+ PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+ PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+ PREPARE st9 AS UPDATE ft4 SET c1 = random();
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ -- restore the original options and check again
+ ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
-- cleanup
DEALLOCATE st1;
***************
*** 584,589 **** DEALLOCATE st2;
--- 604,613 ----
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
***************
*** 16,22 ****
--- 16,24 ----
#include "postgres.h"
#include "access/transam.h"
+ #include "catalog/pg_class.h"
#include "catalog/pg_type.h"
+ #include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
***************
*** 137,142 **** static List *set_returning_clause_references(PlannerInfo *root,
--- 139,146 ----
Plan *topplan,
Index resultRelation,
int rtoffset);
+ static void record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid);
+ static void add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
***************
*** 174,181 **** static bool extract_query_dependencies_walker(Node *node,
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations and user-defined functions are the only types of
! * objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
--- 178,185 ----
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
! * Currently, relations, user-defined functions and FDW-related objects are
! * the only types of objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
***************
*** 426,432 **** add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
--- 430,442 ----
* but it would probably cost more cycles than it would save.
*/
if (newrte->rtekind == RTE_RELATION)
+ {
glob->relationOids = lappend_oid(glob->relationOids, newrte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (newrte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(glob, newrte->relid);
+ }
}
/*
***************
*** 2423,2444 **** record_plan_function_dependency(PlannerInfo *root, Oid funcid)
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
- PlanInvalItem *inval_item = makeNode(PlanInvalItem);
-
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! inval_item->cacheId = PROCOID;
! inval_item->hashValue = GetSysCacheHashValue1(PROCOID,
! ObjectIdGetDatum(funcid));
!
! root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
}
}
/*
* extract_query_dependencies
* Given a rewritten, but not yet planned, query or queries
* (i.e. a Query node or list of Query nodes), extract dependencies
--- 2433,2487 ----
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
! add_inval_item(root->glob, PROCOID, funcid);
}
}
/*
+ * record_plan_foreign_dependencies
+ * Mark the current plan as depending on FDW-related objects.
+ *
+ * This is required since modifications to attributes of FDW-related objects,
+ * such as FDW options, might require replanning.
+ */
+ static void
+ record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid)
+ {
+ ForeignTable *table = GetForeignTable(relid);
+ ForeignServer *server = GetForeignServer(table->serverid);
+
+ /* Record the dependency on the foreign table */
+ add_inval_item(glob, FOREIGNTABLEREL, relid);
+
+ /* Likewise for the foreign server */
+ add_inval_item(glob, FOREIGNSERVEROID, table->serverid);
+
+ /* Likewise for the foreign data wrapper */
+ add_inval_item(glob, FOREIGNDATAWRAPPEROID, server->fdwid);
+ }
+
+ /*
+ * add_inval_item
+ * Add given dependency to glob->invalItems.
+ */
+ static void
+ add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid)
+ {
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheid;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheid,
+ ObjectIdGetDatum(oid));
+
+ glob->invalItems = lappend(glob->invalItems, inval_item);
+ }
+
+ /*
* extract_query_dependencies
* Given a rewritten, but not yet planned, query or queries
* (i.e. a Query node or list of Query nodes), extract dependencies
***************
*** 2510,2517 **** extract_query_dependencies_walker(Node *node, PlannerInfo *context)
--- 2553,2566 ----
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
if (rte->rtekind == RTE_RELATION)
+ {
context->glob->relationOids =
lappend_oid(context->glob->relationOids, rte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (rte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(context->glob, rte->relid);
+ }
}
/* And recurse into the query's subexpressions */
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
***************
*** 27,41 ****
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * Currently, we track exactly the dependencies of plans on relations and
! * user-defined functions. 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
--- 27,42 ----
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
! * Currently, we track exactly the dependencies of plans on relations,
! * user-defined functions and FDW-related objects. On relcache invalidation
! * events or the relevant 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
***************
*** 116,121 **** InitPlanCache(void)
--- 117,125 ----
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheFuncCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
foreign_plan_cache_inval_v5.patchinvalid/octet-stream; name=foreign_plan_cache_inval_v5.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d97e694..568671e 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2480,26 +2480,143 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
Filter: (t1.c8 = $1)
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = $1::integer))
(4 rows)
EXECUTE st5('foo', 1);
c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
----+----+-------+------------------------------+--------------------------+----+------------+-----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+-- changing foreign table options should change the plans
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+PREPARE st9 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 4"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+(5 rows)
+
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
+DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
QUERY PLAN
-------------------------------------------------------------------------------
Limit
Output: c1, c2, c3, c4, c5, c6, c7, c8
-> Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
Filter: (t1.tableoid = '1259'::oid)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 4f68e89..f7eca2d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -570,27 +570,51 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st4(1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st4(1);
-- value of $1 should not be sent to remote
PREPARE st5(user_enum,int) AS SELECT * FROM ft1 t1 WHERE c8 = $1 and c1 = $2;
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+-- changing foreign table options should change the plans
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+PREPARE st9 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
+DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'ft1'::regclass LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d10a983..2f6b4a0 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -9,21 +9,23 @@
*
*
* IDENTIFICATION
* src/backend/optimizer/plan/setrefs.c
*
*-------------------------------------------------------------------------
*/
#include "postgres.h"
#include "access/transam.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_type.h"
+#include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
#include "optimizer/planmain.h"
#include "optimizer/planner.h"
#include "optimizer/tlist.h"
#include "tcop/utility.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
@@ -130,20 +132,22 @@ static Node *fix_upper_expr(PlannerInfo *root,
indexed_tlist *subplan_itlist,
Index newvarno,
int rtoffset);
static Node *fix_upper_expr_mutator(Node *node,
fix_upper_expr_context *context);
static List *set_returning_clause_references(PlannerInfo *root,
List *rlist,
Plan *topplan,
Index resultRelation,
int rtoffset);
+static void record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid);
+static void add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
/*****************************************************************************
*
* SUBPLAN REFERENCES
*
*****************************************************************************/
/*
@@ -167,22 +171,22 @@ static bool extract_query_dependencies_walker(Node *node,
* 5. PARAM_MULTIEXPR Params are replaced by regular PARAM_EXEC Params,
* now that we have finished planning all MULTIEXPR subplans.
*
* 6. We compute regproc OIDs for operators (ie, we look up the function
* that implements each op).
*
* 7. We create lists of specific objects that the plan depends on.
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
- * Currently, relations and user-defined functions are the only types of
- * objects that are explicitly tracked this way.
+ * Currently, relations, user-defined functions and FDW-related objects are
+ * the only types of objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
* We also perform one final optimization step, which is to delete
* SubqueryScan plan nodes that aren't doing anything useful (ie, have
* no qual and a no-op targetlist). The reason for doing this last is that
* it can't readily be done before set_plan_references, because it would
* break set_upper_references: the Vars in the subquery's top tlist
* wouldn't match up with the Vars in the outer plan tree. The SubqueryScan
* serves a necessary function as a buffer between outer query and subquery
@@ -419,21 +423,27 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
*
* We do this even though the RTE might be unreferenced in the plan tree;
* this would correspond to cases such as views that were expanded, child
* tables that were eliminated by constraint exclusion, etc. Schema
* invalidation on such a rel must still force rebuilding of the plan.
*
* Note we don't bother to avoid making duplicate list entries. We could,
* but it would probably cost more cycles than it would save.
*/
if (newrte->rtekind == RTE_RELATION)
+ {
glob->relationOids = lappend_oid(glob->relationOids, newrte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (newrte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(glob, newrte->relid);
+ }
}
/*
* set_plan_refs: recurse through the Plan nodes of a single subquery level
*/
static Plan *
set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
{
ListCell *l;
@@ -2416,36 +2426,69 @@ record_plan_function_dependency(PlannerInfo *root, Oid funcid)
/*
* For performance reasons, we don't bother to track built-in functions;
* we just assume they'll never change (or at least not in ways that'd
* invalidate plans using them). For this purpose we can consider a
* built-in function to be one with OID less than FirstBootstrapObjectId.
* Note that the OID generator guarantees never to generate such an OID
* after startup, even at OID wraparound.
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
- PlanInvalItem *inval_item = makeNode(PlanInvalItem);
-
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
- inval_item->cacheId = PROCOID;
- inval_item->hashValue = GetSysCacheHashValue1(PROCOID,
- ObjectIdGetDatum(funcid));
-
- root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+ add_inval_item(root->glob, PROCOID, funcid);
}
}
/*
+ * record_plan_foreign_dependencies
+ * Mark the current plan as depending on FDW-related objects.
+ *
+ * This is required since modifications to attributes of FDW-related objects,
+ * such as FDW options, might require replanning.
+ */
+static void
+record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid)
+{
+ ForeignTable *table = GetForeignTable(relid);
+ ForeignServer *server = GetForeignServer(table->serverid);
+
+ /* Record the dependency on the foreign table */
+ add_inval_item(glob, FOREIGNTABLEREL, relid);
+
+ /* Likewise for the foreign server */
+ add_inval_item(glob, FOREIGNSERVEROID, table->serverid);
+
+ /* Likewise for the foreign data wrapper */
+ add_inval_item(glob, FOREIGNDATAWRAPPEROID, server->fdwid);
+}
+
+/*
+ * add_inval_item
+ * Add given dependency to glob->invalItems.
+ */
+static void
+add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheid;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheid,
+ ObjectIdGetDatum(oid));
+
+ glob->invalItems = lappend(glob->invalItems, inval_item);
+}
+
+/*
* extract_query_dependencies
* Given a rewritten, but not yet planned, query or queries
* (i.e. a Query node or list of Query nodes), extract dependencies
* just as set_plan_references would do. Also detect whether any
* rewrite steps were affected by RLS.
*
* This is needed by plancache.c to handle invalidation of cached unplanned
* queries.
*/
void
@@ -2503,21 +2546,27 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context)
/* Remember if any Query has RLS quals applied by rewriter */
if (query->hasRowSecurity)
context->glob->dependsOnRole = true;
/* Collect relation OIDs in this Query's rtable */
foreach(lc, query->rtable)
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
if (rte->rtekind == RTE_RELATION)
+ {
context->glob->relationOids =
lappend_oid(context->glob->relationOids, rte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (rte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(context->glob, rte->relid);
+ }
}
/* And recurse into the query's subexpressions */
return query_tree_walker(query, extract_query_dependencies_walker,
(void *) context, 0);
}
return expression_tree_walker(node, extract_query_dependencies_walker,
(void *) context);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index c96a865..962fd17 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -20,29 +20,29 @@
* or, if RLS is involved, if the user changes or the RLS environment changes.
*
* Note that if the sinval was a result of user DDL actions, parse analysis
* could throw an error, for example if a column referenced by the query is
* no longer present. Another possibility is for the query's output tupdesc
* to change (for instance "SELECT *" might expand differently than before).
* The creator of a cached plan can specify whether it is allowable for the
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
- * Currently, we track exactly the dependencies of plans on relations and
- * user-defined functions. 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.
+ * Currently, we track exactly the dependencies of plans on relations,
+ * user-defined functions and FDW-related objects. On the corresponding
+ * relcache 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
* src/backend/utils/cache/plancache.c
*
*-------------------------------------------------------------------------
*/
@@ -109,20 +109,23 @@ static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
/*
* InitPlanCache: initialize module during InitPostgres.
*
* All we need to do is hook into inval.c's callback lists.
*/
void
InitPlanCache(void)
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheFuncCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
}
/*
* CreateCachedPlan: initially create a plan cache entry.
*
* Creation of a cached plan is divided into two steps, CreateCachedPlan and
* CompleteCachedPlan. CreateCachedPlan should be called after running the
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
Attachments:
foreign_plan_cache_inval_v6.patchbinary/octet-stream; name=foreign_plan_cache_inval_v6.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d97e694..568671e 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2487,12 +2487,129 @@ EXECUTE st5('foo', 1);
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+-- changing foreign table options should change the plans
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+PREPARE st9 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 4"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 4" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 4" FOR UPDATE
+(5 rows)
+
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+--------------------------------------------------
+ Foreign Scan on public.ft4
+ Output: c1, c2, c3
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+ QUERY PLAN
+-----------------------------------------------------------------------
+ Insert on public.ft4
+ Remote SQL: INSERT INTO "S 1"."T 3"(c1, c2, c3) VALUES ($1, $2, $3)
+ -> Result
+ Output: 101, 102, 'foo'::text
+(4 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.ft4
+ -> Foreign Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = c1
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ QUERY PLAN
+---------------------------------------------------------------------
+ Update on public.ft4
+ Remote SQL: UPDATE "S 1"."T 3" SET c1 = $2 WHERE ctid = $1
+ -> Foreign Scan on public.ft4
+ Output: random(), c2, c3, ctid
+ Remote SQL: SELECT c2, c3, ctid FROM "S 1"."T 3" FOR UPDATE
+(5 rows)
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
+DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 4f68e89..f7eca2d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -577,6 +577,26 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+-- changing foreign table options should change the plans
+PREPARE st6 AS SELECT * FROM ft4;
+PREPARE st7 AS INSERT INTO ft4 VALUES (101, 102, 'foo');
+PREPARE st8 AS UPDATE ft4 SET c1 = c1;
+PREPARE st9 AS UPDATE ft4 SET c1 = random();
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 4');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
+-- restore the original options and check again
+ALTER FOREIGN TABLE ft4 OPTIONS (SET table_name 'T 3');
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9;
-- cleanup
DEALLOCATE st1;
@@ -584,6 +604,10 @@ DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+DEALLOCATE st6;
+DEALLOCATE st7;
+DEALLOCATE st8;
+DEALLOCATE st9;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d10a983..2f6b4a0 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -16,7 +16,9 @@
#include "postgres.h"
#include "access/transam.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_type.h"
+#include "foreign/foreign.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "optimizer/pathnode.h"
@@ -137,6 +139,8 @@ static List *set_returning_clause_references(PlannerInfo *root,
Plan *topplan,
Index resultRelation,
int rtoffset);
+static void record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid);
+static void add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid);
static bool extract_query_dependencies_walker(Node *node,
PlannerInfo *context);
@@ -174,8 +178,8 @@ static bool extract_query_dependencies_walker(Node *node,
* This will be used by plancache.c to drive invalidation of cached plans.
* Relation dependencies are represented by OIDs, and everything else by
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
- * Currently, relations and user-defined functions are the only types of
- * objects that are explicitly tracked this way.
+ * Currently, relations, user-defined functions and FDW-related objects are
+ * the only types of objects that are explicitly tracked this way.
*
* 8. We assign every plan node in the tree a unique ID.
*
@@ -426,7 +430,13 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)
* but it would probably cost more cycles than it would save.
*/
if (newrte->rtekind == RTE_RELATION)
+ {
glob->relationOids = lappend_oid(glob->relationOids, newrte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (newrte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(glob, newrte->relid);
+ }
}
/*
@@ -2423,22 +2433,55 @@ record_plan_function_dependency(PlannerInfo *root, Oid funcid)
*/
if (funcid >= (Oid) FirstBootstrapObjectId)
{
- PlanInvalItem *inval_item = makeNode(PlanInvalItem);
-
/*
* It would work to use any syscache on pg_proc, but the easiest is
* PROCOID since we already have the function's OID at hand. Note
* that plancache.c knows we use PROCOID.
*/
- inval_item->cacheId = PROCOID;
- inval_item->hashValue = GetSysCacheHashValue1(PROCOID,
- ObjectIdGetDatum(funcid));
-
- root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
+ add_inval_item(root->glob, PROCOID, funcid);
}
}
/*
+ * record_plan_foreign_dependencies
+ * Mark the current plan as depending on FDW-related objects.
+ *
+ * This is required since modifications to attributes of FDW-related objects,
+ * such as FDW options, might require replanning.
+ */
+static void
+record_plan_foreign_dependencies(PlannerGlobal *glob, Oid relid)
+{
+ ForeignTable *table = GetForeignTable(relid);
+ ForeignServer *server = GetForeignServer(table->serverid);
+
+ /* Record the dependency on the foreign table */
+ add_inval_item(glob, FOREIGNTABLEREL, relid);
+
+ /* Likewise for the foreign server */
+ add_inval_item(glob, FOREIGNSERVEROID, table->serverid);
+
+ /* Likewise for the foreign data wrapper */
+ add_inval_item(glob, FOREIGNDATAWRAPPEROID, server->fdwid);
+}
+
+/*
+ * add_inval_item
+ * Add given dependency to glob->invalItems.
+ */
+static void
+add_inval_item(PlannerGlobal *glob, int cacheid, Oid oid)
+{
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ inval_item->cacheId = cacheid;
+ inval_item->hashValue = GetSysCacheHashValue1(cacheid,
+ ObjectIdGetDatum(oid));
+
+ glob->invalItems = lappend(glob->invalItems, inval_item);
+}
+
+/*
* extract_query_dependencies
* Given a rewritten, but not yet planned, query or queries
* (i.e. a Query node or list of Query nodes), extract dependencies
@@ -2510,8 +2553,14 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context)
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
if (rte->rtekind == RTE_RELATION)
+ {
context->glob->relationOids =
lappend_oid(context->glob->relationOids, rte->relid);
+
+ /* Collect dependencies on FDW-related objects too */
+ if (rte->relkind == RELKIND_FOREIGN_TABLE)
+ record_plan_foreign_dependencies(context->glob, rte->relid);
+ }
}
/* And recurse into the query's subexpressions */
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index c96a865..4637745 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -27,15 +27,15 @@
* query to change output tupdesc on replan --- if so, it's up to the
* caller to notice changes and cope with them.
*
- * Currently, we track exactly the dependencies of plans on relations and
- * user-defined functions. 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.
+ * Currently, we track exactly the dependencies of plans on relations,
+ * user-defined functions and FDW-related objects. On the corresponding
+ * 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.
*
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
@@ -116,6 +116,9 @@ InitPlanCache(void)
{
CacheRegisterRelcacheCallback(PlanCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(PROCOID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNTABLEREL, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheFuncCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheFuncCallback, (Datum) 0);
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
@@ -1751,8 +1754,9 @@ PlanCacheRelCallback(Datum arg, Oid relid)
* Invalidate all plans mentioning the object with the specified hash value,
* or all plans mentioning any member of this cache if hashvalue == 0.
*
- * Note that the coding would support use for multiple caches, but right
- * now only user-defined functions are tracked this way.
+ * Note that the coding would support use for multiple caches, and currently,
+ * in addition to user-defined functions, FDW-related objects are tracked this
+ * way.
*/
static void
PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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]/messages/by-id/29681.1459779873@sss.pgh.pa.us. 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]: /messages/by-id/29681.1459779873@sss.pgh.pa.us
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[ 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
foreign-plan-inval.patchtext/x-patch; name=foreign-plan-inval.patchDownload
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 3575,3586 **** EXECUTE st5('foo', 1);
--- 3575,3754 ----
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+ -- altering FDW options requires replanning
+ PREPARE st6 AS SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ ----------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1 t1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = c2))
+ (3 rows)
+
+ PREPARE st7(int,int,text) AS INSERT INTO ft1 (c1,c2,c3) VALUES ($1,$2,$3);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7(1001,101,'foo');
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Insert on public.ft1
+ Remote SQL: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+ -> Result
+ Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft1 '::character(10), NULL::user_enum
+ (4 rows)
+
+ PREPARE st8(int) AS UPDATE ft1 SET c3 = 'bar' WHERE c1 = $1 RETURNING *;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8(1001);
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------
+ Update on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ -> Foreign Update on public.ft1
+ Remote SQL: UPDATE "S 1"."T 1" SET c3 = 'bar'::text WHERE (("C 1" = 1001)) RETURNING "C 1", c2, c3, c4, c5, c6, c7, c8
+ (4 rows)
+
+ PREPARE st9(int) AS DELETE FROM ft1 WHERE c1 = $1 RETURNING *;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9(1001);
+ QUERY PLAN
+ ----------------------------------------------------------------------------------------------------------------
+ Delete on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ -> Foreign Delete on public.ft1
+ Remote SQL: DELETE FROM "S 1"."T 1" WHERE (("C 1" = 1001)) RETURNING "C 1", c2, c3, c4, c5, c6, c7, c8
+ (4 rows)
+
+ ALTER TABLE "S 1"."T 1" RENAME TO "T 0";
+ ALTER FOREIGN TABLE ft1 OPTIONS (SET table_name 'T 0');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ QUERY PLAN
+ ----------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1 t1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 0" WHERE (("C 1" = c2))
+ (3 rows)
+
+ EXECUTE st6;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+ ----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+ 2 | 2 | 00002 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2 | 2 | foo
+ 3 | 3 | 00003 | Sun Jan 04 00:00:00 1970 PST | Sun Jan 04 00:00:00 1970 | 3 | 3 | foo
+ 4 | 4 | 00004 | Mon Jan 05 00:00:00 1970 PST | Mon Jan 05 00:00:00 1970 | 4 | 4 | foo
+ 5 | 5 | 00005 | Tue Jan 06 00:00:00 1970 PST | Tue Jan 06 00:00:00 1970 | 5 | 5 | foo
+ 6 | 6 | 00006 | Wed Jan 07 00:00:00 1970 PST | Wed Jan 07 00:00:00 1970 | 6 | 6 | foo
+ 7 | 7 | 00007 | Thu Jan 08 00:00:00 1970 PST | Thu Jan 08 00:00:00 1970 | 7 | 7 | foo
+ 8 | 8 | 00008 | Fri Jan 09 00:00:00 1970 PST | Fri Jan 09 00:00:00 1970 | 8 | 8 | foo
+ 9 | 9 | 00009 | Sat Jan 10 00:00:00 1970 PST | Sat Jan 10 00:00:00 1970 | 9 | 9 | foo
+ (9 rows)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7(1001,101,'foo');
+ QUERY PLAN
+ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Insert on public.ft1
+ Remote SQL: INSERT INTO "S 1"."T 0"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+ -> Result
+ Output: NULL::integer, 1001, 101, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft1 '::character(10), NULL::user_enum
+ (4 rows)
+
+ EXECUTE st7(1001,101,'foo');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8(1001);
+ QUERY PLAN
+ --------------------------------------------------------------------------------------------------------------------------------
+ Update on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ -> Foreign Update on public.ft1
+ Remote SQL: UPDATE "S 1"."T 0" SET c3 = 'bar'::text WHERE (("C 1" = 1001)) RETURNING "C 1", c2, c3, c4, c5, c6, c7, c8
+ (4 rows)
+
+ EXECUTE st8(1001);
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+ ------+-----+-----+----+----+----+------------+----
+ 1001 | 101 | bar | | | | ft1 |
+ (1 row)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9(1001);
+ QUERY PLAN
+ ----------------------------------------------------------------------------------------------------------------
+ Delete on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ -> Foreign Delete on public.ft1
+ Remote SQL: DELETE FROM "S 1"."T 0" WHERE (("C 1" = 1001)) RETURNING "C 1", c2, c3, c4, c5, c6, c7, c8
+ (4 rows)
+
+ EXECUTE st9(1001);
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+ ------+-----+-----+----+----+----+------------+----
+ 1001 | 101 | bar | | | | ft1 |
+ (1 row)
+
+ ALTER TABLE "S 1"."T 0" RENAME TO "T 1";
+ ALTER FOREIGN TABLE ft1 OPTIONS (SET table_name 'T 1');
+ PREPARE st10 AS SELECT count(c3) FROM ft1 t1 WHERE t1.c1 = postgres_fdw_abs(t1.c2);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ -----------------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(c3))
+ Relations: Aggregate on (public.ft1 t1)
+ Remote SQL: SELECT count(c3) FROM "S 1"."T 1" WHERE (("C 1" = public.postgres_fdw_abs(c2)))
+ (4 rows)
+
+ PREPARE st11 AS SELECT count(c3) FROM ft1 t1 WHERE t1.c1 === t1.c2;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st11;
+ QUERY PLAN
+ -----------------------------------------------------------------------------------------
+ Foreign Scan
+ Output: (count(c3))
+ Relations: Aggregate on (public.ft1 t1)
+ Remote SQL: SELECT count(c3) FROM "S 1"."T 1" WHERE (("C 1" OPERATOR(public.===) c2))
+ (4 rows)
+
+ ALTER SERVER loopback OPTIONS (DROP extensions);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ QUERY PLAN
+ -----------------------------------------------------------
+ Aggregate
+ Output: count(c3)
+ -> Foreign Scan on public.ft1 t1
+ Output: c3
+ Filter: (t1.c1 = postgres_fdw_abs(t1.c2))
+ Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1"
+ (6 rows)
+
+ EXECUTE st10;
+ count
+ -------
+ 9
+ (1 row)
+
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st11;
+ QUERY PLAN
+ -----------------------------------------------------------
+ Aggregate
+ Output: count(c3)
+ -> Foreign Scan on public.ft1 t1
+ Output: c3
+ Filter: (t1.c1 === t1.c2)
+ Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1"
+ (6 rows)
+
+ EXECUTE st11;
+ count
+ -------
+ 9
+ (1 row)
+
+ ALTER SERVER loopback OPTIONS (ADD extensions 'postgres_fdw');
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
+ DEALLOCATE st11;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 881,892 **** EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
--- 881,930 ----
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st5('foo', 1);
EXECUTE st5('foo', 1);
+ -- altering FDW options requires replanning
+ PREPARE st6 AS SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ PREPARE st7(int,int,text) AS INSERT INTO ft1 (c1,c2,c3) VALUES ($1,$2,$3);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7(1001,101,'foo');
+ PREPARE st8(int) AS UPDATE ft1 SET c3 = 'bar' WHERE c1 = $1 RETURNING *;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8(1001);
+ PREPARE st9(int) AS DELETE FROM ft1 WHERE c1 = $1 RETURNING *;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9(1001);
+ ALTER TABLE "S 1"."T 1" RENAME TO "T 0";
+ ALTER FOREIGN TABLE ft1 OPTIONS (SET table_name 'T 0');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st6;
+ EXECUTE st6;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st7(1001,101,'foo');
+ EXECUTE st7(1001,101,'foo');
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st8(1001);
+ EXECUTE st8(1001);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st9(1001);
+ EXECUTE st9(1001);
+ ALTER TABLE "S 1"."T 0" RENAME TO "T 1";
+ ALTER FOREIGN TABLE ft1 OPTIONS (SET table_name 'T 1');
+ PREPARE st10 AS SELECT count(c3) FROM ft1 t1 WHERE t1.c1 = postgres_fdw_abs(t1.c2);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ PREPARE st11 AS SELECT count(c3) FROM ft1 t1 WHERE t1.c1 === t1.c2;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st11;
+ ALTER SERVER loopback OPTIONS (DROP extensions);
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st10;
+ EXECUTE st10;
+ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st11;
+ EXECUTE st11;
+ ALTER SERVER loopback OPTIONS (ADD extensions 'postgres_fdw');
+
-- cleanup
DEALLOCATE st1;
DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
+ DEALLOCATE st6;
+ DEALLOCATE st7;
+ DEALLOCATE st8;
+ DEALLOCATE st9;
+ DEALLOCATE st10;
+ DEALLOCATE st11;
-- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS OFF)
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 11266,11271 **** ATExecGenericOptions(Relation rel, List *options)
--- 11266,11277 ----
simple_heap_update(ftrel, &tuple->t_self, tuple);
CatalogUpdateIndexes(ftrel, tuple);
+ /*
+ * Invalidate relcache so that after this commit all sessions will
+ * refresh any cached plans that might reference the old options
+ */
+ CacheInvalidateRelcache(rel);
+
InvokeObjectPostAlterHook(ForeignTableRelationId,
RelationGetRelid(rel), 0);
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
***************
*** 118,123 **** InitPlanCache(void)
--- 118,125 ----
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID, PlanCacheSysCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, PlanCacheSysCallback, (Datum) 0);
}
/*
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 30, 2016 at 8:05 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.
Moved to next CF with "needs review" status.
Regards,
Hari Babu
Fujitsu Australia
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers