Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8255566

Browse files
committed
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
1 parent 392b2e5 commit 8255566

File tree

9 files changed

+188
-17
lines changed

9 files changed

+188
-17
lines changed

contrib/dblink/Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ SHLIB_LINK = $(libpq)
77
SHLIB_PREREQS = submake-libpq
88

99
EXTENSION = dblink
10-
DATA = dblink--1.0.sql dblink--unpackaged--1.0.sql
10+
DATA = dblink--1.1.sql dblink--1.0--1.1.sql dblink--unpackaged--1.0.sql
1111

1212
REGRESS = dblink
1313

contrib/dblink/dblink--1.0--1.1.sql

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/* contrib/dblink/dblink--1.0--1.1.sql */
2+
3+
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
4+
\echo Use "ALTER EXTENSION dblink UPDATE TO '1.1'" to load this file. \quit
5+
6+
CREATE FUNCTION dblink_fdw_validator(
7+
options text[],
8+
catalog oid
9+
)
10+
RETURNS void
11+
AS 'MODULE_PATHNAME', 'dblink_fdw_validator'
12+
LANGUAGE C STRICT;
13+
14+
CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;

contrib/dblink/dblink--1.0.sql renamed to contrib/dblink/dblink--1.1.sql

+13-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* contrib/dblink/dblink--1.0.sql */
1+
/* contrib/dblink/dblink--1.1.sql */
22

33
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
44
\echo Use "CREATE EXTENSION dblink" to load this file. \quit
@@ -221,3 +221,15 @@ CREATE FUNCTION dblink_get_notify(
221221
RETURNS setof record
222222
AS 'MODULE_PATHNAME', 'dblink_get_notify'
223223
LANGUAGE C STRICT;
224+
225+
/* New stuff in 1.1 begins here */
226+
227+
CREATE FUNCTION dblink_fdw_validator(
228+
options text[],
229+
catalog oid
230+
)
231+
RETURNS void
232+
AS 'MODULE_PATHNAME', 'dblink_fdw_validator'
233+
LANGUAGE C STRICT;
234+
235+
CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;

contrib/dblink/dblink.c

+130
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,12 @@
3737
#include "libpq-fe.h"
3838

3939
#include "access/htup_details.h"
40+
#include "access/reloptions.h"
4041
#include "catalog/indexing.h"
4142
#include "catalog/namespace.h"
43+
#include "catalog/pg_foreign_server.h"
4244
#include "catalog/pg_type.h"
45+
#include "catalog/pg_user_mapping.h"
4346
#include "executor/spi.h"
4447
#include "foreign/foreign.h"
4548
#include "funcapi.h"
@@ -113,6 +116,8 @@ static char *escape_param_str(const char *from);
113116
static void validate_pkattnums(Relation rel,
114117
int2vector *pkattnums_arg, int32 pknumatts_arg,
115118
int **pkattnums, int *pknumatts);
119+
static bool is_valid_dblink_option(const PQconninfoOption *options,
120+
const char *option, Oid context);
116121

117122
/* Global */
118123
static remoteConn *pconn = NULL;
@@ -1912,6 +1917,75 @@ dblink_get_notify(PG_FUNCTION_ARGS)
19121917
return (Datum) 0;
19131918
}
19141919

