Re: dblink vs SQL/MED - Mailing list pgsql-hackers

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;
+ }