Thread: dblink vs SQL/MED

dblink vs SQL/MED

From
Tom Lane
Date:
Peter wrote:
> SQL/MED catalog manipulation facilities
> 
> This doesn't do any remote or external things yet, but it gives modules
> like plproxy and dblink a standardized and future-proof system for
> managing their connection information.

It seems this is a pile of pretty useless code, so far as the core
distribution is concerned, unless somebody fixes dblink to use it.
Is that on anyone's radar for 8.4?
        regards, tom lane


Re: dblink vs SQL/MED

From
Joe Conway
Date:
Tom Lane wrote:
> Peter wrote:
>> SQL/MED catalog manipulation facilities
>>
>> This doesn't do any remote or external things yet, but it gives modules
>> like plproxy and dblink a standardized and future-proof system for
>> managing their connection information.
> 
> It seems this is a pile of pretty useless code, so far as the core
> distribution is concerned, unless somebody fixes dblink to use it.
> Is that on anyone's radar for 8.4?

How much time is left to get it done? I might be able to find some time 
on the weekend after Christmas, or the weekend after New Years day -- is 
that soon enough?

Joe


Re: dblink vs SQL/MED

From
Tom Lane
Date:
Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> It seems this is a pile of pretty useless code, so far as the core
>> distribution is concerned, unless somebody fixes dblink to use it.
>> Is that on anyone's radar for 8.4?

> How much time is left to get it done? I might be able to find some time 
> on the weekend after Christmas, or the weekend after New Years day -- is 
> that soon enough?

Well, it seems quite clear to me that we're not entering beta on Jan 1.
And even if we were, we've traditionally cut more slack for contrib
modules than the core database.  So if you can do something about it in
the next few weeks, that'd be great.
        regards, tom lane


Re: dblink vs SQL/MED

From
Peter Eisentraut
Date:
On Saturday 20 December 2008 19:33:17 Tom Lane wrote:
> Peter wrote:
> > SQL/MED catalog manipulation facilities
> >
> > This doesn't do any remote or external things yet, but it gives modules
> > like plproxy and dblink a standardized and future-proof system for
> > managing their connection information.
>
> It seems this is a pile of pretty useless code, so far as the core
> distribution is concerned, unless somebody fixes dblink to use it.
> Is that on anyone's radar for 8.4?

Martin had sent some code for that with his original patch series.  I or 
someone can review that next.


Re: dblink vs SQL/MED

From
Joe Conway
Date:
Peter Eisentraut wrote:
> On Saturday 20 December 2008 19:33:17 Tom Lane wrote:
>> Peter wrote:
>>> SQL/MED catalog manipulation facilities
>>>
>>> This doesn't do any remote or external things yet, but it gives modules
>>> like plproxy and dblink a standardized and future-proof system for
>>> managing their connection information.
>> It seems this is a pile of pretty useless code, so far as the core
>> distribution is concerned, unless somebody fixes dblink to use it.
>> Is that on anyone's radar for 8.4?
>
> Martin had sent some code for that with his original patch series.  I or
> someone can review that next.

Here is what I would propose (still needs documentation and regression
test changes, but wanted feedback first). I think it is better to keep
the changes to dblink very simple.

After looking for an already existing dblink rconn, the passed connstr
is checked to see if it matches a valid foreign data server prior to
being used as a connstr. If so, a connstr is constructed from the
foreign server and user mapping options (for current user). The
resulting connstr is then treated exactly as if it were one that was
passed directly to dblink.

Two specific questions on this approach:
1. This implies that the exact same dblink_connstr_check() is performed
    on a predefined foreign server and user mapping as a raw connstr --
    is this desireable? I'm not entirely clear on the intended purpose
    and use of foreign data wrappers yet.
2. It seems like get_connect_string() is generically useful to any
    client of postgresql_fdw.c -- should it go there instead of dblink?

Thanks,

Joe
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.77
diff -c -r1.77 dblink.c
*** dblink.c    1 Jan 2009 17:23:31 -0000    1.77
--- dblink.c    2 Jan 2009 22:14:43 -0000
***************
*** 46,51 ****
--- 46,52 ----
  #include "catalog/pg_type.h"
  #include "executor/executor.h"
  #include "executor/spi.h"
+ #include "foreign/foreign.h"
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
  #include "nodes/execnodes.h"
***************
*** 96,101 ****
--- 97,103 ----
  static void dblink_connstr_check(const char *connstr);
  static void dblink_security_check(PGconn *conn, remoteConn *rconn);
  static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
+ static char *get_connect_string(const char *servername);

  /* Global */
  static remoteConn *pconn = NULL;
***************
*** 165,171 ****
              } \
              else \
              { \
!                 connstr = conname_or_str; \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
--- 167,175 ----
              } \
              else \
              { \
!                 connstr = get_connect_string(conname_or_str); \
!                 if (connstr == NULL) \
!                     connstr = conname_or_str; \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
***************
*** 210,215 ****
--- 214,220 ----
  Datum
  dblink_connect(PG_FUNCTION_ARGS)
  {
+     char       *conname_or_str = NULL;
      char       *connstr = NULL;
      char       *connname = NULL;
      char       *msg;
***************
*** 220,235 ****

      if (PG_NARGS() == 2)
      {
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
--- 225,245 ----

      if (PG_NARGS() == 2)
      {
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

+     /* first check for valid foreign data server */
+     connstr = get_connect_string(conname_or_str);
+     if (connstr == NULL)
+         connstr = conname_or_str;
+
      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
***************
*** 2358,2360 ****
--- 2368,2410 ----
           errcontext("Error occurred on dblink connection named \"%s\": %s.",
                      dblink_context_conname, dblink_context_msg)));
  }
+
+ /*
+  * Obtain connection string for a foreign server
+  */
+ static char *
+ get_connect_string(const char *servername)
+ {
+     ForeignServer       *foreign_server;
+     UserMapping           *user_mapping;
+     ListCell           *cell;
+     StringInfo            buf = makeStringInfo();
+
+     /* first gather the server connstr options */
+     foreign_server = GetForeignServerByName(servername, true);
+
+     if (foreign_server)
+     {
+         foreach (cell, foreign_server->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         /* next get the user connstr options */
+         user_mapping = GetUserMapping(GetUserId(), foreign_server->serverid);
+         foreach (cell, user_mapping->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         return pstrdup(buf->data);
+     }
+     else
+         return NULL;
+ }

Re: dblink vs SQL/MED

From
Joe Conway
Date:
Joe Conway wrote:
> Peter Eisentraut wrote:
>> Martin had sent some code for that with his original patch series.  I
>> or someone can review that next.
>
> Here is what I would propose (still needs documentation and regression
> test changes, but wanted feedback first). I think it is better to keep
> the changes to dblink very simple.

The attached now includes documentation and regression test changes. It
also includes the refactoring to pull dblink_send_query() out of
dblink_record_internal() and the fix for the bug reported by Oleksiy
Shchukin.

> After looking for an already existing dblink rconn, the passed connstr
> is checked to see if it matches a valid foreign data server prior to
> being used as a connstr. If so, a connstr is constructed from the
> foreign server and user mapping options (for current user). The
> resulting connstr is then treated exactly as if it were one that was
> passed directly to dblink.
>
> Two specific questions on this approach:
> 1. This implies that the exact same dblink_connstr_check() is performed
>    on a predefined foreign server and user mapping as a raw connstr --
>    is this desireable? I'm not entirely clear on the intended purpose
>    and use of foreign data wrappers yet.

On the one hand, why be any less stringent on an fdw server than any
other connect string? But on the other hand, the fdw server definition
has supposedly been vetted by a superuser. Any thoughts of this?

> 2. It seems like get_connect_string() is generically useful to any
>    client of postgresql_fdw.c -- should it go there instead of dblink?

I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
-- objections?

Thanks,

Joe
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.77
diff -c -r1.77 dblink.c
*** contrib/dblink/dblink.c    1 Jan 2009 17:23:31 -0000    1.77
--- contrib/dblink/dblink.c    3 Jan 2009 21:25:34 -0000
***************
*** 46,51 ****
--- 46,52 ----
  #include "catalog/pg_type.h"
  #include "executor/executor.h"
  #include "executor/spi.h"
+ #include "foreign/foreign.h"
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
  #include "nodes/execnodes.h"
***************
*** 77,83 ****
  /*
   * Internal declarations
   */
! static Datum dblink_record_internal(FunctionCallInfo fcinfo, bool is_async, bool do_get);
  static remoteConn *getConnectionByName(const char *name);
  static HTAB *createConnHash(void);
  static void createNewConnection(const char *name, remoteConn * rconn);
--- 78,84 ----
  /*
   * Internal declarations
   */
! static Datum dblink_record_internal(FunctionCallInfo fcinfo, bool is_async);
  static remoteConn *getConnectionByName(const char *name);
  static HTAB *createConnHash(void);
  static void createNewConnection(const char *name, remoteConn * rconn);
***************
*** 96,101 ****
--- 97,103 ----
  static void dblink_connstr_check(const char *connstr);
  static void dblink_security_check(PGconn *conn, remoteConn *rconn);
  static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
+ static char *get_connect_string(const char *servername);

  /* Global */
  static remoteConn *pconn = NULL;
***************
*** 165,171 ****
              } \
              else \
              { \
!                 connstr = conname_or_str; \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
--- 167,175 ----
              } \
              else \
              { \
!                 connstr = get_connect_string(conname_or_str); \
!                 if (connstr == NULL) \
!                     connstr = conname_or_str; \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
***************
*** 210,215 ****
--- 214,220 ----
  Datum
  dblink_connect(PG_FUNCTION_ARGS)
  {
+     char       *conname_or_str = NULL;
      char       *connstr = NULL;
      char       *connname = NULL;
      char       *msg;
***************
*** 220,235 ****

      if (PG_NARGS() == 2)
      {
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
--- 225,245 ----

      if (PG_NARGS() == 2)
      {
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

+     /* first check for valid foreign data server */
+     connstr = get_connect_string(conname_or_str);
+     if (connstr == NULL)
+         connstr = conname_or_str;
+
      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
***************
*** 689,713 ****
  Datum
  dblink_record(PG_FUNCTION_ARGS)
  {
!     return dblink_record_internal(fcinfo, false, false);
  }

  PG_FUNCTION_INFO_V1(dblink_send_query);
  Datum
  dblink_send_query(PG_FUNCTION_ARGS)
  {
!     return dblink_record_internal(fcinfo, true, false);
  }

  PG_FUNCTION_INFO_V1(dblink_get_result);
  Datum
  dblink_get_result(PG_FUNCTION_ARGS)
  {
!     return dblink_record_internal(fcinfo, true, true);
  }

  static Datum
! dblink_record_internal(FunctionCallInfo fcinfo, bool is_async, bool do_get)
  {
      FuncCallContext *funcctx;
      TupleDesc    tupdesc = NULL;
--- 699,745 ----
  Datum
  dblink_record(PG_FUNCTION_ARGS)
  {
!     return dblink_record_internal(fcinfo, false);
  }

  PG_FUNCTION_INFO_V1(dblink_send_query);
  Datum
  dblink_send_query(PG_FUNCTION_ARGS)
  {
!     PGconn       *conn = NULL;
!     char       *connstr = NULL;
!     char       *sql = NULL;
!     remoteConn *rconn = NULL;
!     char       *msg;
!     bool        freeconn = false;
!     int            retval;
!
!     if (PG_NARGS() == 2)
!     {
!         DBLINK_GET_CONN;
!         sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
!     }
!     else
!         /* shouldn't happen */
!         elog(ERROR, "wrong number of arguments");
!
!     /* async query send */
!     retval = PQsendQuery(conn, sql);
!     if (retval != 1)
!         elog(NOTICE, "%s", PQerrorMessage(conn));
!
!     PG_RETURN_INT32(retval);
  }

  PG_FUNCTION_INFO_V1(dblink_get_result);
  Datum
  dblink_get_result(PG_FUNCTION_ARGS)
  {
!     return dblink_record_internal(fcinfo, true);
  }

  static Datum
! dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
  {
      FuncCallContext *funcctx;
      TupleDesc    tupdesc = NULL;
***************
*** 775,788 ****
                  /* shouldn't happen */
                  elog(ERROR, "wrong number of arguments");
          }
!         else if (is_async && do_get)
          {
              /* get async result */
              if (PG_NARGS() == 2)
              {
                  /* text,bool */
                  DBLINK_GET_CONN;
!                 fail = PG_GETARG_BOOL(2);
              }
              else if (PG_NARGS() == 1)
              {
--- 807,820 ----
                  /* shouldn't happen */
                  elog(ERROR, "wrong number of arguments");
          }
!         else /* is_async */
          {
              /* get async result */
              if (PG_NARGS() == 2)
              {
                  /* text,bool */
                  DBLINK_GET_CONN;
!                 fail = PG_GETARG_BOOL(1);
              }
              else if (PG_NARGS() == 1)
              {
***************
*** 793,929 ****
                  /* shouldn't happen */
                  elog(ERROR, "wrong number of arguments");
          }
-         else
-         {
-             /* send async query */
-             if (PG_NARGS() == 2)
-             {
-                 DBLINK_GET_CONN;
-                 sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
-             }
-             else
-                 /* shouldn't happen */
-                 elog(ERROR, "wrong number of arguments");
-         }

          if (!conn)
              DBLINK_CONN_NOT_AVAIL;

!         if (!is_async || (is_async && do_get))
          {
!             /* synchronous query, or async result retrieval */
!             if (!is_async)
!                 res = PQexec(conn, sql);
!             else
!             {
!                 res = PQgetResult(conn);
!                 /* NULL means we're all done with the async results */
!                 if (!res)
!                 {
!                     MemoryContextSwitchTo(oldcontext);
!                     SRF_RETURN_DONE(funcctx);
!                 }
!             }
!
!             if (!res ||
!                 (PQresultStatus(res) != PGRES_COMMAND_OK &&
!                  PQresultStatus(res) != PGRES_TUPLES_OK))
              {
-                 dblink_res_error(conname, res, "could not execute query", fail);
-                 if (freeconn)
-                     PQfinish(conn);
                  MemoryContextSwitchTo(oldcontext);
                  SRF_RETURN_DONE(funcctx);
              }

!             if (PQresultStatus(res) == PGRES_COMMAND_OK)
!             {
!                 is_sql_cmd = true;
!
!                 /* need a tuple descriptor representing one TEXT column */
!                 tupdesc = CreateTemplateTupleDesc(1, false);
!                 TupleDescInitEntry(tupdesc, (AttrNumber) 1, "status",
!                                    TEXTOID, -1, 0);
!
!                 /*
!                  * and save a copy of the command status string to return as
!                  * our result tuple
!                  */
!                 sql_cmd_status = PQcmdStatus(res);
!                 funcctx->max_calls = 1;
!             }
!             else
!                 funcctx->max_calls = PQntuples(res);
!
!             /* got results, keep track of them */
!             funcctx->user_fctx = res;
!
!             /* if needed, close the connection to the database and cleanup */
              if (freeconn)
                  PQfinish(conn);

!             if (!is_sql_cmd)
!             {
!                 /* get a tuple descriptor for our result type */
!                 switch (get_call_result_type(fcinfo, NULL, &tupdesc))
!                 {
!                     case TYPEFUNC_COMPOSITE:
!                         /* success */
!                         break;
!                     case TYPEFUNC_RECORD:
!                         /* failed to determine actual type of RECORD */
!                         ereport(ERROR,
!                                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                         errmsg("function returning record called in context "
!                                "that cannot accept type record")));
!                         break;
!                     default:
!                         /* result type isn't composite */
!                         elog(ERROR, "return type must be a row type");
!                         break;
!                 }

!                 /* make sure we have a persistent copy of the tupdesc */
!                 tupdesc = CreateTupleDescCopy(tupdesc);
!             }

              /*
!              * check result and tuple descriptor have the same number of
!              * columns
               */
!             if (PQnfields(res) != tupdesc->natts)
!                 ereport(ERROR,
!                         (errcode(ERRCODE_DATATYPE_MISMATCH),
!                          errmsg("remote query result rowtype does not match "
!                                 "the specified FROM clause rowtype")));

!             /* fast track when no results */
!             if (funcctx->max_calls < 1)
              {
!                 if (res)
!                     PQclear(res);
!                 MemoryContextSwitchTo(oldcontext);
!                 SRF_RETURN_DONE(funcctx);
              }

!             /* store needed metadata for subsequent calls */
!             attinmeta = TupleDescGetAttInMetadata(tupdesc);
!             funcctx->attinmeta = attinmeta;
!
!             MemoryContextSwitchTo(oldcontext);
          }
!         else
          {
!             /* async query send */
              MemoryContextSwitchTo(oldcontext);
!             PG_RETURN_INT32(PQsendQuery(conn, sql));
          }
-     }

!     if (is_async && !do_get)
!     {
!         /* async query send -- should not happen */
!         elog(ERROR, "async query send called more than once");

      }

--- 825,934 ----
                  /* shouldn't happen */
                  elog(ERROR, "wrong number of arguments");
          }

          if (!conn)
              DBLINK_CONN_NOT_AVAIL;

!         /* synchronous query, or async result retrieval */
!         if (!is_async)
!             res = PQexec(conn, sql);
!         else
          {
!             res = PQgetResult(conn);
!             /* NULL means we're all done with the async results */
!             if (!res)
              {
                  MemoryContextSwitchTo(oldcontext);
                  SRF_RETURN_DONE(funcctx);
              }
+         }

!         if (!res ||
!             (PQresultStatus(res) != PGRES_COMMAND_OK &&
!              PQresultStatus(res) != PGRES_TUPLES_OK))
!         {
!             dblink_res_error(conname, res, "could not execute query", fail);
              if (freeconn)
                  PQfinish(conn);
+             MemoryContextSwitchTo(oldcontext);
+             SRF_RETURN_DONE(funcctx);
+         }

!         if (PQresultStatus(res) == PGRES_COMMAND_OK)
!         {
!             is_sql_cmd = true;

!             /* need a tuple descriptor representing one TEXT column */
!             tupdesc = CreateTemplateTupleDesc(1, false);
!             TupleDescInitEntry(tupdesc, (AttrNumber) 1, "status",
!                                TEXTOID, -1, 0);

              /*
!              * and save a copy of the command status string to return as
!              * our result tuple
               */
!             sql_cmd_status = PQcmdStatus(res);
!             funcctx->max_calls = 1;
!         }
!         else
!             funcctx->max_calls = PQntuples(res);

!         /* got results, keep track of them */
!         funcctx->user_fctx = res;
!
!         /* if needed, close the connection to the database and cleanup */
!         if (freeconn)
!             PQfinish(conn);
!
!         if (!is_sql_cmd)
!         {
!             /* get a tuple descriptor for our result type */
!             switch (get_call_result_type(fcinfo, NULL, &tupdesc))
              {
!                 case TYPEFUNC_COMPOSITE:
!                     /* success */
!                     break;
!                 case TYPEFUNC_RECORD:
!                     /* failed to determine actual type of RECORD */
!                     ereport(ERROR,
!                             (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                     errmsg("function returning record called in context "
!                            "that cannot accept type record")));
!                     break;
!                 default:
!                     /* result type isn't composite */
!                     elog(ERROR, "return type must be a row type");
!                     break;
              }

!             /* make sure we have a persistent copy of the tupdesc */
!             tupdesc = CreateTupleDescCopy(tupdesc);
          }
!
!         /*
!          * check result and tuple descriptor have the same number of
!          * columns
!          */
!         if (PQnfields(res) != tupdesc->natts)
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("remote query result rowtype does not match "
!                             "the specified FROM clause rowtype")));
!
!         /* fast track when no results */
!         if (funcctx->max_calls < 1)
          {
!             if (res)
!                 PQclear(res);
              MemoryContextSwitchTo(oldcontext);
!             SRF_RETURN_DONE(funcctx);
          }

!         /* store needed metadata for subsequent calls */
!         attinmeta = TupleDescGetAttInMetadata(tupdesc);
!         funcctx->attinmeta = attinmeta;
!
!         MemoryContextSwitchTo(oldcontext);

      }

***************
*** 2358,2360 ****
--- 2363,2405 ----
           errcontext("Error occurred on dblink connection named \"%s\": %s.",
                      dblink_context_conname, dblink_context_msg)));
  }