1920+
/*
1921+
* Validate the options given to a dblink foreign server or user mapping.
1922+
* Raise an error if any option is invalid.
1923+
*
1924+
* We just check the names of options here, so semantic errors in options,
1925+
* such as invalid numeric format, will be detected at the attempt to connect.
1926+
*/
1927+
PG_FUNCTION_INFO_V1(dblink_fdw_validator);
1928+
Datum
1929+
dblink_fdw_validator(PG_FUNCTION_ARGS)
1930+
{
1931+
List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
1932+
Oid context = PG_GETARG_OID(1);
1933+
ListCell *cell;
1934+
1935+
static const PQconninfoOption *options = NULL;
1936+
1937+
/*
1938+
* Get list of valid libpq options.
1939+
*
1940+
* To avoid unnecessary work, we get the list once and use it throughout
1941+
* the lifetime of this backend process. We don't need to care about
1942+
* memory context issues, because PQconndefaults allocates with malloc.
1943+
*/
1944+
if (!options)
1945+
{
1946+
options = PQconndefaults();
1947+
if (!options) /* assume reason for failure is OOM */
1948+
ereport(ERROR,
1949+
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
1950+
errmsg("out of memory"),
1951+
errdetail("could not get libpq's default connection options")));
1952+
}
1953+
1954+
/* Validate each supplied option. */
1955+
foreach(cell, options_list)
1956+
{
1957+
DefElem *def = (DefElem *) lfirst(cell);
1958+
1959+
if (!is_valid_dblink_option(options, def->defname, context))
1960+
{
1961+
/*
1962+
* Unknown option, or invalid option for the context specified,
1963+
* so complain about it. Provide a hint with list of valid
1964+
* options for the context.
1965+
*/
1966+
StringInfoData buf;
1967+
const PQconninfoOption *opt;
1968+
1969+
initStringInfo(&buf);
1970+
for (opt = options; opt->keyword; opt++)
1971+
{
1972+
if (is_valid_dblink_option(options, opt->keyword, context))
1973+
appendStringInfo(&buf, "%s%s",
1974+
(buf.len > 0) ? ", " : "",
1975+
opt->keyword);
1976+
}
1977+
ereport(ERROR,
1978+
(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
1979+
errmsg("invalid option \"%s\"", def->defname),
1980+
errhint("Valid options in this context are: %s",
1981+
buf.data)));
1982+
}
1983+
}
1984+
1985+
PG_RETURN_VOID();
1986+
}
1987+
1988+
19151989
/*************************************************************
19161990
* internal functions
19171991
*/
@@ -2768,3 +2842,59 @@ validate_pkattnums(Relation rel,
27682842
errmsg("invalid attribute number %d", pkattnum)));
27692843
}
27702844
}
2845+
2846+
/*
2847+
* Check if the specified connection option is valid.
2848+
*
2849+
* We basically allow whatever libpq thinks is an option, with these
2850+
* restrictions:
2851+
* debug options: disallowed
2852+
* "client_encoding": disallowed
2853+
* "user": valid only in USER MAPPING options
2854+
* secure options (eg password): valid only in USER MAPPING options
2855+
* others: valid only in FOREIGN SERVER options
2856+
*
2857+
* We disallow client_encoding because it would be overridden anyway via
2858+
* PQclientEncoding; allowing it to be specified would merely promote
2859+
* confusion.
2860+
*/
2861+
static bool
2862+
is_valid_dblink_option(const PQconninfoOption *options, const char *option,
2863+
Oid context)
2864+
{
2865+
const PQconninfoOption *opt;
2866+
2867+
/* Look up the option in libpq result */
2868+
for (opt = options; opt->keyword; opt++)
2869+
{
2870+
if (strcmp(opt->keyword, option) == 0)
2871+
break;
2872+
}
2873+
if (opt->keyword == NULL)
2874+
return false;
2875+
2876+
/* Disallow debug options (particularly "replication") */
2877+
if (strchr(opt->dispchar, 'D'))
2878+
return false;
2879+
2880+
/* Disallow "client_encoding" */
2881+
if (strcmp(opt->keyword, "client_encoding") == 0)
2882+
return false;
2883+
2884+
/*
2885+
* If the option is "user" or marked secure, it should be specified only
2886+
* in USER MAPPING. Others should be specified only in SERVER.
2887+
*/
2888+
if (strcmp(opt->keyword, "user") == 0 || strchr(opt->dispchar, '*'))
2889+
{
2890+
if (context != UserMappingRelationId)
2891+
return false;
2892+
}
2893+
else
2894+
{
2895+
if (context != ForeignServerRelationId)
2896+
return false;
2897+
}
2898+
2899+
return true;
2900+
}

contrib/dblink/dblink.control

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# dblink extension
22
comment = 'connect to other PostgreSQL databases from within a database'
3-
default_version = '1.0'
3+
default_version = '1.1'
44
module_pathname = '$libdir/dblink'
55
relocatable = true

contrib/dblink/dblink.h

+1
Original file line numberDiff line numberDiff line change
@@ -58,5 +58,6 @@ extern Datum dblink_build_sql_delete(PG_FUNCTION_ARGS);
5858
extern Datum dblink_build_sql_update(PG_FUNCTION_ARGS);
5959
extern Datum dblink_current_query(PG_FUNCTION_ARGS);
6060
extern Datum dblink_get_notify(PG_FUNCTION_ARGS);
61+
extern Datum dblink_fdw_validator(PG_FUNCTION_ARGS);
6162

6263
#endif /* DBLINK_H */

contrib/dblink/expected/dblink.out

