dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
I ran into this today:
select current_database() as current_db \gset
CREATE EXTENSION postgres_fdw;
CREATE EXTENSION
CREATE EXTENSION dblink;
CREATE EXTENSION
CREATE ROLE bob LOGIN PASSWORD 'bob';
CREATE ROLE
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
CREATE SERVER
CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob',
password 'bob' );
CREATE USER MAPPING
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
x
---
1
(1 row)
ALTER SERVER bob_srv OPTIONS (updatable 'true');
ALTER SERVER
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
psql:bug_example.sql:18: ERROR: could not establish connection
DETAIL: invalid connection option "updatable"
Is this something we want to fix?
If so, are there any other fdw/server/user-mapping options that we don't
want to pass along to the connect string?
Steps to re-create:
bug_example.sh:#!/bin/bash
dropdb bug_example
dropuser bob
createdb bug_example
psql bug_example -f bug_example.sql
bug_example.sql:
\set ECHO all
select current_database() as current_db \gset
CREATE EXTENSION postgres_fdw;
CREATE EXTENSION dblink;
CREATE ROLE bob LOGIN PASSWORD 'bob';
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob',
password 'bob' );
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
ALTER SERVER bob_srv OPTIONS (updatable 'true');
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
On Sun, Nov 20, 2016 at 7:37 PM, Corey Huinker <corey.huinker@gmail.com> wrote:
I ran into this today:
select current_database() as current_db \gset
CREATE EXTENSION postgres_fdw;
CREATE EXTENSION
CREATE EXTENSION dblink;
CREATE EXTENSION
CREATE ROLE bob LOGIN PASSWORD 'bob';
CREATE ROLE
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
CREATE SERVER
CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob', password
'bob' );
CREATE USER MAPPING
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
x
---
1
(1 row)ALTER SERVER bob_srv OPTIONS (updatable 'true');
ALTER SERVER
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
psql:bug_example.sql:18: ERROR: could not establish connection
DETAIL: invalid connection option "updatable"Is this something we want to fix?
Looks like a bug to me.
If so, are there any other fdw/server/user-mapping options that we don't
want to pass along to the connect string?
InitPgFdwOptions has an array labeled as /* non-libpq FDW-specific FDW
options */. Presumably all of those should be handled in a common
way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Corey Huinker <corey.huinker@gmail.com> writes:
I ran into this today:
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
...
ALTER SERVER bob_srv OPTIONS (updatable 'true');
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
psql:bug_example.sql:18: ERROR: could not establish connection
DETAIL: invalid connection option "updatable"
Is this something we want to fix?
The dblink docs recommend using dblink_fdw as the FDW for this purpose,
which would only accept legal connstr options. However, I can see the
point of using a postgres_fdw server instead, and considering that
dblink isn't actually enforcing use of any particular FDW type, it seems
like the onus should be on it to be more wary of what the options are.
It looks like this might be fairly easy to fix by having
get_connect_string() use is_valid_dblink_option() to check each
option name, and silently ignore options that are inappropriate.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/21/2016 02:16 PM, Tom Lane wrote:
Corey Huinker <corey.huinker@gmail.com> writes:
I ran into this today:
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
...
ALTER SERVER bob_srv OPTIONS (updatable 'true');
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
psql:bug_example.sql:18: ERROR: could not establish connection
DETAIL: invalid connection option "updatable"Is this something we want to fix?
The dblink docs recommend using dblink_fdw as the FDW for this purpose,
which would only accept legal connstr options. However, I can see the
point of using a postgres_fdw server instead, and considering that
dblink isn't actually enforcing use of any particular FDW type, it seems
like the onus should be on it to be more wary of what the options are.It looks like this might be fairly easy to fix by having
get_connect_string() use is_valid_dblink_option() to check each
option name, and silently ignore options that are inappropriate.
Thanks for the report and analysis. I'll take a look at creating a patch
this week.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
The dblink docs recommend using dblink_fdw as the FDW for this purpose,
which would only accept legal connstr options. However, I can see the
point of using a postgres_fdw server instead, and considering that
dblink isn't actually enforcing use of any particular FDW type, it seems
like the onus should be on it to be more wary of what the options are.
I have to admit, this was the first I'd heard of dblink_fdw. I'm glad it
exists, though. And yes, I'd like to be able to use postgres_fdw entries
because there's value in knowing that the connection for your ad-hoc SQL
exactly matches the connection used for other FDW tables.
It looks like this might be fairly easy to fix by having
get_connect_string() use is_valid_dblink_option() to check each
option name, and silently ignore options that are inappropriate.
From what I can tell, it is very straightforward, the context oids are set
up just a few lines above where the new is_valid_dblink_option() calls
would be.
I'm happy to write the patch, for both v10 and any back-patches we feel are
necessary. However, I suspect with a patch this trivial that reviewing
another person's patch might be more work for a committer than just doing
it themselves. If that's not the case, let me know and I'll get started.
It looks like this might be fairly easy to fix by having
get_connect_string() use is_valid_dblink_option() to check each
option name, and silently ignore options that are inappropriate.From what I can tell, it is very straightforward, the context oids are set
up just a few lines above where the new is_valid_dblink_option() calls
would be.I'm happy to write the patch, for both v10 and any back-patches we feel
are necessary. However, I suspect with a patch this trivial that reviewing
another person's patch might be more work for a committer than just doing
it themselves. If that's not the case, let me know and I'll get started.
Joe indicated that he wouldn't be able to get to the patch until this
weekend at the earliest, so I went ahead and made the patches on my own.
Nothing unusual to report for master, 9.6, 9.5, or 9.3. The patch is
basically the same for all of them and I was able to re-run the test script
at the beginning of the thread to ensure that the fix worked.
In 9.4, I encountered a complaint about flex 2.6.0. After a little research
it seems that a fix for that made it into versions 9.3+, but not 9.4. That
mini-patch is attached as well (0001.configure.94.diff). The dblink patch
for 9.4 was basically the same as the others.
The issue (no validation of connection string elements pulled from an FDW)
exists in 9.2, however, the only possible source of such options I know of
(postgres_fdw) does not. So I doubt we need to patch 9.2, but it's trivial
to do so if we want to.
Attachments:
0001.configure.94.difftext/plain; charset=US-ASCII; name=0001.configure.94.diffDownload
diff --git a/configure b/configure
index 5204869..7da2dac 100755
--- a/configure
+++ b/configure
@@ -7166,7 +7166,7 @@ else
echo '%%' > conftest.l
if $pgac_candidate -t conftest.l 2>/dev/null | grep FLEX_SCANNER >/dev/null 2>&1; then
pgac_flex_version=`$pgac_candidate --version 2>/dev/null`
- if echo "$pgac_flex_version" | sed 's/[.a-z]/ /g' | $AWK '{ if ($1 = 2 && $2 = 5 && $3 >= 31) exit 0; else exit 1;}'
+ if echo "$pgac_flex_version" | sed 's/[.a-z]/ /g' | $AWK '{ if ($1 == 2 && ($2 > 5 || ($2 == 5 && $3 >= 31))) exit 0; else exit 1;}'
then
pgac_cv_path_flex=$pgac_candidate
break 2
0001.dblink_check_connect_options.93.difftext/plain; charset=US-ASCII; name=0001.dblink_check_connect_options.93.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index af062fa..491dec8 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2731,6 +2732,24 @@ get_connect_string(const char *servername)
ForeignDataWrapper *fdw;
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options.
+ *
+ * To avoid unnecessary work, we get the list once and use it throughout
+ * the lifetime of this backend process. We don't need to care about
+ * memory context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq's default connection options")));
+ }
/* first gather the server connstr options */
srvname = pstrdup(servername);
@@ -2755,16 +2774,18 @@ get_connect_string(const char *servername)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->options)
@@ -2772,8 +2793,9 @@ get_connect_string(const char *servername)
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, UserMappingRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
return buf->data;
0001.dblink_check_connect_options.94.difftext/plain; charset=US-ASCII; name=0001.dblink_check_connect_options.94.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 009b877..9f01ef7 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2728,6 +2729,24 @@ get_connect_string(const char *servername)
ForeignDataWrapper *fdw;
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options.
+ *
+ * To avoid unnecessary work, we get the list once and use it throughout
+ * the lifetime of this backend process. We don't need to care about
+ * memory context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq's default connection options")));
+ }
/* first gather the server connstr options */
srvname = pstrdup(servername);
@@ -2752,16 +2771,18 @@ get_connect_string(const char *servername)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->options)
@@ -2769,8 +2790,9 @@ get_connect_string(const char *servername)
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, UserMappingRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
return buf->data;
0001.dblink_check_connect_options.95.difftext/plain; charset=US-ASCII; name=0001.dblink_check_connect_options.95.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index c5892d3..e041381 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2728,6 +2729,24 @@ get_connect_string(const char *servername)
ForeignDataWrapper *fdw;
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options.
+ *
+ * To avoid unnecessary work, we get the list once and use it throughout
+ * the lifetime of this backend process. We don't need to care about
+ * memory context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq's default connection options")));
+ }
/* first gather the server connstr options */
srvname = pstrdup(servername);
@@ -2752,16 +2771,18 @@ get_connect_string(const char *servername)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->options)
@@ -2769,8 +2790,9 @@ get_connect_string(const char *servername)
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, UserMappingRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
return buf->data;
0001.dblink_check_connect_options.96.difftext/plain; charset=US-ASCII; name=0001.dblink_check_connect_options.96.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d4f9090..0d0b848 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2726,6 +2727,24 @@ get_connect_string(const char *servername)
ForeignDataWrapper *fdw;
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options.
+ *
+ * To avoid unnecessary work, we get the list once and use it throughout
+ * the lifetime of this backend process. We don't need to care about
+ * memory context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq's default connection options")));
+ }
/* first gather the server connstr options */
srvname = pstrdup(servername);
@@ -2750,16 +2769,18 @@ get_connect_string(const char *servername)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->options)
@@ -2767,8 +2788,9 @@ get_connect_string(const char *servername)
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, UserMappingRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
return buf->data;
0001.dblink_check_connect_options.master.difftext/plain; charset=US-ASCII; name=0001.dblink_check_connect_options.master.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d4f9090..ac8cdce 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2727,6 +2728,25 @@ get_connect_string(const char *servername)
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options.
+ *
+ * To avoid unnecessary work, we get the list once and use it throughout
+ * the lifetime of this backend process. We don't need to care about
+ * memory context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq's default connection options")));
+ }
+
/* first gather the server connstr options */
srvname = pstrdup(servername);
truncate_identifier(srvname, strlen(srvname), false);
@@ -2750,16 +2770,18 @@ get_connect_string(const char *servername)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->options)
@@ -2767,8 +2789,9 @@ get_connect_string(const char *servername)
DefElem *def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
- escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname, UserMappingRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+ escape_param_str(strVal(def->arg)));
}
return buf->data;
Corey Huinker <corey.huinker@gmail.com> writes:
In 9.4, I encountered a complaint about flex 2.6.0. After a little research
it seems that a fix for that made it into versions 9.3+, but not 9.4.
Er ... what? See 1ba874505.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 22, 2016 at 3:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Corey Huinker <corey.huinker@gmail.com> writes:
In 9.4, I encountered a complaint about flex 2.6.0. After a little
research
it seems that a fix for that made it into versions 9.3+, but not 9.4.
Er ... what? See 1ba874505.
regards, tom lane
Maybe git fetch might didn't pick up that change. Odd that it did pick it
up on all other REL_* branches. Feel free to disregard that file.
I re-ran git pulls from my committed local branches to the corresponding
REL9_x_STABLE branch, and encountered no conflicts, so the 0001.dblink.*
patches should still be good.
On 11/21/2016 03:59 PM, Corey Huinker wrote:
On 11/21/2016 02:16 PM, Tom Lane wrote:
The dblink docs recommend using dblink_fdw as the FDW for this purpose,
which would only accept legal connstr options. However, I can see the
point of using a postgres_fdw server instead, and considering that
dblink isn't actually enforcing use of any particular FDW type, it seems
like the onus should be on it to be more wary of what the options are.
I have to admit, this was the first I'd heard of dblink_fdw. I'm glad it
exists, though. And yes, I'd like to be able to use postgres_fdw entries
because there's value in knowing that the connection for your ad-hoc SQL
exactly matches the connection used for other FDW tables.
I'm happy to write the patch, for both v10 and any back-patches we feel
are necessary.
I looked at Corey's patch, which is straightforward enough, but I was
left wondering if dblink should be allowing any FDW at all (as it does
currently), or should it be limited to dblink_fdw and postgres_fdw? It
doesn't make sense to even try if the FDW does not connect via libpq.
Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
supports a libpq connection it would make sense to allows other FDWs
with this attribute, but since there is none the current state strikes
me as a bad idea.
Thoughts?
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway <mail@joeconway.com> wrote:
Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
supports a libpq connection it would make sense to allows other FDWs
with this attribute, but since there is none the current state strikes
me as a bad idea.Thoughts?
libpq is proper to the implementation of the FDW, not the wrapper on
top of it, so using in the CREATE FDW command a way to do the
decision-making that does not look right to me. Filtering things at
connection attempt is a better solution.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Dec 18, 2016 at 4:57 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway <mail@joeconway.com> wrote:
Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
supports a libpq connection it would make sense to allows other FDWs
with this attribute, but since there is none the current state strikes
me as a bad idea.Thoughts?
libpq is proper to the implementation of the FDW, not the wrapper on
top of it, so using in the CREATE FDW command a way to do the
decision-making that does not look right to me. Filtering things at
connection attempt is a better solution.
--
Michael
The only benefit I see would be giving the user a slightly more clear error
message like ('dblink doesn't work with %', 'mysql') instead of the error
about the failure of the connect string generated by the options that did
overlap.
Gaming out the options of a Wrapper X pointing to server Y:
1. Wrapper X does not have enough overlap in options to accidentally create
a legit connect string:
Connection fails, user gets a message about the incompleteness of the
connection.
2. Wrapper X has enough options (i.e. user+host+dbname,etc) to generate a
legit connect string (with matching port), but server+port pointed to by X
isn't listening or doesn't speak libpq:
Connection fails, user gets an error message.
3. Wrapper X has enough options (i.e. user+host+dbname,etc) to generate a
legit connect string (without matching port), but server+5432 pointed to by
X doesn't speak libpq:
Whatever *is* listening on 5432 has a chance to try to handshake
libpq-style, and I guess there's a chance a hostile service listening on
that port might know enough libpq to siphon off the credentials, but the
creds it would get would be to a pgsql service on Y and Y is already
compromised. Also, that vulnerability would exist for FDWs that do speak
libpq as well.
4. Wrapper X has enough overlap in options to create a legit connect string
which happens to speak libpq:
Connection succeeds, and it's a feature not a bug.
On 12/18/2016 02:47 PM, Corey Huinker wrote:
On Sun, Dec 18, 2016 at 4:57 PM, Michael Paquier wrote:
On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway wrote:
Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
supports a libpq connection it would make sense to allows other FDWs
with this attribute, but since there is none the current state strikes
me as a bad idea.
Thoughts?
libpq is proper to the implementation of the FDW, not the wrapper on
top of it, so using in the CREATE FDW command a way to do the
decision-making that does not look right to me. Filtering things at
connection attempt is a better solution.
The only benefit I see would be giving the user a slightly more clear
error message like ('dblink doesn't work with %', 'mysql') instead of
the error about the failure of the connect string generated by the
options that did overlap.
Ok, I committed Corey's patch with only minor whitespace changes.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development