From: Joe Conway <mail@joeconway.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Hackers (PostgreSQL) <pgsql-hackers@postgresql.org>
Cc: Peter Eisentraut <peter_e@gmx.net>
Subject: Re: dblink patches for comment
Date: Tue, 02 Jun 2009 16:08:18 -0700
Message-ID: <4A25B0E2.3090302@joeconway.com> (raw)
In-Reply-To: <193.1243905503@sss.pgh.pa.us>
References: <4A1C9C8C.6030405@joeconway.com>
<20613.1243466644@sss.pgh.pa.us>
<4A232074.5020506@joeconway.com>
<193.1243905503@sss.pgh.pa.us>
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
Attachments:
[text/x-patch] dblink.2009.06.02.02-sqlmed.diff (7.8K, ../4A25B0E2.3090302@joeconway.com/2-dblink.2009.06.02.02-sqlmed.diff)
download | inline diff:
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;
[text/x-patch] dblink.2009.06.02.02-sqlmed_doc.diff (3.1K, ../4A25B0E2.3090302@joeconway.com/3-dblink.2009.06.02.02-sqlmed_doc.diff)
download | inline diff:
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>
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mail@joeconway.com, tgl@sss.pgh.pa.us, peter_e@gmx.net
Subject: Re: dblink patches for comment
In-Reply-To: <4A25B0E2.3090302@joeconway.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox