On 2016/04/05 0:23, Tom Lane wrote:
> Amit Langote <amitlangot...@gmail.com> writes:
>> On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <t...@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
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);
 }
 
 /*
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to