[Fwd: Re: dblink patches for comment] - Mailing list pgsql-hackers

Based on Tom's post today about RC1, it sounds like I need to get this
committed very soon. Any complaints?

Joe


-------- Original Message --------
Subject: Re: [HACKERS] dblink patches for comment
Date: Tue, 02 Jun 2009 16:08:18 -0700
From: Joe Conway <mail@joeconway.com>

Tom Lane wrote:
> The docs patch looks okay, except this comment is a bit hazy:
>
>> +  -- Note: local connection must require authentication for this to work properly
>
> I think what it means is
>
>> +  -- Note: local connection must require password authentication for this to work properly
>
> If not, please clarify some other way.  It might also be good to be a
> bit more clear about what "fail to work properly" might entail.

OK, hopefully the attached is more clear.

> As far as the code goes, hopefully Peter will take a look since he's
> spent more time on the SQL/MED code than I have.  The only thing I can
> see that looks bogus is that get_connect_string() is failing to handle
> any quoting/escaping that might be needed for the values to be inserted
> into the connection string.  I don't recall offhand what rules libpq
> has for that, but I hope it at least implements doubled single quotes...

Added quote_literal_cstr() around the connection string params. Also
found I needed to restrict the servername string length to NAMEDATALEN
in order to avoid an assert if a full connection string is passed to
dblink_connect().

Other comments?

Thanks,

Joe


Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.78
diff -c -r1.78 dblink.c
*** dblink.c    2 Jun 2009 03:21:56 -0000    1.78
--- dblink.c    2 Jun 2009 22:55:42 -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,177 ----
              } \
              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 ****
--- 216,222 ----
  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);
--- 227,247 ----

      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);
***************
*** 2353,2355 ****
--- 2365,2426 ----
           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 = NULL;
+     UserMapping           *user_mapping;
+     ListCell           *cell;
+     StringInfo            buf = makeStringInfo();
+     ForeignDataWrapper *fdw;
+     AclResult            aclresult;
+
+     /* first gather the server connstr options */
+     if (strlen(servername) < NAMEDATALEN)
+         foreign_server = GetForeignServerByName(servername, true);
+
+     if (foreign_server)
+     {
+         Oid        serverid = foreign_server->serverid;
+         Oid        fdwid = foreign_server->fdwid;
+         Oid        userid = GetUserId();
+
+         user_mapping = GetUserMapping(userid, serverid);
+         fdw    = GetForeignDataWrapper(fdwid);
+
+         /* Check permissions, user must have usage on the server. */
+         aclresult = pg_foreign_server_aclcheck(serverid, userid, ACL_USAGE);
+         if (aclresult != ACLCHECK_OK)
+             aclcheck_error(aclresult, ACL_KIND_FOREIGN_SERVER, foreign_server->servername);
+
+         foreach (cell, fdw->options)
+         {
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s=%s ", def->defname, quote_literal_cstr(strVal(def->arg)));
+         }
+
+         foreach (cell, foreign_server->options)
+         {
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s=%s ", def->defname, quote_literal_cstr(strVal(def->arg)));
+         }
+
+         foreach (cell, user_mapping->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s=%s ", def->defname, quote_literal_cstr(strVal(def->arg)));
+         }
+
+         return pstrdup(buf->data);
+     }
+     else
+         return NULL;
+ }
Index: 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
*** expected/dblink.out    3 Jul 2008 03:56:57 -0000    1.24
--- expected/dblink.out    2 Jun 2009 16:54:37 -0000
***************
*** 784,786 ****
--- 784,829 ----
   OK
  (1 row)

+ -- test foreign data wrapper functionality
+ CREATE USER dblink_regression_test;
+ CREATE FOREIGN DATA WRAPPER postgresql;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ -- should fail
+ SELECT dblink_connect('myconn', 'fdtest');
+ ERROR:  password is required
+ DETAIL:  Non-superusers must provide a password in the connection string.
+ -- should succeed
+ 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 USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
+ DROP USER MAPPING FOR public SERVER fdtest;
+ DROP SERVER fdtest;
+ DROP FOREIGN DATA WRAPPER postgresql;
Index: 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
*** sql/dblink.sql    6 Apr 2008 16:54:48 -0000    1.20
--- sql/dblink.sql    2 Jun 2009 16:54:37 -0000
***************
*** 364,366 ****
--- 364,391 ----
  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;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ -- should fail
+ SELECT dblink_connect('myconn', 'fdtest');
+ -- should succeed
+ 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 USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
+ DROP USER MAPPING FOR public SERVER fdtest;
+ DROP SERVER fdtest;
+ DROP FOREIGN DATA WRAPPER postgresql;

Index: dblink.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/dblink.sgml,v
retrieving revision 1.6
diff -c -r1.6 dblink.sgml
*** dblink.sgml    12 Nov 2008 15:52:44 -0000    1.6
--- dblink.sgml    2 Jun 2009 22:54:59 -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,177 ----
   ----------------
    OK
   (1 row)
+
+  -- FOREIGN DATA WRAPPER functionality
+  -- Note: local connection must require password authentication for this to work properly
+  --       Otherwise, you will receive the following error from dblink_connect():
+  --       ----------------------------------------------------------------------
+  --       ERROR:  password is required
+  --       DETAIL:  Non-superuser cannot connect if the server does not request a password.
+  --       HINT:  Target server's authentication method must be changed.
+  CREATE USER dblink_regression_test WITH PASSWORD 'secret';
+  CREATE FOREIGN DATA WRAPPER postgresql;
+  CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (hostaddr '127.0.0.1', dbname 'contrib_regression');
+
+  CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest OPTIONS (user 'dblink_regression_test', password
'secret');
+  GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+  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 USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+  REVOKE SELECT ON TABLE foo FROM  dblink_regression_test;
+  DROP USER MAPPING FOR dblink_regression_test SERVER fdtest;
+  DROP USER dblink_regression_test;
+  DROP SERVER fdtest;
+  DROP FOREIGN DATA WRAPPER postgresql;
     </programlisting>
    </refsect1>
   </refentry>


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers