Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. - Mailing list pgsql-hackers

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

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

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

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


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

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

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

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

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

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


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


regards,

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