+14-3
Original file line numberDiff line numberDiff line change
@@ -783,8 +783,20 @@ SELECT dblink_disconnect('dtest1');
783783

784784
-- test foreign data wrapper functionality
785785
CREATE USER dblink_regression_test;
786-
CREATE FOREIGN DATA WRAPPER postgresql;
787-
CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
786+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
787+
OPTIONS (invalid 'val'); -- fail, invalid option
788+
ERROR: invalid option "invalid"
789+
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
790+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
791+
OPTIONS (password 'val'); -- fail, can't specify password here
792+
ERROR: invalid option "password"
793+
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
794+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
795+
OPTIONS (dbname 'contrib_regression');
796+
CREATE USER MAPPING FOR public SERVER fdtest
797+
OPTIONS (server 'localhost'); -- fail, can't specify server here
798+
ERROR: invalid option "server"
799+
HINT: Valid options in this context are: user, password
788800
CREATE USER MAPPING FOR public SERVER fdtest;
789801
GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
790802
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
823835
DROP USER dblink_regression_test;
824836
DROP USER MAPPING FOR public SERVER fdtest;
825837
DROP SERVER fdtest;
826-
DROP FOREIGN DATA WRAPPER postgresql;
827838
-- test asynchronous notifications
828839
SELECT dblink_connect('dbname=contrib_regression');
829840
dblink_connect

contrib/dblink/sql/dblink.sql

+10-3
Original file line numberDiff line numberDiff line change
@@ -361,9 +361,17 @@ SELECT dblink_disconnect('dtest1');
361361
-- test foreign data wrapper functionality
362362
CREATE USER dblink_regression_test;
363363

364-
CREATE FOREIGN DATA WRAPPER postgresql;
365-
CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
364+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
365+
OPTIONS (invalid 'val'); -- fail, invalid option
366+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
367+
OPTIONS (password 'val'); -- fail, can't specify password here
368+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw
369+
OPTIONS (dbname 'contrib_regression');
370+
371+
CREATE USER MAPPING FOR public SERVER fdtest
372+
OPTIONS (server 'localhost'); -- fail, can't specify server here
366373
CREATE USER MAPPING FOR public SERVER fdtest;
374+
367375
GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
368376
GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
369377

@@ -381,7 +389,6 @@ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_t
381389
DROP USER dblink_regression_test;
382390
DROP USER MAPPING FOR public SERVER fdtest;
383391
DROP SERVER fdtest;
384-
DROP FOREIGN DATA WRAPPER postgresql;
385392

386393
-- test asynchronous notifications
387394
SELECT dblink_connect('dbname=contrib_regression');

doc/src/sgml/dblink.sgml

+4-8
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,10 @@ dblink_connect(text connname, text connstr) returns text
4646

4747
<para>
4848
The connection string may also be the name of an existing foreign
49-
server. It is recommended to use
50-
the <function>postgresql_fdw_validator</function> when defining
51-
the corresponding foreign-data wrapper. See the example below, as
52-
well as the following:
49+
server. It is recommended to use the foreign-data wrapper
50+
<literal>dblink_fdw</literal> when defining the corresponding foreign
51+
server. See the example below, as well as the following:
5352
<simplelist type="inline">
54-
<member><xref linkend="sql-createforeigndatawrapper"></member>
5553
<member><xref linkend="sql-createserver"></member>
5654
<member><xref linkend="sql-createusermapping"></member>
5755
</simplelist>
@@ -136,8 +134,7 @@ SELECT dblink_connect('myconn', 'dbname=postgres');
136134
-- DETAIL: Non-superuser cannot connect if the server does not request a password.
137135
-- HINT: Target server's authentication method must be changed.
138136
CREATE USER dblink_regression_test WITH PASSWORD 'secret';
139-
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
140-
CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (hostaddr '127.0.0.1', dbname 'contrib_regression');
137+
CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (hostaddr '127.0.0.1', dbname 'contrib_regression');
141138

142139
CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest OPTIONS (user 'dblink_regression_test', password 'secret');
143140
GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
@@ -173,7 +170,6 @@ REVOKE SELECT ON TABLE foo FROM dblink_regression_test;
173170
DROP USER MAPPING FOR dblink_regression_test SERVER fdtest;
174171
DROP USER dblink_regression_test;
175172
DROP SERVER fdtest;
176-
DROP FOREIGN DATA WRAPPER postgresql;
177173
</screen>
178174
</refsect1>
179175
</refentry>

0 commit comments

Comments
 (0)