Avoid multiple foreign server connections when all use same user mapping.

Previously, postgres_fdw's connection cache was keyed by user OID and
server OID, but this can lead to multiple connections when it's not
really necessary.  In particular, if all relevant users are mapped to
the public user mapping, then their connection options are certainly
the same, so one connection can be used for all of them.

While we're cleaning things up here, drop the "server" argument to
GetConnection(), which isn't really needed.  This saves a few cycles
because callers no longer have to look this up; the function itself
does, but only when establishing a new connection, not when reusing
an existing one.

Ashutosh Bapat, with a few small changes by me.

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 0c6c3cee3070092d67b0abfacc8733cb69fc0b57..6e664b0213a4e9eff399dde092a983fc0570eac8 100644 (file)
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -24,9 +24,11 @@
 /*
  * Connection cache hash table entry
  *
- * The lookup key in this hash table is the foreign server OID plus the user
- * mapping OID.  (We use just one connection per user per foreign server,
- * so that we can ensure all scans use the same snapshot during a query.)
+ * The lookup key in this hash table is the user mapping OID. We use just one
+ * connection per user mapping ID, which ensures that all the scans use the
+ * same snapshot during a query.  Using the user mapping OID rather than
+ * the foreign server OID + user OID avoids creating multiple connections when
+ * the public user mapping applies to all user OIDs.
  *
  * The "conn" pointer can be NULL if we don't currently have a live connection.
  * When we do have a connection, xact_depth tracks the current depth of
@@ -35,11 +37,7 @@
  * ourselves, so that rolling back a subtransaction will kill the right
  * queries and not the wrong ones.
  */
-typedef struct ConnCacheKey
-{
-       Oid                     serverid;               /* OID of foreign server */
-       Oid                     userid;                 /* OID of local user whose mapping we use */
-} ConnCacheKey;
+typedef Oid ConnCacheKey;
 
 typedef struct ConnCacheEntry
 {
@@ -94,8 +92,7 @@ static void pgfdw_subxact_callback(SubXactEvent event,
  * mid-transaction anyway.
  */
 PGconn *
-GetConnection(ForeignServer *server, UserMapping *user,
-                         bool will_prep_stmt)
+GetConnection(UserMapping *user, bool will_prep_stmt)
 {
        bool            found;
        ConnCacheEntry *entry;
@@ -127,8 +124,7 @@ GetConnection(ForeignServer *server, UserMapping *user,
        xact_got_connection = true;
 
        /* Create hash key for the entry.  Assume no pad bytes in key struct */
-       key.serverid = server->serverid;
-       key.userid = user->userid;
+       key = user->umid;
 
        /*
         * Find or create cached entry for requested connection.
@@ -156,12 +152,15 @@ GetConnection(ForeignServer *server, UserMapping *user,
         */
        if (entry->conn == NULL)
        {
+               ForeignServer *server = GetForeignServer(user->serverid);
+
                entry->xact_depth = 0;  /* just to be sure */
                entry->have_prep_stmt = false;
                entry->have_error = false;
                entry->conn = connect_pg_server(server, user);
-               elog(DEBUG3, "new postgres_fdw connection %p for server \"%s\"",
-                        entry->conn, server->servername);
+
+               elog(DEBUG3, "new postgres_fdw connection %p for server \"%s\ (user mapping oid %d, userid %d)",
+                        entry->conn, server->servername, user->umid, user->userid);
        }
 
        /*

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 374faf5a25ac77a3492f5acbb13b497547d898d1..a237e152c02f0fa60c9f3796ccc121ecc0ea83ff 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1101,7 +1101,6 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
        RangeTblEntry *rte;
        Oid                     userid;
        ForeignTable *table;
-       ForeignServer *server;
        UserMapping *user;
        int                     numParams;
        int                     i;
@@ -1129,14 +1128,13 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
        /* Get info about foreign table. */
        fsstate->rel = node->ss.ss_currentRelation;
        table = GetForeignTable(RelationGetRelid(fsstate->rel));
-       server = GetForeignServer(table->serverid);
-       user = GetUserMapping(userid, server->serverid);
+       user = GetUserMapping(userid, table->serverid);
 
        /*
         * Get connection to the foreign server.  Connection manager will
         * establish new connection if necessary.
         */
-       fsstate->conn = GetConnection(server, user, false);
+       fsstate->conn = GetConnection(user, false);
 
        /* Assign a unique ID for my cursor */
        fsstate->cursor_number = GetCursorNumber(fsstate->conn);
@@ -1503,7 +1501,6 @@ postgresBeginForeignModify(ModifyTableState *mtstate,
        RangeTblEntry *rte;
        Oid                     userid;
        ForeignTable *table;
-       ForeignServer *server;
        UserMapping *user;
        AttrNumber      n_params;
        Oid                     typefnoid;
@@ -1530,11 +1527,10 @@ postgresBeginForeignModify(ModifyTableState *mtstate,
 
        /* Get info about foreign table. */
        table = GetForeignTable(RelationGetRelid(rel));
-       server = GetForeignServer(table->serverid);
-       user = GetUserMapping(userid, server->serverid);
+       user = GetUserMapping(userid, table->serverid);
 
        /* Open connection; report that we'll create a prepared statement. */
-       fmstate->conn = GetConnection(server, user, true);
+       fmstate->conn = GetConnection(user, true);
        fmstate->p_name = NULL;         /* prepared statement not made yet */
 
        /* Deconstruct fdw_private data. */
@@ -1988,7 +1984,7 @@ estimate_path_cost_size(PlannerInfo *root,
                        appendOrderByClause(&sql, root, baserel, pathkeys);
 
                /* Get the remote estimate */
-               conn = GetConnection(fpinfo->server, fpinfo->user, false);
+               conn = GetConnection(fpinfo->user, false);
                get_remote_estimate(sql.data, conn, &rows, &width,
                                                        &startup_cost, &total_cost);
                ReleaseConnection(conn);
@@ -2544,7 +2540,6 @@ postgresAnalyzeForeignTable(Relation relation,
                                                        BlockNumber *totalpages)
 {
        ForeignTable *table;
-       ForeignServer *server;
        UserMapping *user;
        PGconn     *conn;
        StringInfoData sql;
@@ -2565,9 +2560,8 @@ postgresAnalyzeForeignTable(Relation relation,
         * owner, even if the ANALYZE was started by some other user.
         */
        table = GetForeignTable(RelationGetRelid(relation));
-       server = GetForeignServer(table->serverid);
-       user = GetUserMapping(relation->rd_rel->relowner, server->serverid);
-       conn = GetConnection(server, user, false);
+       user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
+       conn = GetConnection(user, false);
 
        /*
         * Construct command to get page count for relation.
@@ -2626,7 +2620,6 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 {
        PgFdwAnalyzeState astate;
        ForeignTable *table;
-       ForeignServer *server;
        UserMapping *user;
        PGconn     *conn;
        unsigned int cursor_number;
@@ -2657,9 +2650,8 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
         * owner, even if the ANALYZE was started by some other user.
         */
        table = GetForeignTable(RelationGetRelid(relation));
-       server = GetForeignServer(table->serverid);
-       user = GetUserMapping(relation->rd_rel->relowner, server->serverid);
-       conn = GetConnection(server, user, false);
+       user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
+       conn = GetConnection(user, false);
 
        /*
         * Construct cursor that retrieves whole rows from remote.
@@ -2860,7 +2852,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
         */
        server = GetForeignServer(serverOid);
        mapping = GetUserMapping(GetUserId(), server->serverid);
-       conn = GetConnection(server, mapping, false);
+       conn = GetConnection(mapping, false);
 
        /* Don't attempt to import collation if remote server hasn't got it */
        if (PQserverVersion(conn) < 90100)

diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 85535360475f4847acc4eef5498d9bf8c00ae7cf..59e9f60c04bbe065f1d207360b77f5a8d7df0159 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -60,8 +60,7 @@ extern int    set_transmission_modes(void);
 extern void reset_transmission_modes(int nestlevel);
 
 /* in connection.c */
-extern PGconn *GetConnection(ForeignServer *server, UserMapping *user,
-                         bool will_prep_stmt);
+extern PGconn *GetConnection(UserMapping *user, bool will_prep_stmt);
 extern void ReleaseConnection(PGconn *conn);
 extern unsigned int GetCursorNumber(PGconn *conn);
 extern unsigned int GetPrepStmtNumber(PGconn *conn);

diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 14e082b5b8b2b62d58151756847cf87f7a67a1b1..c24b11b685ce16c0183630bb01397bc135b8e1e9 100644 (file)
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -193,6 +193,7 @@ GetUserMapping(Oid userid, Oid serverid)
                                                MappingUserName(userid))));
 
        um = (UserMapping *) palloc(sizeof(UserMapping));
+       um->umid = HeapTupleGetOid(tp);
        um->userid = userid;
        um->serverid = serverid;
 

diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 2c1ada1b12a41b1e11d6950a882842a538b26214..5dc2c90f3c31028ded6c7299614bf30d0fe68ca1 100644 (file)
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -55,6 +55,7 @@ typedef struct ForeignServer
 
 typedef struct UserMapping
 {
+       Oid                     umid;                   /* Oid of user mapping */
        Oid                     userid;                 /* local user Oid */
        Oid                     serverid;               /* server Oid */
        List       *options;            /* useoptions as DefElem list */