+
+ /*
+  * Obtain connection string for a foreign server
+  */
+ static char *
+ get_connect_string(const char *servername)
+ {
+     ForeignServer       *foreign_server;
+     UserMapping           *user_mapping;
+     ListCell           *cell;
+     StringInfo            buf = makeStringInfo();
+
+     /* first gather the server connstr options */
+     foreign_server = GetForeignServerByName(servername, true);
+
+     if (foreign_server)
+     {
+         foreach (cell, foreign_server->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         /* next get the user connstr options */
+         user_mapping = GetUserMapping(GetUserId(), foreign_server->serverid);
+         foreach (cell, user_mapping->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         return pstrdup(buf->data);
+     }
+     else
+         return NULL;
+ }
Index: contrib/dblink/expected/dblink.out
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/expected/dblink.out,v
retrieving revision 1.24
diff -c -r1.24 dblink.out
*** contrib/dblink/expected/dblink.out    3 Jul 2008 03:56:57 -0000    1.24
--- contrib/dblink/expected/dblink.out    3 Jan 2009 02:56:06 -0000
***************
*** 784,786 ****
--- 784,819 ----
   OK
  (1 row)

+ -- test foreign data wrapper functionality
+ CREATE USER dblink_regression_test;
+ CREATE FOREIGN DATA WRAPPER postgresql LIBRARY 'postgresql_fdw' LANGUAGE C;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ SELECT dblink_connect_u('myconn', 'fdtest');
+  dblink_connect_u
+ ------------------
+  OK
+ (1 row)
+
+ SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+  a  | b |       c
+ ----+---+---------------
+   0 | a | {a0,b0,c0}
+   1 | b | {a1,b1,c1}
+   2 | c | {a2,b2,c2}
+   3 | d | {a3,b3,c3}
+   4 | e | {a4,b4,c4}
+   5 | f | {a5,b5,c5}
+   6 | g | {a6,b6,c6}
+   7 | h | {a7,b7,c7}
+   8 | i | {a8,b8,c8}
+   9 | j | {a9,b9,c9}
+  10 | k | {a10,b10,c10}
+ (11 rows)
+
+ \c - :ORIGINAL_USER
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
Index: contrib/dblink/sql/dblink.sql
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/sql/dblink.sql,v
retrieving revision 1.20
diff -c -r1.20 dblink.sql
*** contrib/dblink/sql/dblink.sql    6 Apr 2008 16:54:48 -0000    1.20
--- contrib/dblink/sql/dblink.sql    3 Jan 2009 02:54:29 -0000
***************
*** 364,366 ****
--- 364,383 ----
  SELECT dblink_cancel_query('dtest1');
  SELECT dblink_error_message('dtest1');
  SELECT dblink_disconnect('dtest1');
+
+ -- test foreign data wrapper functionality
+ CREATE USER dblink_regression_test;
+
+ CREATE FOREIGN DATA WRAPPER postgresql LIBRARY 'postgresql_fdw' LANGUAGE C;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ SELECT dblink_connect_u('myconn', 'fdtest');
+ SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+
+ \c - :ORIGINAL_USER
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
Index: doc/src/sgml/dblink.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/dblink.sgml,v
retrieving revision 1.6
diff -c -r1.6 dblink.sgml
*** doc/src/sgml/dblink.sgml    12 Nov 2008 15:52:44 -0000    1.6
--- doc/src/sgml/dblink.sgml    3 Jan 2009 06:25:10 -0000
***************
*** 42,47 ****
--- 42,59 ----
      only one unnamed connection is permitted at a time.  The connection
      will persist until closed or until the database session is ended.
     </para>
+
+    <para>
+     The connection string may also be the name of an existing foreign
+     server that utilizes the postgresql_fdw foreign data wrapper library.
+     See the example below, as well as the following:
+     <simplelist type="inline">
+      <member><xref linkend="sql-createforeigndatawrapper" endterm="sql-createforeigndatawrapper-title"></member>
+      <member><xref linkend="sql-createserver" endterm="sql-createserver-title"></member>
+      <member><xref linkend="sql-createusermapping" endterm="sql-createusermapping-title"></member>
+     </simplelist>
+    </para>
+
    </refsect1>

    <refsect1>
***************
*** 113,118 ****
--- 125,168 ----
   ----------------
    OK
   (1 row)
+
+  -- FOREIGN DATA WRAPPER functionality
+  -- Note: local connection must require authentication for this to work properly
+  CREATE USER dblink_regression_test WITH PASSWORD 'secret';
+  -- The next two statements are unneeded in the contrib_regression database as they
+  -- have already been performed. They will produce ERROR messages.
+  CREATE FOREIGN DATA WRAPPER postgresql LIBRARY 'postgresql_fdw' LANGUAGE C;
+  CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+  CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest OPTIONS (user 'dblink_regression_test', password
'secret');
+  GRANT SELECT ON TABLE foo TO dblink_regression_test;
+  \set ORIGINAL_USER :USER
+  \c - dblink_regression_test
+  SELECT dblink_connect('myconn', 'fdtest');
+   dblink_connect
+  ----------------
+   OK
+  (1 row)
+
+  SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+   a  | b |       c
+  ----+---+---------------
+    0 | a | {a0,b0,c0}
+    1 | b | {a1,b1,c1}
+    2 | c | {a2,b2,c2}
+    3 | d | {a3,b3,c3}
+    4 | e | {a4,b4,c4}
+    5 | f | {a5,b5,c5}
+    6 | g | {a6,b6,c6}
+    7 | h | {a7,b7,c7}
+    8 | i | {a8,b8,c8}
+    9 | j | {a9,b9,c9}
+   10 | k | {a10,b10,c10}
+  (11 rows)
+
+  \c - :ORIGINAL_USER
+  REVOKE SELECT ON TABLE foo FROM dblink_regression_test;
+  DROP USER MAPPING FOR dblink_regression_test SERVER fdtest;
+  DROP USER dblink_regression_test;
     </programlisting>
    </refsect1>
   </refentry>

Re: dblink vs SQL/MED

From
Martin Pihlak
Date:
Joe Conway wrote:
>> Two specific questions on this approach:
>> 1. This implies that the exact same dblink_connstr_check() is performed
>>    on a predefined foreign server and user mapping as a raw connstr --
>>    is this desireable? I'm not entirely clear on the intended purpose
>>    and use of foreign data wrappers yet.
> 
> On the one hand, why be any less stringent on an fdw server than any
> other connect string? But on the other hand, the fdw server definition
> has supposedly been vetted by a superuser. Any thoughts of this?
> 

I'd vote for allowing aribitrary connect strings -- ordinary users cannot
create servers and user mappings unless explicitly granted the privileges.
It probably should be noted in the documentation that allowing ordinary
users to create user mappings enables the use of postgres .pgpass file.
Not sure where to put this at the moment.

>> 2. It seems like get_connect_string() is generically useful to any
>>    client of postgresql_fdw.c -- should it go there instead of dblink?
> 
> I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
> -- objections?
> 

There is some discussion in another thread about this:
http://archives.postgresql.org/pgsql-hackers/2008-12/msg01875.php
http://archives.postgresql.org/pgsql-hackers/2009-01/msg00021.php

The initial approach was to let each foreign data wrapper provide its own
connection string/list builder function. Latest is to provide the lookup
functions in foreign.c, and use the same functions for all the different
fdw's. I was about to implement those but got distracted. Will resume now.

regards,
Martin



dblink vs SQL/MED - security and implementation details

From
Joe Conway
Date:
(changed the subject to hopefully get a few more eyes looking at this 
thread)

Martin Pihlak wrote:
> 
> I'd vote for allowing aribitrary connect strings -- ordinary users cannot
> create servers and user mappings unless explicitly granted the privileges.
> It probably should be noted in the documentation that allowing ordinary
> users to create user mappings enables the use of postgres .pgpass file.
> Not sure where to put this at the moment.

I'm mainly concerned about re-opening security holes that we spent a lot 
of time debating and subsequently closing. I suspect if we assume that 
any FDW-derived connect string can bypass the checks we put in place, we 
will regret it later. But I'm open to arguments on both sides...

>>> 2. It seems like get_connect_string() is generically useful to any
>>>    client of postgresql_fdw.c -- should it go there instead of dblink?
>> I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
>> -- objections?
> 
> There is some discussion in another thread about this:
> http://archives.postgresql.org/pgsql-hackers/2008-12/msg01875.php
> http://archives.postgresql.org/pgsql-hackers/2009-01/msg00021.php
> 
> The initial approach was to let each foreign data wrapper provide its own
> connection string/list builder function. Latest is to provide the lookup
> functions in foreign.c, and use the same functions for all the different
> fdw's. I was about to implement those but got distracted. Will resume now.

It seems to me that get_connect_string() (or whatever we decide to call 
it), is very libpq specific, and therefore belongs with postgresql_fdw.c 
rather than foreign.c. But if we can't reach a consensus there is no 
harm in leaving it as a dblink.c specific static function either.

Joe


Re: dblink vs SQL/MED - security and implementation details

From
Peter Eisentraut
Date:
Joe Conway wrote:
> I'm mainly concerned about re-opening security holes that we spent a lot 
> of time debating and subsequently closing. I suspect if we assume that 
> any FDW-derived connect string can bypass the checks we put in place, we 
> will regret it later. But I'm open to arguments on both sides...

Can you elaborate what security issues you are concerned about?

> It seems to me that get_connect_string() (or whatever we decide to call 
> it), is very libpq specific, and therefore belongs with postgresql_fdw.c 
> rather than foreign.c. But if we can't reach a consensus there is no 
> harm in leaving it as a dblink.c specific static function either.

postgresql_fdw.c is a module with a defined API.  I don't see what you 
would achieve by putting an ad hoc function in there.


Re: dblink vs SQL/MED - security and implementation details

From
Martin Pihlak
Date:
Joe Conway wrote:
> I'm mainly concerned about re-opening security holes that we spent a lot
> of time debating and subsequently closing. I suspect if we assume that
> any FDW-derived connect string can bypass the checks we put in place, we
> will regret it later. But I'm open to arguments on both sides...
> 

In order to create a foreign server, the user needs USAGE on the foreign
data wrapper. Creating user mappings requires the user to be the owner of
the server. Both need explicit grants or alters by the superuser. This is
a bit more relaxed than the current superuser check, but still only trusted
users can define arbitrary connections.

Also, allowing passwordless user mappings adds some flexibility for defining
connections - storing passwords in .pgpass, pgservice or not using a password
at all (pg_hba trust etc.).

regards,
Martin


Re: dblink vs SQL/MED - security and implementation details

From
Joe Conway
Date:
Peter Eisentraut wrote:
> Joe Conway wrote:
>> I'm mainly concerned about re-opening security holes that we spent a 
>> lot of time debating and subsequently closing. I suspect if we assume 
>> that any FDW-derived connect string can bypass the checks we put in 
>> place, we will regret it later. But I'm open to arguments on both 
>> sides...
> 
> Can you elaborate what security issues you are concerned about?

For example: on a freshly installed postgres installation, that does 
*not* require authentication, you would get the following behavior which 
previously was found to be unacceptable:

8<--------------------------------------------------------------
--
-- as the superuser
--
CREATE FOREIGN DATA WRAPPER postgresql LIBRARY 'postgresql_fdw' LANGUAGE C;
CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
CREATE USER dblink_regression_test;
CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest;
\c - dblink_regression_test
psql (8.4devel)
You are now connected to database "contrib_regression" as user 
"dblink_regression_test".
--
-- now as untrusted user dblink_regression_test
--
contrib_regression=> SELECT dblink_connect('myconn', 'fdtest'); dblink_connect
---------------- OK
(1 row)

contrib_regression=> SELECT * FROM dblink('myconn','SELECT 
current_user') AS t(u text);    u
---------- postgres
(1 row)
8<--------------------------------------------------------------

Now, we can blame the superuser for explicitly allowing this, but I 
don't see that as much different from the previously voiced security 
concerns. I'm sure there are other privilege escalation scenarios, but I 
haven't tried at all hard to think of them...

Joe



Re: dblink vs SQL/MED - security and implementation details

From
Peter Eisentraut
Date:
On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:
> --
> -- now as untrusted user dblink_regression_test
> --
> contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
>   dblink_connect
> ----------------
>   OK
> (1 row)

I think you want some permission checking on fdtest then, right?


Re: dblink vs SQL/MED - security and implementation details

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> I think you want some permission checking on fdtest then, right?

What about the permissions on the system catalogs themselves?
AFAICT, the pg_user_mappings view will expose user passwords to
the "owner" of the foreign server, which doesn't seem good.
        regards, tom lane


Re: dblink vs SQL/MED - security and implementation details

From
Peter Eisentraut
Date:
On Tuesday 06 January 2009 19:50:51 Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > I think you want some permission checking on fdtest then, right?
>
> What about the permissions on the system catalogs themselves?
> AFAICT, the pg_user_mappings view will expose user passwords to
> the "owner" of the foreign server, which doesn't seem good.

Well, no one is forcing you to put a password there.  dblink has had its 
mechanisms for obtaining passwords until now, and those are not invalidated 
by this.  There are as always limited use cases for hardcoding passwords, but 
in a fully multiuser environment you probably want to use a different 
authentication mechanism.  Eventually, when we allow these modules to 
actually call out, we will have to seriously evaluate that.  But for right 
now, if you don't want your password in there, don't put it there.


Re: dblink vs SQL/MED - security and implementation details

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> On Tuesday 06 January 2009 19:50:51 Tom Lane wrote:
>> What about the permissions on the system catalogs themselves?
>> AFAICT, the pg_user_mappings view will expose user passwords to
>> the "owner" of the foreign server, which doesn't seem good.

> Well, no one is forcing you to put a password there.  dblink has had its 
> mechanisms for obtaining passwords until now, and those are not invalidated 
> by this.  There are as always limited use cases for hardcoding passwords, but 
> in a fully multiuser environment you probably want to use a different 
> authentication mechanism.  Eventually, when we allow these modules to 
> actually call out, we will have to seriously evaluate that.  But for right 
> now, if you don't want your password in there, don't put it there.

Huh?  The advertised reason for putting in all this stuff was to provide
a thought-through, secure mechanism for dealing with connection
information.  If we haven't done that thinking yet, I'm of the opinion
the whole thing should be ripped out until we have.  It's of exactly
zero value if it cannot be trusted with a password.
        regards, tom lane


Re: dblink vs SQL/MED - security and implementation details

From
Martin Pihlak
Date:
Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> I think you want some permission checking on fdtest then, right?
> 
> What about the permissions on the system catalogs themselves?
> AFAICT, the pg_user_mappings view will expose user passwords to
> the "owner" of the foreign server, which doesn't seem good.
> 

Usually it would have been the server owner who created those user
mappings in the first place -- so the passwords are already known
to him/her. Of course it is possible to create the mappings first
and later change the ownership of the server, thus exposing the
passwords to a new role. But IMHO, it would be reasonable to assume
that the owner of the server has full control over its user mappings.

regards,
Martin




Re: dblink vs SQL/MED - security and implementation details

From
Martin Pihlak
Date:
Peter Eisentraut wrote:
> On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:
>> contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
>>   dblink_connect
>> ----------------
>>   OK
>> (1 row)
> 
> I think you want some permission checking on fdtest then, right?
> 

The proposed "connection lookup" functions have USAGE check on the
server.

About the connstr validation -- it would be best done in the connection
lookup function. IMO it would make sense to validate the connstring if the
foreign server is not OWNED by a superuser. This would enable less trusted
to create and own servers but would force them to provide a username and
password (validate in CreateUserMapping and GetForeignConnectionOptions).
And superuser could still set up a connection that makes use of .pgpass,
pgservice etc. Comments?

regards,
Martin



Re: dblink vs SQL/MED - security and implementation details

From
Joe Conway
Date:
Peter Eisentraut wrote:
> On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:
>> --
>> -- now as untrusted user dblink_regression_test
>> --
>> contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
>>   dblink_connect
>> ----------------
>>   OK
>> (1 row)
> 
> I think you want some permission checking on fdtest then, right?

I don't see anything documented under GRANT which controls privileges on 
a mapping, and the USAGE on a server only controls what a user can see 
by query. I assume that if the superuser creates a mapping from user foo 
to server bar, foo can still use bar via the mapping, even if they don't 
have USAGE granted on the server. It isn't clear from the docs what is 
intended, so I could have that wrong.

But even if foo is granted USAGE on bar, I think you miss the point. If you:

1. grant a non-superuser (foo) access to a server (bar)
2. create a mapping for foo to bar which includes no password
3. configure bar to not require authentication (trust)

you will get the privilege escalation as shown (e.g. foo becomes 
postgres on bar).

Joe



Re: dblink vs SQL/MED - security and implementation details

From
Tom Lane
Date:
Martin Pihlak <martin.pihlak@gmail.com> writes:
> Usually it would have been the server owner who created those user
> mappings in the first place -- so the passwords are already known
> to him/her. Of course it is possible to create the mappings first
> and later change the ownership of the server, thus exposing the
> passwords to a new role. But IMHO, it would be reasonable to assume
> that the owner of the server has full control over its user mappings.

So the DBA should know his users' passwords for remote sites?  That's
not normally considered good security practice.

If the passwords were encrypted strings it might be acceptable, but
without some libpq changes I think they'd have to be cleartext :-(
        regards, tom lane


Re: dblink vs SQL/MED - security and implementation details

From
Peter Eisentraut
Date:
Martin Pihlak wrote:
> Usually it would have been the server owner who created those user
> mappings in the first place -- so the passwords are already known
> to him/her. Of course it is possible to create the mappings first
> and later change the ownership of the server, thus exposing the
> passwords to a new role. But IMHO, it would be reasonable to assume
> that the owner of the server has full control over its user mappings.

Maybe we should rethink that.  In the SQL standard, it says that USAGE 
on the server is required to create a user mapping.  I think we could 
let users create mappings for their own user name if they have USAGE. 
And change the system views so a user can only see his own user mapping 
options.

More generally, using passwords in this way is always going to be a 
mediocre security solution, since the plaintext password will leak in 
all kinds of directions: system catalogs readable by superuser, server 
log readable by adm group members (depends on OS), psql_history, etc.  I 
imagine that a proper authentication solution would involve credentials 
forwarding with either Kerberos or SSL or the like.


Re: dblink vs SQL/MED - security and implementation details

From
Peter Eisentraut
Date:
Joe Conway wrote:
> I don't see anything documented under GRANT which controls privileges on 
> a mapping, and the USAGE on a server only controls what a user can see 
> by query. I assume that if the superuser creates a mapping from user foo 
> to server bar, foo can still use bar via the mapping, even if they don't 
> have USAGE granted on the server. It isn't clear from the docs what is 
> intended, so I could have that wrong.

I think you are misunderstanding some details.  A user mapping does not 
map from a user to a server.  It maps, in the original sense of the 
meaning, a local user to a remote user within the context of a server. 
More generally, it maps a local user to a set of options that are 
necessary to reach the remote server, which would typically include 
remote user name and possibly password.

Regarding the scenario you describe above, a typical use case in a full 
implementation would be:

CREATE SERVER superdb FOREIGN DATA WRAPPER postgresql OPTIONS (host 
'localhost', port '5432', dbname 'mydb');

CREATE USER MAPPING FOR CURRENT_USER SERVER superdb OPTIONS (user 
'guest', password 'sekret');

CREATE FOREIGN TABLE foo SERVER superdb;

Then, when you access table "foo", the foreign-data wrapper would 
(depending on the implementation) connect to a PostgreSQL database using 
the union of the server and the user mapping options for the current 
user.  You could also put the user mapping options "user" and "password" 
into the server definition, if you only want to use one user identity. 
Or put the hostname into each user mapping.  It is just a mechanism to 
separate global and per-user connection options.

To get back to your point, if a superuser created a user mapping for 
user foo.  The permission check in the above example is that CREATE 
FOREIGN TABLE checks whether the current user has USAGE on the server. 
So the user mapping would possibly exist but not be usable.  So there is 
no problem.

dblink more or less takes the place of CREATE FOREIGN TABLE here, so the 
permission check should be more or less the same.

> But even if foo is granted USAGE on bar, I think you miss the point. If 
> you:
> 
> 1. grant a non-superuser (foo) access to a server (bar)
> 2. create a mapping for foo to bar which includes no password
> 3. configure bar to not require authentication (trust)
> 
> you will get the privilege escalation as shown (e.g. foo becomes 
> postgres on bar).

Don't do that then.

You could also:

1. write a function in an untrusted PL that calls out to a different 
database server (bar) (think PL/sh or PL/Perl + psql: it's one line)

2. grant non-superuser (foo) EXECUTE on that function

3. configure bar to not require authentication (trust)

Don't do that either. :-)

But dblink already has its own mechanisms for handling this case, and no 
one is asking you do give this up.  If we actually implement 
foreign-data wrappers, we will have to revisit this, but I don't see any 
need for change now.

Basically, both of the above scenarios are equivalent to granting 
EXECUTE on dblink_connect_u(), which is possible but not recommended in 
normal circumstances.  You just have to be careful about what you grant.