From 8255566f9d479fdfeea09da3141d26afdbb5abe2 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 10 Oct 2012 16:53:08 -0400 Subject: [PATCH] Create an improved FDW option validator function for contrib/dblink. dblink now has its own validator function dblink_fdw_validator(), which is better than the core function postgresql_fdw_validator() because it gets the list of legal options from libpq instead of having a hard-wired list. Make the dblink extension module provide a standard foreign data wrapper dblink_fdw that encapsulates use of this validator, and recommend use of that wrapper instead of making up wrappers on the fly. Unfortunately, because ad-hoc wrappers *were* recommended practice previously, it's not clear when we can get rid of postgresql_fdw_validator without causing upgrade problems. But this is a step in the right direction. Shigeru Hanada, reviewed by KaiGai Kohei --- contrib/dblink/Makefile | 2 +- contrib/dblink/dblink--1.0--1.1.sql | 14 ++ .../{dblink--1.0.sql => dblink--1.1.sql} | 14 +- contrib/dblink/dblink.c | 130 ++++++++++++++++++ contrib/dblink/dblink.control | 2 +- contrib/dblink/dblink.h | 1 + contrib/dblink/expected/dblink.out | 17 ++- contrib/dblink/sql/dblink.sql | 13 +- doc/src/sgml/dblink.sgml | 12 +- 9 files changed, 188 insertions(+), 17 deletions(-) create mode 100644 contrib/dblink/dblink--1.0--1.1.sql rename contrib/dblink/{dblink--1.0.sql => dblink--1.1.sql} (94%) diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile index ac637480eb..a27db88484 100644 --- a/contrib/dblink/Makefile +++ b/contrib/dblink/Makefile @@ -7,7 +7,7 @@ SHLIB_LINK = $(libpq) SHLIB_PREREQS = submake-libpq EXTENSION = dblink -DATA = dblink--1.0.sql dblink--unpackaged--1.0.sql +DATA = dblink--1.1.sql dblink--1.0--1.1.sql dblink--unpackaged--1.0.sql REGRESS = dblink diff --git a/contrib/dblink/dblink--1.0--1.1.sql b/contrib/dblink/dblink--1.0--1.1.sql new file mode 100644 index 0000000000..c964f87a7b --- /dev/null +++ b/contrib/dblink/dblink--1.0--1.1.sql @@ -0,0 +1,14 @@ +/* contrib/dblink/dblink--1.0--1.1.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION dblink UPDATE TO '1.1'" to load this file. \quit + +CREATE FUNCTION dblink_fdw_validator( + options text[], + catalog oid +) +RETURNS void +AS 'MODULE_PATHNAME', 'dblink_fdw_validator' +LANGUAGE C STRICT; + +CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator; diff --git a/contrib/dblink/dblink--1.0.sql b/contrib/dblink/dblink--1.1.sql similarity index 94% rename from contrib/dblink/dblink--1.0.sql rename to contrib/dblink/dblink--1.1.sql index 1fec9e3944..873355382c 100644 --- a/contrib/dblink/dblink--1.0.sql +++ b/contrib/dblink/dblink--1.1.sql @@ -1,4 +1,4 @@ -/* contrib/dblink/dblink--1.0.sql */ +/* contrib/dblink/dblink--1.1.sql */ -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION dblink" to load this file. \quit @@ -221,3 +221,15 @@ CREATE FUNCTION dblink_get_notify( RETURNS setof record AS 'MODULE_PATHNAME', 'dblink_get_notify' LANGUAGE C STRICT; + +/* New stuff in 1.1 begins here */ + +CREATE FUNCTION dblink_fdw_validator( + options text[], + catalog oid +) +RETURNS void +AS 'MODULE_PATHNAME', 'dblink_fdw_validator' +LANGUAGE C STRICT; + +CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator; diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index 9f1dac899e..ceec6ff813 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -37,9 +37,12 @@ #include "libpq-fe.h" #include "access/htup_details.h" +#include "access/reloptions.h" #include "catalog/indexing.h" #include "catalog/namespace.h" +#include "catalog/pg_foreign_server.h" #include "catalog/pg_type.h" +#include "catalog/pg_user_mapping.h" #include "executor/spi.h" #include "foreign/foreign.h" #include "funcapi.h" @@ -113,6 +116,8 @@ static char *escape_param_str(const char *from); static void validate_pkattnums(Relation rel, int2vector *pkattnums_arg, int32 pknumatts_arg, int **pkattnums, int *pknumatts); +static bool is_valid_dblink_option(const PQconninfoOption *options, + const char *option, Oid context); /* Global */ static remoteConn *pconn = NULL; @@ -1912,6 +1917,75 @@ dblink_get_notify(PG_FUNCTION_ARGS) return (Datum) 0; } +/* + * Validate the options given to a dblink foreign server or user mapping. + * Raise an error if any option is invalid. + * + * We just check the names of options here, so semantic errors in options, + * such as invalid numeric format, will be detected at the attempt to connect. + */ +PG_FUNCTION_INFO_V1(dblink_fdw_validator); +Datum +dblink_fdw_validator(PG_FUNCTION_ARGS) +{ + List *options_list = untransformRelOptions(PG_GETARG_DATUM(0)); + Oid context = PG_GETARG_OID(1); + ListCell *cell; + + 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"))); + } + + /* Validate each supplied option. */ + foreach(cell, options_list) + { + DefElem *def = (DefElem *) lfirst(cell); + + if (!is_valid_dblink_option(options, def->defname, context)) + { + /* + * Unknown option, or invalid option for the context specified, + * so complain about it. Provide a hint with list of valid + * options for the context. + */ + StringInfoData buf; + const PQconninfoOption *opt; + + initStringInfo(&buf); + for (opt = options; opt->keyword; opt++) + { + if (is_valid_dblink_option(options, opt->keyword, context)) + appendStringInfo(&buf, "%s%s", + (buf.len > 0) ? ", " : "", + opt->keyword); + } + ereport(ERROR, + (errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND), + errmsg("invalid option \"%s\"", def->defname), + errhint("Valid options in this context are: %s", + buf.data))); + } + } + + PG_RETURN_VOID(); +} + + /************************************************************* * internal functions */ @@ -2768,3 +2842,59 @@ validate_pkattnums(Relation rel, errmsg("invalid attribute number %d", pkattnum))); } } + +/* + * Check if the specified connection option is valid. + * + * We basically allow whatever libpq thinks is an option, with these + * restrictions: + * debug options: disallowed + * "client_encoding": disallowed + * "user": valid only in USER MAPPING options + * secure options (eg password): valid only in USER MAPPING options + * others: valid only in FOREIGN SERVER options + * + * We disallow client_encoding because it would be overridden anyway via + * PQclientEncoding; allowing it to be specified would merely promote + * confusion. + */ +static bool +is_valid_dblink_option(const PQconninfoOption *options, const char *option, + Oid context) +{ + const PQconninfoOption *opt; + + /* Look up the option in libpq result */ + for (opt = options; opt->keyword; opt++) + { + if (strcmp(opt->keyword, option) == 0) + break; + } + if (opt->keyword == NULL) + return false; + + /* Disallow debug options (particularly "replication") */ + if (strchr(opt->dispchar, 'D')) + return false; + + /* Disallow "client_encoding" */ + if (strcmp(opt->keyword, "client_encoding") == 0) + return false; + + /* + * If the option is "user" or marked secure, it should be specified only + * in USER MAPPING. Others should be specified only in SERVER. + */ + if (strcmp(opt->keyword, "user") == 0 || strchr(opt->dispchar, '*')) + { + if (context != UserMappingRelationId) + return false; + } + else + { + if (context != ForeignServerRelationId) + return false; + } + + return true; +} diff --git a/contrib/dblink/dblink.control b/contrib/dblink/dblink.control index 4333a9b618..39f439affc 100644 --- a/contrib/dblink/dblink.control +++ b/contrib/dblink/dblink.control @@ -1,5 +1,5 @@ # dblink extension comment = 'connect to other PostgreSQL databases from within a database' -default_version = '1.0' +default_version = '1.1' module_pathname = '$libdir/dblink' relocatable = true diff --git a/contrib/dblink/dblink.h b/contrib/dblink/dblink.h index 935d283d31..fd11658106 100644 --- a/contrib/dblink/dblink.h +++ b/contrib/dblink/dblink.h @@ -58,5 +58,6 @@ extern Datum dblink_build_sql_delete(PG_FUNCTION_ARGS); extern Datum dblink_build_sql_update(PG_FUNCTION_ARGS); extern Datum dblink_current_query(PG_FUNCTION_ARGS); extern Datum dblink_get_notify(PG_FUNCTION_ARGS); +extern Datum dblink_fdw_validator(PG_FUNCTION_ARGS); #endif /* DBLINK_H */ diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out index a1899ed3c2..974d001be9 100644 --- a/contrib/dblink/expected/dblink.out +++ b/contrib/dblink/expected/dblink.out @@ -783,8 +783,20 @@ 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 SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (invalid 'val'); -- fail, invalid option +ERROR: invalid option "invalid" +HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, application_name, fallback_application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer +CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (password 'val'); -- fail, can't specify password here +ERROR: invalid option "password" +HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, application_name, fallback_application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer +CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (dbname 'contrib_regression'); +CREATE USER MAPPING FOR public SERVER fdtest + OPTIONS (server 'localhost'); -- fail, can't specify server here +ERROR: invalid option "server" +HINT: Valid options in this context are: user, password 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; @@ -823,7 +835,6 @@ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_t DROP USER dblink_regression_test; DROP USER MAPPING FOR public SERVER fdtest; DROP SERVER fdtest; -DROP FOREIGN DATA WRAPPER postgresql; -- test asynchronous notifications SELECT dblink_connect('dbname=contrib_regression'); dblink_connect diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql index 8c8ffe233c..0372af6182 100644 --- a/contrib/dblink/sql/dblink.sql +++ b/contrib/dblink/sql/dblink.sql @@ -361,9 +361,17 @@ 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 SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (invalid 'val'); -- fail, invalid option +CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (password 'val'); -- fail, can't specify password here +CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw + OPTIONS (dbname 'contrib_regression'); + +CREATE USER MAPPING FOR public SERVER fdtest + OPTIONS (server 'localhost'); -- fail, can't specify server here 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; @@ -381,7 +389,6 @@ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_t DROP USER dblink_regression_test; DROP USER MAPPING FOR public SERVER fdtest; DROP SERVER fdtest; -DROP FOREIGN DATA WRAPPER postgresql; -- test asynchronous notifications SELECT dblink_connect('dbname=contrib_regression'); diff --git a/doc/src/sgml/dblink.sgml b/doc/src/sgml/dblink.sgml index 72ca765be7..186ab86586 100644 --- a/doc/src/sgml/dblink.sgml +++ b/doc/src/sgml/dblink.sgml @@ -46,12 +46,10 @@ dblink_connect(text connname, text connstr) returns text The connection string may also be the name of an existing foreign - server. It is recommended to use - the postgresql_fdw_validator when defining - the corresponding foreign-data wrapper. See the example below, as - well as the following: + server. It is recommended to use the foreign-data wrapper + dblink_fdw when defining the corresponding foreign + server. See the example below, as well as the following: - @@ -136,8 +134,7 @@ SELECT dblink_connect('myconn', 'dbname=postgres'); -- 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 VALIDATOR postgresql_fdw_validator; -CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (hostaddr '127.0.0.1', dbname 'contrib_regression'); +CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw 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; @@ -173,7 +170,6 @@ 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;