Move postgresql_fdw_validator into dblink
I'd like to propose moving postgresql_fdw_validator into contrib/dblink
as dblink's own validator.
Main purpose of this proposal is to reserve the name "postgresql_fdw"
for concrete FDW for PostgreSQL. I used to use "pgsql_fdw" as the name,
but in previous CF I got comments that full product name is appropriate
rather than abbreviation (e.g. pgsql_fdw) for FDW's name.
In addition, this change would avoid potential problem that third-party
product might use this validator and introduce undesirable dependency
between PG core.
This change breaks backward compatibility, but AFAIK no one except
dblink seems to use this validator, so it would not be serious problem.
# Please let me know if any product uses this validator!
Changes in this patch are:
1) Move postgresql_fdw_validator from core backend to contrib/dblink
with renaming to dblink_fdw_validator. Also I modified this validator
so that it uses PQconndefault() to get libpq's valid options instead of
having its own options list.
2) For ease of use, dblink's CREATE EXTENSION provides default FDW
dblink_fdw which accepts libpq's connection options via user mapping
("user" and secret options such as "password") and foreign server (all
other options).
3) Bump dblink's version to 1.1. Of cource upgrade script is provided.
4) Update documents. (Should we mention removal of
postgresql_fdw_validator?)
5) Use simplified postgresql_fdw_validator in regression test
foreign_data. I didn't change actual test cases because they don't seem
to depend on postgresql_fdw_validator deeply.
Comments and questions are welcome.
--
Shigeru HANADA
Attachments:
dblink_fdw_validator.patchtext/plain; charset=Shift_JIS; name=dblink_fdw_validator.patchDownload
diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile
index ac63748..a27db88 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 0000000..f224d3d
--- /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 IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink--1.0.sql b/contrib/dblink/dblink--1.0.sql
deleted file mode 100644
index 1fec9e3..0000000
--- a/contrib/dblink/dblink--1.0.sql
+++ /dev/null
@@ -1,223 +0,0 @@
-/* contrib/dblink/dblink--1.0.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION dblink" to load this file. \quit
-
--- dblink_connect now restricts non-superusers to password
--- authenticated connections
-CREATE FUNCTION dblink_connect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_connect (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
--- dblink_connect_u allows non-superusers to use
--- non-password authenticated connections, but initially
--- privileges are revoked from public
-CREATE FUNCTION dblink_connect_u (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-CREATE FUNCTION dblink_connect_u (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
-REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
-
-CREATE FUNCTION dblink_disconnect ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_disconnect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text,boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE TYPE dblink_pkey_results AS (position int, colname text);
-
-CREATE FUNCTION dblink_get_pkey (text)
-RETURNS setof dblink_pkey_results
-AS 'MODULE_PATHNAME','dblink_get_pkey'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_insert'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_delete'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_update'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_current_query ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_current_query'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_send_query(text, text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_send_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_is_busy(text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_is_busy'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text, bool)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_connections()
-RETURNS text[]
-AS 'MODULE_PATHNAME', 'dblink_get_connections'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_cancel_query(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_cancel_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_error_message(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_error_message'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- conname TEXT,
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
diff --git a/contrib/dblink/dblink--1.1.sql b/contrib/dblink/dblink--1.1.sql
new file mode 100644
index 0000000..e23b5c9
--- /dev/null
+++ b/contrib/dblink/dblink--1.1.sql
@@ -0,0 +1,234 @@
+/* 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
+
+-- dblink_connect now restricts non-superusers to password
+-- authenticated connections
+CREATE FUNCTION dblink_connect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_connect (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+-- dblink_connect_u allows non-superusers to use
+-- non-password authenticated connections, but initially
+-- privileges are revoked from public
+CREATE FUNCTION dblink_connect_u (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+CREATE FUNCTION dblink_connect_u (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
+CREATE FUNCTION dblink_disconnect ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_disconnect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text,boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE TYPE dblink_pkey_results AS (position int, colname text);
+
+CREATE FUNCTION dblink_get_pkey (text)
+RETURNS setof dblink_pkey_results
+AS 'MODULE_PATHNAME','dblink_get_pkey'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_insert'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_delete'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_update'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_current_query ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_current_query'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_send_query(text, text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_send_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_is_busy(text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_is_busy'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text, bool)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_connections()
+RETURNS text[]
+AS 'MODULE_PATHNAME', 'dblink_get_connections'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_cancel_query(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_cancel_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_error_message(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_error_message'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ conname TEXT,
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+/* stuffs added since 1.1 */
+CREATE FUNCTION dblink_fdw_validator(
+ options text[],
+ catalog oid
+)
+RETURNS void
+AS 'MODULE_PATHNAME', 'dblink_fdw_validator'
+LANGUAGE C IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 9f1dac8..6545cd2 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(PQconninfoOption *options,
+ const char *option, Oid context);
/* Global */
static remoteConn *pconn = NULL;
@@ -1912,6 +1917,76 @@ dblink_get_notify(PG_FUNCTION_ARGS)
return (Datum) 0;
}
+/*
+ * Validate the generic option given to SERVER or USER MAPPING.
+ * Raise an ERROR if the option is considered invalid.
+ *
+ * We just validate the name of options here, so semantic errors of 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;
+ PQconninfoOption *options = NULL;
+
+ /*
+ * Get list of valid libpq options. It contains default values too, but we
+ * don't care the values. Obtained list is allocated with malloc, so we
+ * must free it before leaving this function.
+ */
+ options = PQconndefaults();
+ if (options == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq default connection options")));
+ PG_TRY();
+ {
+ /* Validate each supplied option. */
+ foreach(cell, options_list)
+ {
+ PQconninfoOption *opt;
+ DefElem *def = lfirst(cell);
+ StringInfoData buf;
+
+ if (!is_valid_dblink_option(options, def->defname, context))
+ {
+ /*
+ * Unknown option, or invalid option for the context specified,
+ * complain about it. Provide a hint with list of valid
+ * options for the object.
+ */
+ 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_CATCH();
+ {
+ free(options);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ free(options);
+ PG_RETURN_VOID();
+}
+
/*************************************************************
* internal functions
*/
@@ -2768,3 +2843,52 @@ validate_pkattnums(Relation rel,
errmsg("invalid attribute number %d", pkattnum)));
}
}
+
+/*
+ * Check if the provided option is one of valid dblink options. Valid libpq
+ * conninfo options are classified as below:
+ *
+ * every debug options : invalid in every context
+ * every secure options: valid in only USER MAPPING options
+ * "user" : valid in only USER MAPPING options
+ * Others : valid in both SERVER options and USER MAPPING options
+ *
+ * Note that we accept client_encoding as options, but it is always ignored
+ * because we override it with calling PQclientEncoding to ensure that proper
+ * conversion is used.
+ */
+static bool
+is_valid_dblink_option(PQconninfoOption *options, const char *option,
+ Oid context)
+{
+ PQconninfoOption *opt;
+
+ for (opt = options; opt->keyword; opt++)
+ if (strcmp(opt->keyword, option) == 0)
+ break;
+ if (opt->keyword == NULL)
+ return false;
+
+ /*
+ * All debug options "replication" is used in walreceiver.
+ */
+ if (strcmp(opt->dispchar, "D") == 0)
+ return false;
+
+ /*
+ * If the options is secure one or "user", it should be specified in only
+ * USER MAPPING options. Others should be specified in only SERVER options.
+ */
+ if (strcmp(opt->keyword, "user") == 0 || strcmp(opt->dispchar, "*") == 0)
+ {
+ 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 4333a9b..39f439a 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 935d283..fd11658 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 a1899ed..1cc06e3 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -783,8 +783,16 @@ 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'); -- ERROR
+ERROR: invalid option "invalid"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+ERROR: invalid option "password"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+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 +831,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 8c8ffe2..171ee50 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -361,8 +361,10 @@ 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'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (password 'val'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (dbname 'contrib_regression');
+CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- ERROR
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 +383,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 72ca765..186ab86 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
<para>
The connection string may also be the name of an existing foreign
- server. It is recommended to use
- the <function>postgresql_fdw_validator</function> 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
+ <literal>dblink_fdw</literal> when defining the corresponding foreign
+ server. See the example below, as well as the following:
<simplelist type="inline">
- <member><xref linkend="sql-createforeigndatawrapper"></member>
<member><xref linkend="sql-createserver"></member>
<member><xref linkend="sql-createusermapping"></member>
</simplelist>
@@ -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;
</screen>
</refsect1>
</refentry>
diff --git a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
index 804fb47..d9936e8 100644
--- a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
+++ b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
@@ -121,14 +121,6 @@ CREATE FOREIGN DATA WRAPPER <replaceable class="parameter">name</replaceable>
There is no support for updating a foreign table, and optimization of
queries is primitive (and mostly left to the wrapper, too).
</para>
-
- <para>
- There is one built-in foreign-data wrapper validator function
- provided:
- <filename>postgresql_fdw_validator</filename>, which accepts
- options corresponding to <application>libpq</> connection
- parameters.
- </para>
</refsect1>
<refsect1>
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index d8845aa..8400db4 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -27,7 +27,6 @@
extern Datum pg_options_to_table(PG_FUNCTION_ARGS);
-extern Datum postgresql_fdw_validator(PG_FUNCTION_ARGS);
/*
@@ -434,105 +433,6 @@ pg_options_to_table(PG_FUNCTION_ARGS)
/*
- * Describes the valid options for postgresql FDW, server, and user mapping.
- */
-struct ConnectionOption
-{
- const char *optname;
- Oid optcontext; /* Oid of catalog in which option may appear */
-};
-
-/*
- * Copied from fe-connect.c PQconninfoOptions.
- *
- * The list is small - don't bother with bsearch if it stays so.
- */
-static struct ConnectionOption libpq_conninfo_options[] = {
- {"authtype", ForeignServerRelationId},
- {"service", ForeignServerRelationId},
- {"user", UserMappingRelationId},
- {"password", UserMappingRelationId},
- {"connect_timeout", ForeignServerRelationId},
- {"dbname", ForeignServerRelationId},
- {"host", ForeignServerRelationId},
- {"hostaddr", ForeignServerRelationId},
- {"port", ForeignServerRelationId},
- {"tty", ForeignServerRelationId},
- {"options", ForeignServerRelationId},
- {"requiressl", ForeignServerRelationId},
- {"sslmode", ForeignServerRelationId},
- {"gsslib", ForeignServerRelationId},
- {NULL, InvalidOid}
-};
-
-
-/*
- * Check if the provided option is one of libpq conninfo options.
- * context is the Oid of the catalog the option came from, or 0 if we
- * don't care.
- */
-static bool
-is_conninfo_option(const char *option, Oid context)
-{
- struct ConnectionOption *opt;
-
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (context == opt->optcontext && strcmp(opt->optname, option) == 0)
- return true;
- return false;
-}
-
-
-/*
- * Validate the generic option given to SERVER or USER MAPPING.
- * Raise an ERROR if the option or its value is considered
- * invalid.
- *
- * Valid server options are all libpq conninfo options except
- * user and password -- these may only appear in USER MAPPING options.
- */
-Datum
-postgresql_fdw_validator(PG_FUNCTION_ARGS)
-{
- List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
- Oid catalog = PG_GETARG_OID(1);
-
- ListCell *cell;
-
- foreach(cell, options_list)
- {
- DefElem *def = lfirst(cell);
-
- if (!is_conninfo_option(def->defname, catalog))
- {
- struct ConnectionOption *opt;
- StringInfoData buf;
-
- /*
- * Unknown option specified, complain about it. Provide a hint
- * with list of valid options for the object.
- */
- initStringInfo(&buf);
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (catalog == opt->optcontext)
- appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
- opt->optname);
-
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid option \"%s\"", def->defname),
- errhint("Valid options in this context are: %s",
- buf.data)));
-
- PG_RETURN_BOOL(false);
- }
- }
-
- PG_RETURN_BOOL(true);
-}
-
-
-/*
* get_foreign_data_wrapper_oid - given a FDW name, look up the OID
*
* If missing_ok is false, throw an error if name not found. If true, just
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 77a3b41..67a488e8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3433,9 +3433,6 @@ DESCR("filenode identifier of relation");
DATA(insert OID = 3034 ( pg_relation_filepath PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 25 "2205" _null_ _null_ _null_ _null_ pg_relation_filepath _null_ _null_ _null_ ));
DESCR("file path of relation");
-DATA(insert OID = 2316 ( postgresql_fdw_validator PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 16 "1009 26" _null_ _null_ _null_ _null_ postgresql_fdw_validator _null_ _null_ _null_));
-DESCR("(internal)");
-
DATA(insert OID = 2290 ( record_in PGNSP PGUID 12 1 0 0 0 f f f f t f s 3 0 2249 "2275 26 23" _null_ _null_ _null_ _null_ record_in _null_ _null_ _null_ ));
DESCR("I/O");
DATA(insert OID = 2291 ( record_out PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2249" _null_ _null_ _null_ _null_ record_out _null_ _null_ _null_ ));
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 1bfdca1..0a5b525 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -13,6 +13,40 @@ CREATE ROLE regress_test_role2;
CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -1202,6 +1236,7 @@ DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
NOTICE: drop cascades to server s0
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
-- At this point we should have no wrappers, no servers, and no mappings.
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 38057db..46e358b 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -20,6 +20,41 @@ CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
+
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -497,6 +532,7 @@ DROP ROLE unprivileged_role;
DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
Hanada-san,
What about your plan to upstream contrib/pgsql_fdw module on the upcoming
commit-fest?
Even though I understand the point I noticed (miss-synchronization of sub-
transaction block between local and remote side) is never easy problem to
solve, it is worth to get the patch on the table of discussion.
In my opinion, it is an idea to split-off the transaction control portion as
a limitation of current version. For example, just raise an error when
the foreign-table being referenced in sub-transaction block; with explicit
description in the document.
Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
contrib/pgsql_fdw patch based on this patch.
2012/9/11 Shigeru HANADA <shigeru.hanada@gmail.com>:
I'd like to propose moving postgresql_fdw_validator into contrib/dblink
as dblink's own validator.Main purpose of this proposal is to reserve the name "postgresql_fdw"
for concrete FDW for PostgreSQL. I used to use "pgsql_fdw" as the name,
but in previous CF I got comments that full product name is appropriate
rather than abbreviation (e.g. pgsql_fdw) for FDW's name.In addition, this change would avoid potential problem that third-party
product might use this validator and introduce undesirable dependency
between PG core.This change breaks backward compatibility, but AFAIK no one except
dblink seems to use this validator, so it would not be serious problem.
# Please let me know if any product uses this validator!Changes in this patch are:
1) Move postgresql_fdw_validator from core backend to contrib/dblink
with renaming to dblink_fdw_validator. Also I modified this validator
so that it uses PQconndefault() to get libpq's valid options instead of
having its own options list.2) For ease of use, dblink's CREATE EXTENSION provides default FDW
dblink_fdw which accepts libpq's connection options via user mapping
("user" and secret options such as "password") and foreign server (all
other options).3) Bump dblink's version to 1.1. Of cource upgrade script is provided.
4) Update documents. (Should we mention removal of
postgresql_fdw_validator?)5) Use simplified postgresql_fdw_validator in regression test
foreign_data. I didn't change actual test cases because they don't seem
to depend on postgresql_fdw_validator deeply.Comments and questions are welcome.
--
Shigeru HANADA--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Kaigai-san,
(2012/09/13 16:56), Kohei KaiGai wrote:
What about your plan to upstream contrib/pgsql_fdw module on the upcoming
commit-fest?
I will post pgsql_fdw patch (though it will be renamed to
postgresql_fdw) in opening CF (2012 Sep), as soon as I resolve a bug in
ANALYZE support, maybe on tomorrow. :-)
Even though I understand the point I noticed (miss-synchronization of sub-
transaction block between local and remote side) is never easy problem to
solve, it is worth to get the patch on the table of discussion.
In my opinion, it is an idea to split-off the transaction control portion as
a limitation of current version. For example, just raise an error when
the foreign-table being referenced in sub-transaction block; with explicit
description in the document.
I agree not to support synchronize TX block between remote and local, at
least in next CF (I mean keeping remote TX open until local COMMIT or
ABORT). It would require 2PC and many issues to be solved, so I'd like
to focus fundamental part first. OTOH, using foreign tables in
sub-transaction seems essential to me.
Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
contrib/pgsql_fdw patch based on this patch.
Thanks for your volunteer :-)
Regards,
--
Shigeru HANADA
Hanada-san,
I checked your patch. It can be applied to the latest master branch
without any conflicts, and regression tests were fine.
Unlike the original postgresql_fdw_validator(), the new
dblink_fdw_validator() has wise idea; that pulls list of connection
options from libpq, instead of self-defined static table.
I basically agree not to have multiple source of option list.
+ /*
+ * Get list of valid libpq options. It contains default values too, but we
+ * don't care the values. Obtained list is allocated with malloc, so we
+ * must free it before leaving this function.
+ */
+ options = PQconndefaults();
+ if (options == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq default connection options")));I doubt whether PQconndefaults needs to be invoked for each
validator calls. At least, supported option list of libpq should be
never changed during run-time. So, I think PQconndefaults()
should be called only once at first invocation, then option list
can be stored in static variables or somewhere.
As source code comments says, it is allocated with malloc, thus,
we don't worry about unintentional release. :-)
I don't think other part of this patch is arguable.
Thanks,
2012/9/13 Shigeru HANADA <shigeru.hanada@gmail.com>:
Kaigai-san,
(2012/09/13 16:56), Kohei KaiGai wrote:
What about your plan to upstream contrib/pgsql_fdw module on the upcoming
commit-fest?I will post pgsql_fdw patch (though it will be renamed to
postgresql_fdw) in opening CF (2012 Sep), as soon as I resolve a bug in
ANALYZE support, maybe on tomorrow. :-)Even though I understand the point I noticed (miss-synchronization of sub-
transaction block between local and remote side) is never easy problem to
solve, it is worth to get the patch on the table of discussion.
In my opinion, it is an idea to split-off the transaction control portion as
a limitation of current version. For example, just raise an error when
the foreign-table being referenced in sub-transaction block; with explicit
description in the document.I agree not to support synchronize TX block between remote and local, at
least in next CF (I mean keeping remote TX open until local COMMIT or
ABORT). It would require 2PC and many issues to be solved, so I'd like
to focus fundamental part first. OTOH, using foreign tables in
sub-transaction seems essential to me.Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
contrib/pgsql_fdw patch based on this patch.Thanks for your volunteer :-)
Regards,
--
Shigeru HANADA
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Hanada-san,
The attached patch is a revised one according to my previous
suggestion. It re-defines "PQconninfoOption *options" as static
variable with NULL initial value, then, PQconndefaults() shall
be invoked at once. The default options never changed during
duration of the backend process, so here is no reason why we
allocate and free this object for each validator invocation.
If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.
Thanks,
2012/9/20 Kohei KaiGai <kaigai@kaigai.gr.jp>:
Hanada-san,
I checked your patch. It can be applied to the latest master branch
without any conflicts, and regression tests were fine.Unlike the original postgresql_fdw_validator(), the new
dblink_fdw_validator() has wise idea; that pulls list of connection
options from libpq, instead of self-defined static table.
I basically agree not to have multiple source of option list.+ /* + * Get list of valid libpq options. It contains default values too, but we + * don't care the values. Obtained list is allocated with malloc, so we + * must free it before leaving this function. + */ + options = PQconndefaults(); + if (options == NULL) + ereport(ERROR, + (errcode(ERRCODE_FDW_OUT_OF_MEMORY), + errmsg("out of memory"), + errdetail("could not get libpq default connection options")));I doubt whether PQconndefaults needs to be invoked for each
validator calls. At least, supported option list of libpq should be
never changed during run-time. So, I think PQconndefaults()
should be called only once at first invocation, then option list
can be stored in static variables or somewhere.
As source code comments says, it is allocated with malloc, thus,
we don't worry about unintentional release. :-)I don't think other part of this patch is arguable.
Thanks,
2012/9/13 Shigeru HANADA <shigeru.hanada@gmail.com>:
Kaigai-san,
(2012/09/13 16:56), Kohei KaiGai wrote:
What about your plan to upstream contrib/pgsql_fdw module on the upcoming
commit-fest?I will post pgsql_fdw patch (though it will be renamed to
postgresql_fdw) in opening CF (2012 Sep), as soon as I resolve a bug in
ANALYZE support, maybe on tomorrow. :-)Even though I understand the point I noticed (miss-synchronization of sub-
transaction block between local and remote side) is never easy problem to
solve, it is worth to get the patch on the table of discussion.
In my opinion, it is an idea to split-off the transaction control portion as
a limitation of current version. For example, just raise an error when
the foreign-table being referenced in sub-transaction block; with explicit
description in the document.I agree not to support synchronize TX block between remote and local, at
least in next CF (I mean keeping remote TX open until local COMMIT or
ABORT). It would require 2PC and many issues to be solved, so I'd like
to focus fundamental part first. OTOH, using foreign tables in
sub-transaction seems essential to me.Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
contrib/pgsql_fdw patch based on this patch.Thanks for your volunteer :-)
Regards,
--
Shigeru HANADA--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachments:
dblink_fdw_validator.v2.patchapplication/octet-stream; name=dblink_fdw_validator.v2.patchDownload
contrib/dblink/Makefile | 2 +-
contrib/dblink/dblink--1.0--1.1.sql | 14 ++
contrib/dblink/dblink--1.0.sql | 223 ---------------------
contrib/dblink/dblink--1.1.sql | 234 ++++++++++++++++++++++
contrib/dblink/dblink.c | 117 +++++++++++
contrib/dblink/dblink.control | 2 +-
contrib/dblink/dblink.h | 1 +
contrib/dblink/expected/dblink.out | 13 +-
contrib/dblink/sql/dblink.sql | 7 +-
doc/src/sgml/dblink.sgml | 12 +-
doc/src/sgml/ref/create_foreign_data_wrapper.sgml | 8 -
src/backend/foreign/foreign.c | 100 ---------
src/include/catalog/pg_proc.h | 3 -
src/test/regress/expected/foreign_data.out | 35 ++++
src/test/regress/sql/foreign_data.sql | 36 ++++
15 files changed, 457 insertions(+), 350 deletions(-)
diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile
index ac63748..a27db88 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 0000000..f224d3d
--- /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 IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink--1.0.sql b/contrib/dblink/dblink--1.0.sql
deleted file mode 100644
index 1fec9e3..0000000
--- a/contrib/dblink/dblink--1.0.sql
+++ /dev/null
@@ -1,223 +0,0 @@
-/* contrib/dblink/dblink--1.0.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION dblink" to load this file. \quit
-
--- dblink_connect now restricts non-superusers to password
--- authenticated connections
-CREATE FUNCTION dblink_connect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_connect (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
--- dblink_connect_u allows non-superusers to use
--- non-password authenticated connections, but initially
--- privileges are revoked from public
-CREATE FUNCTION dblink_connect_u (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-CREATE FUNCTION dblink_connect_u (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
-REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
-
-CREATE FUNCTION dblink_disconnect ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_disconnect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text,boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE TYPE dblink_pkey_results AS (position int, colname text);
-
-CREATE FUNCTION dblink_get_pkey (text)
-RETURNS setof dblink_pkey_results
-AS 'MODULE_PATHNAME','dblink_get_pkey'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_insert'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_delete'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_update'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_current_query ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_current_query'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_send_query(text, text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_send_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_is_busy(text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_is_busy'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text, bool)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_connections()
-RETURNS text[]
-AS 'MODULE_PATHNAME', 'dblink_get_connections'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_cancel_query(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_cancel_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_error_message(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_error_message'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- conname TEXT,
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
diff --git a/contrib/dblink/dblink--1.1.sql b/contrib/dblink/dblink--1.1.sql
new file mode 100644
index 0000000..e23b5c9
--- /dev/null
+++ b/contrib/dblink/dblink--1.1.sql
@@ -0,0 +1,234 @@
+/* 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
+
+-- dblink_connect now restricts non-superusers to password
+-- authenticated connections
+CREATE FUNCTION dblink_connect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_connect (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+-- dblink_connect_u allows non-superusers to use
+-- non-password authenticated connections, but initially
+-- privileges are revoked from public
+CREATE FUNCTION dblink_connect_u (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+CREATE FUNCTION dblink_connect_u (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
+CREATE FUNCTION dblink_disconnect ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_disconnect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text,boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE TYPE dblink_pkey_results AS (position int, colname text);
+
+CREATE FUNCTION dblink_get_pkey (text)
+RETURNS setof dblink_pkey_results
+AS 'MODULE_PATHNAME','dblink_get_pkey'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_insert'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_delete'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_update'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_current_query ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_current_query'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_send_query(text, text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_send_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_is_busy(text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_is_busy'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text, bool)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_connections()
+RETURNS text[]
+AS 'MODULE_PATHNAME', 'dblink_get_connections'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_cancel_query(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_cancel_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_error_message(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_error_message'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ conname TEXT,
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+/* stuffs added since 1.1 */
+CREATE FUNCTION dblink_fdw_validator(
+ options text[],
+ catalog oid
+)
+RETURNS void
+AS 'MODULE_PATHNAME', 'dblink_fdw_validator'
+LANGUAGE C IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 9f1dac8..ed6385b 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(PQconninfoOption *options,
+ const char *option, Oid context);
/* Global */
static remoteConn *pconn = NULL;
@@ -1912,6 +1917,69 @@ dblink_get_notify(PG_FUNCTION_ARGS)
return (Datum) 0;
}
+/*
+ * Validate the generic option given to SERVER or USER MAPPING.
+ * Raise an ERROR if the option is considered invalid.
+ *
+ * We just validate the name of options here, so semantic errors of 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)
+{
+ static PQconninfoOption *options = NULL;
+ List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
+ Oid context = PG_GETARG_OID(1);
+ ListCell *cell;
+
+ /*
+ * Get list of valid libpq options. It contains default values too, but we
+ * don't care the values. Obtained list is allocated with malloc, so we
+ * must free it before leaving this function.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options)
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq default connection options")));
+ }
+
+ /* Validate each supplied option. */
+ foreach(cell, options_list)
+ {
+ PQconninfoOption *opt;
+ DefElem *def = lfirst(cell);
+ StringInfoData buf;
+
+ if (!is_valid_dblink_option(options, def->defname, context))
+ {
+ /*
+ * Unknown option, or invalid option for the context specified,
+ * complain about it. Provide a hint with list of valid
+ * options for the object.
+ */
+ 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 +2836,52 @@ validate_pkattnums(Relation rel,
errmsg("invalid attribute number %d", pkattnum)));
}
}
+
+/*
+ * Check if the provided option is one of valid dblink options. Valid libpq
+ * conninfo options are classified as below:
+ *
+ * every debug options : invalid in every context
+ * every secure options: valid in only USER MAPPING options
+ * "user" : valid in only USER MAPPING options
+ * Others : valid in both SERVER options and USER MAPPING options
+ *
+ * Note that we accept client_encoding as options, but it is always ignored
+ * because we override it with calling PQclientEncoding to ensure that proper
+ * conversion is used.
+ */
+static bool
+is_valid_dblink_option(PQconninfoOption *options, const char *option,
+ Oid context)
+{
+ PQconninfoOption *opt;
+
+ for (opt = options; opt->keyword; opt++)
+ if (strcmp(opt->keyword, option) == 0)
+ break;
+ if (opt->keyword == NULL)
+ return false;
+
+ /*
+ * All debug options "replication" is used in walreceiver.
+ */
+ if (strcmp(opt->dispchar, "D") == 0)
+ return false;
+
+ /*
+ * If the options is secure one or "user", it should be specified in only
+ * USER MAPPING options. Others should be specified in only SERVER options.
+ */
+ if (strcmp(opt->keyword, "user") == 0 || strcmp(opt->dispchar, "*") == 0)
+ {
+ 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 4333a9b..39f439a 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 935d283..fd11658 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 a1899ed..1cc06e3 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -783,8 +783,16 @@ 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'); -- ERROR
+ERROR: invalid option "invalid"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+ERROR: invalid option "password"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+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 +831,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 8c8ffe2..171ee50 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -361,8 +361,10 @@ 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'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (password 'val'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (dbname 'contrib_regression');
+CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- ERROR
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 +383,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 72ca765..186ab86 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
<para>
The connection string may also be the name of an existing foreign
- server. It is recommended to use
- the <function>postgresql_fdw_validator</function> 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
+ <literal>dblink_fdw</literal> when defining the corresponding foreign
+ server. See the example below, as well as the following:
<simplelist type="inline">
- <member><xref linkend="sql-createforeigndatawrapper"></member>
<member><xref linkend="sql-createserver"></member>
<member><xref linkend="sql-createusermapping"></member>
</simplelist>
@@ -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;
</screen>
</refsect1>
</refentry>
diff --git a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
index 804fb47..d9936e8 100644
--- a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
+++ b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
@@ -121,14 +121,6 @@ CREATE FOREIGN DATA WRAPPER <replaceable class="parameter">name</replaceable>
There is no support for updating a foreign table, and optimization of
queries is primitive (and mostly left to the wrapper, too).
</para>
-
- <para>
- There is one built-in foreign-data wrapper validator function
- provided:
- <filename>postgresql_fdw_validator</filename>, which accepts
- options corresponding to <application>libpq</> connection
- parameters.
- </para>
</refsect1>
<refsect1>
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index d8845aa..8400db4 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -27,7 +27,6 @@
extern Datum pg_options_to_table(PG_FUNCTION_ARGS);
-extern Datum postgresql_fdw_validator(PG_FUNCTION_ARGS);
/*
@@ -434,105 +433,6 @@ pg_options_to_table(PG_FUNCTION_ARGS)
/*
- * Describes the valid options for postgresql FDW, server, and user mapping.
- */
-struct ConnectionOption
-{
- const char *optname;
- Oid optcontext; /* Oid of catalog in which option may appear */
-};
-
-/*
- * Copied from fe-connect.c PQconninfoOptions.
- *
- * The list is small - don't bother with bsearch if it stays so.
- */
-static struct ConnectionOption libpq_conninfo_options[] = {
- {"authtype", ForeignServerRelationId},
- {"service", ForeignServerRelationId},
- {"user", UserMappingRelationId},
- {"password", UserMappingRelationId},
- {"connect_timeout", ForeignServerRelationId},
- {"dbname", ForeignServerRelationId},
- {"host", ForeignServerRelationId},
- {"hostaddr", ForeignServerRelationId},
- {"port", ForeignServerRelationId},
- {"tty", ForeignServerRelationId},
- {"options", ForeignServerRelationId},
- {"requiressl", ForeignServerRelationId},
- {"sslmode", ForeignServerRelationId},
- {"gsslib", ForeignServerRelationId},
- {NULL, InvalidOid}
-};
-
-
-/*
- * Check if the provided option is one of libpq conninfo options.
- * context is the Oid of the catalog the option came from, or 0 if we
- * don't care.
- */
-static bool
-is_conninfo_option(const char *option, Oid context)
-{
- struct ConnectionOption *opt;
-
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (context == opt->optcontext && strcmp(opt->optname, option) == 0)
- return true;
- return false;
-}
-
-
-/*
- * Validate the generic option given to SERVER or USER MAPPING.
- * Raise an ERROR if the option or its value is considered
- * invalid.
- *
- * Valid server options are all libpq conninfo options except
- * user and password -- these may only appear in USER MAPPING options.
- */
-Datum
-postgresql_fdw_validator(PG_FUNCTION_ARGS)
-{
- List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
- Oid catalog = PG_GETARG_OID(1);
-
- ListCell *cell;
-
- foreach(cell, options_list)
- {
- DefElem *def = lfirst(cell);
-
- if (!is_conninfo_option(def->defname, catalog))
- {
- struct ConnectionOption *opt;
- StringInfoData buf;
-
- /*
- * Unknown option specified, complain about it. Provide a hint
- * with list of valid options for the object.
- */
- initStringInfo(&buf);
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (catalog == opt->optcontext)
- appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
- opt->optname);
-
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid option \"%s\"", def->defname),
- errhint("Valid options in this context are: %s",
- buf.data)));
-
- PG_RETURN_BOOL(false);
- }
- }
-
- PG_RETURN_BOOL(true);
-}
-
-
-/*
* get_foreign_data_wrapper_oid - given a FDW name, look up the OID
*
* If missing_ok is false, throw an error if name not found. If true, just
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index a2da836..8357198 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3439,9 +3439,6 @@ DESCR("filenode identifier of relation");
DATA(insert OID = 3034 ( pg_relation_filepath PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 25 "2205" _null_ _null_ _null_ _null_ pg_relation_filepath _null_ _null_ _null_ ));
DESCR("file path of relation");
-DATA(insert OID = 2316 ( postgresql_fdw_validator PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 16 "1009 26" _null_ _null_ _null_ _null_ postgresql_fdw_validator _null_ _null_ _null_));
-DESCR("(internal)");
-
DATA(insert OID = 2290 ( record_in PGNSP PGUID 12 1 0 0 0 f f f f t f s 3 0 2249 "2275 26 23" _null_ _null_ _null_ _null_ record_in _null_ _null_ _null_ ));
DESCR("I/O");
DATA(insert OID = 2291 ( record_out PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2249" _null_ _null_ _null_ _null_ record_out _null_ _null_ _null_ ));
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 1bfdca1..0a5b525 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -13,6 +13,40 @@ CREATE ROLE regress_test_role2;
CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -1202,6 +1236,7 @@ DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
NOTICE: drop cascades to server s0
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
-- At this point we should have no wrappers, no servers, and no mappings.
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 38057db..46e358b 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -20,6 +20,41 @@ CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
+
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -497,6 +532,7 @@ DROP ROLE unprivileged_role;
DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
(2012/10/09 0:30), Kohei KaiGai wrote:
The attached patch is a revised one according to my previous
suggestion. It re-defines "PQconninfoOption *options" as static
variable with NULL initial value, then, PQconndefaults() shall
be invoked at once. The default options never changed during
duration of the backend process, so here is no reason why we
allocate and free this object for each validator invocation.
Sorry for delayed response. It seems reasonable, so I just fixed
obsolete comment and indent. Please see attached v3 patch which was
rebased against latest head of master.
If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.
Please go ahead. :-)
Regards,
--
Shigeru HANADA
Attachments:
dblink_fdw_validator.v3.patchtext/plain; charset=UTF-8; name=dblink_fdw_validator.v3.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile
index ac63748..a27db88 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 0000000..f224d3d
--- /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 IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink--1.0.sql b/contrib/dblink/dblink--1.0.sql
deleted file mode 100644
index 1fec9e3..0000000
--- a/contrib/dblink/dblink--1.0.sql
+++ /dev/null
@@ -1,223 +0,0 @@
-/* contrib/dblink/dblink--1.0.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION dblink" to load this file. \quit
-
--- dblink_connect now restricts non-superusers to password
--- authenticated connections
-CREATE FUNCTION dblink_connect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_connect (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT;
-
--- dblink_connect_u allows non-superusers to use
--- non-password authenticated connections, but initially
--- privileges are revoked from public
-CREATE FUNCTION dblink_connect_u (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-CREATE FUNCTION dblink_connect_u (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_connect'
-LANGUAGE C STRICT SECURITY DEFINER;
-
-REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
-REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
-
-CREATE FUNCTION dblink_disconnect ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_disconnect (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_disconnect'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_open (text, text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_open'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_fetch (text, text, int, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_fetch'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_close (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_close'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink (text, boolean)
-RETURNS setof record
-AS 'MODULE_PATHNAME','dblink_record'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text, text, boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_exec (text,boolean)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_exec'
-LANGUAGE C STRICT;
-
-CREATE TYPE dblink_pkey_results AS (position int, colname text);
-
-CREATE FUNCTION dblink_get_pkey (text)
-RETURNS setof dblink_pkey_results
-AS 'MODULE_PATHNAME','dblink_get_pkey'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_insert'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_delete'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_build_sql_update'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_current_query ()
-RETURNS text
-AS 'MODULE_PATHNAME','dblink_current_query'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_send_query(text, text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_send_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_is_busy(text)
-RETURNS int4
-AS 'MODULE_PATHNAME', 'dblink_is_busy'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_result(text, bool)
-RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'dblink_get_result'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_connections()
-RETURNS text[]
-AS 'MODULE_PATHNAME', 'dblink_get_connections'
-LANGUAGE C;
-
-CREATE FUNCTION dblink_cancel_query(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_cancel_query'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_error_message(text)
-RETURNS text
-AS 'MODULE_PATHNAME', 'dblink_error_message'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION dblink_get_notify(
- conname TEXT,
- OUT notify_name TEXT,
- OUT be_pid INT4,
- OUT extra TEXT
-)
-RETURNS setof record
-AS 'MODULE_PATHNAME', 'dblink_get_notify'
-LANGUAGE C STRICT;
diff --git a/contrib/dblink/dblink--1.1.sql b/contrib/dblink/dblink--1.1.sql
new file mode 100644
index 0000000..e23b5c9
--- /dev/null
+++ b/contrib/dblink/dblink--1.1.sql
@@ -0,0 +1,234 @@
+/* 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
+
+-- dblink_connect now restricts non-superusers to password
+-- authenticated connections
+CREATE FUNCTION dblink_connect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_connect (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT;
+
+-- dblink_connect_u allows non-superusers to use
+-- non-password authenticated connections, but initially
+-- privileges are revoked from public
+CREATE FUNCTION dblink_connect_u (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+CREATE FUNCTION dblink_connect_u (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_connect'
+LANGUAGE C STRICT SECURITY DEFINER;
+
+REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
+CREATE FUNCTION dblink_disconnect ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_disconnect (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_disconnect'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_open (text, text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_open'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_fetch (text, text, int, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_fetch'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_close (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_close'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink (text, boolean)
+RETURNS setof record
+AS 'MODULE_PATHNAME','dblink_record'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_exec (text,boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_exec'
+LANGUAGE C STRICT;
+
+CREATE TYPE dblink_pkey_results AS (position int, colname text);
+
+CREATE FUNCTION dblink_get_pkey (text)
+RETURNS setof dblink_pkey_results
+AS 'MODULE_PATHNAME','dblink_get_pkey'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_insert (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_insert'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_delete (text, int2vector, int, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_delete'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_build_sql_update (text, int2vector, int, _text, _text)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_build_sql_update'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_current_query ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_current_query'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_send_query(text, text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_send_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_is_busy(text)
+RETURNS int4
+AS 'MODULE_PATHNAME', 'dblink_is_busy'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_result(text, bool)
+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'dblink_get_result'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_connections()
+RETURNS text[]
+AS 'MODULE_PATHNAME', 'dblink_get_connections'
+LANGUAGE C;
+
+CREATE FUNCTION dblink_cancel_query(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_cancel_query'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_error_message(text)
+RETURNS text
+AS 'MODULE_PATHNAME', 'dblink_error_message'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION dblink_get_notify(
+ conname TEXT,
+ OUT notify_name TEXT,
+ OUT be_pid INT4,
+ OUT extra TEXT
+)
+RETURNS setof record
+AS 'MODULE_PATHNAME', 'dblink_get_notify'
+LANGUAGE C STRICT;
+
+/* stuffs added since 1.1 */
+CREATE FUNCTION dblink_fdw_validator(
+ options text[],
+ catalog oid
+)
+RETURNS void
+AS 'MODULE_PATHNAME', 'dblink_fdw_validator'
+LANGUAGE C IMMUTABLE;
+
+CREATE FOREIGN DATA WRAPPER dblink_fdw VALIDATOR dblink_fdw_validator;
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 9f1dac8..fbca38e 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(PQconninfoOption *options,
+ const char *option, Oid context);
/* Global */
static remoteConn *pconn = NULL;
@@ -1912,6 +1917,72 @@ dblink_get_notify(PG_FUNCTION_ARGS)
return (Datum) 0;
}
+/*
+ * Validate the generic option given to SERVER or USER MAPPING.
+ * Raise an ERROR if the option is considered invalid.
+ *
+ * We just validate the name of options here, so semantic errors of 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)
+{
+ static PQconninfoOption *options = NULL;
+ List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
+ Oid context = PG_GETARG_OID(1);
+ ListCell *cell;
+
+ /*
+ * Get list of valid libpq options. The list contains default values too,
+ * but we don't care the values.
+ *
+ * To avoid unnecessary memory allocation, obtained list is used through
+ * the lifetime of this backend process. We don't need to care memory
+ * context issues, because PQconndefaults allocates with malloc.
+ */
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options)
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("could not get libpq default connection options")));
+ }
+
+ /* Validate each supplied option. */
+ foreach(cell, options_list)
+ {
+ PQconninfoOption *opt;
+ DefElem *def = lfirst(cell);
+ StringInfoData buf;
+
+ if (!is_valid_dblink_option(options, def->defname, context))
+ {
+ /*
+ * Unknown option, or invalid option for the context specified,
+ * complain about it. Provide a hint with list of valid
+ * options for the object.
+ */
+ 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 +2839,52 @@ validate_pkattnums(Relation rel,
errmsg("invalid attribute number %d", pkattnum)));
}
}
+
+/*
+ * Check if the provided option is one of valid dblink options. Valid libpq
+ * conninfo options are classified as below:
+ *
+ * every debug options : invalid in every context
+ * every secure options: valid in only USER MAPPING options
+ * "user" : valid in only USER MAPPING options
+ * Others : valid in both SERVER options and USER MAPPING options
+ *
+ * Note that we accept client_encoding as options, but it is always ignored
+ * because we override it with calling PQclientEncoding to ensure that proper
+ * conversion is used.
+ */
+static bool
+is_valid_dblink_option(PQconninfoOption *options, const char *option,
+ Oid context)
+{
+ PQconninfoOption *opt;
+
+ for (opt = options; opt->keyword; opt++)
+ if (strcmp(opt->keyword, option) == 0)
+ break;
+ if (opt->keyword == NULL)
+ return false;
+
+ /*
+ * All debug options "replication" is used in walreceiver.
+ */
+ if (strcmp(opt->dispchar, "D") == 0)
+ return false;
+
+ /*
+ * If the options is secure one or "user", it should be specified in only
+ * USER MAPPING options. Others should be specified in only SERVER options.
+ */
+ if (strcmp(opt->keyword, "user") == 0 || strcmp(opt->dispchar, "*") == 0)
+ {
+ 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 4333a9b..39f439a 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 935d283..fd11658 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 a1899ed..1cc06e3 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -783,8 +783,16 @@ 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'); -- ERROR
+ERROR: invalid option "invalid"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+ERROR: invalid option "password"
+HINT: Valid options in this context are: service, connect_timeout, dbname, host, hostaddr, port, client_encoding, 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'); -- ERROR
+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 +831,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 8c8ffe2..171ee50 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -361,8 +361,10 @@ 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'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (password 'val'); -- ERROR
+CREATE SERVER fdtest FOREIGN DATA WRAPPER dblink_fdw OPTIONS (dbname 'contrib_regression');
+CREATE USER MAPPING FOR public SERVER fdtest OPTIONS (server 'localhost'); -- ERROR
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 +383,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 72ca765..186ab86 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
<para>
The connection string may also be the name of an existing foreign
- server. It is recommended to use
- the <function>postgresql_fdw_validator</function> 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
+ <literal>dblink_fdw</literal> when defining the corresponding foreign
+ server. See the example below, as well as the following:
<simplelist type="inline">
- <member><xref linkend="sql-createforeigndatawrapper"></member>
<member><xref linkend="sql-createserver"></member>
<member><xref linkend="sql-createusermapping"></member>
</simplelist>
@@ -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;
</screen>
</refsect1>
</refentry>
diff --git a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
index 804fb47..d9936e8 100644
--- a/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
+++ b/doc/src/sgml/ref/create_foreign_data_wrapper.sgml
@@ -121,14 +121,6 @@ CREATE FOREIGN DATA WRAPPER <replaceable class="parameter">name</replaceable>
There is no support for updating a foreign table, and optimization of
queries is primitive (and mostly left to the wrapper, too).
</para>
-
- <para>
- There is one built-in foreign-data wrapper validator function
- provided:
- <filename>postgresql_fdw_validator</filename>, which accepts
- options corresponding to <application>libpq</> connection
- parameters.
- </para>
</refsect1>
<refsect1>
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index d8845aa..8400db4 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -27,7 +27,6 @@
extern Datum pg_options_to_table(PG_FUNCTION_ARGS);
-extern Datum postgresql_fdw_validator(PG_FUNCTION_ARGS);
/*
@@ -434,105 +433,6 @@ pg_options_to_table(PG_FUNCTION_ARGS)
/*
- * Describes the valid options for postgresql FDW, server, and user mapping.
- */
-struct ConnectionOption
-{
- const char *optname;
- Oid optcontext; /* Oid of catalog in which option may appear */
-};
-
-/*
- * Copied from fe-connect.c PQconninfoOptions.
- *
- * The list is small - don't bother with bsearch if it stays so.
- */
-static struct ConnectionOption libpq_conninfo_options[] = {
- {"authtype", ForeignServerRelationId},
- {"service", ForeignServerRelationId},
- {"user", UserMappingRelationId},
- {"password", UserMappingRelationId},
- {"connect_timeout", ForeignServerRelationId},
- {"dbname", ForeignServerRelationId},
- {"host", ForeignServerRelationId},
- {"hostaddr", ForeignServerRelationId},
- {"port", ForeignServerRelationId},
- {"tty", ForeignServerRelationId},
- {"options", ForeignServerRelationId},
- {"requiressl", ForeignServerRelationId},
- {"sslmode", ForeignServerRelationId},
- {"gsslib", ForeignServerRelationId},
- {NULL, InvalidOid}
-};
-
-
-/*
- * Check if the provided option is one of libpq conninfo options.
- * context is the Oid of the catalog the option came from, or 0 if we
- * don't care.
- */
-static bool
-is_conninfo_option(const char *option, Oid context)
-{
- struct ConnectionOption *opt;
-
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (context == opt->optcontext && strcmp(opt->optname, option) == 0)
- return true;
- return false;
-}
-
-
-/*
- * Validate the generic option given to SERVER or USER MAPPING.
- * Raise an ERROR if the option or its value is considered
- * invalid.
- *
- * Valid server options are all libpq conninfo options except
- * user and password -- these may only appear in USER MAPPING options.
- */
-Datum
-postgresql_fdw_validator(PG_FUNCTION_ARGS)
-{
- List *options_list = untransformRelOptions(PG_GETARG_DATUM(0));
- Oid catalog = PG_GETARG_OID(1);
-
- ListCell *cell;
-
- foreach(cell, options_list)
- {
- DefElem *def = lfirst(cell);
-
- if (!is_conninfo_option(def->defname, catalog))
- {
- struct ConnectionOption *opt;
- StringInfoData buf;
-
- /*
- * Unknown option specified, complain about it. Provide a hint
- * with list of valid options for the object.
- */
- initStringInfo(&buf);
- for (opt = libpq_conninfo_options; opt->optname; opt++)
- if (catalog == opt->optcontext)
- appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
- opt->optname);
-
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid option \"%s\"", def->defname),
- errhint("Valid options in this context are: %s",
- buf.data)));
-
- PG_RETURN_BOOL(false);
- }
- }
-
- PG_RETURN_BOOL(true);
-}
-
-
-/*
* get_foreign_data_wrapper_oid - given a FDW name, look up the OID
*
* If missing_ok is false, throw an error if name not found. If true, just
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 77a3b41..67a488e8 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3433,9 +3433,6 @@ DESCR("filenode identifier of relation");
DATA(insert OID = 3034 ( pg_relation_filepath PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 25 "2205" _null_ _null_ _null_ _null_ pg_relation_filepath _null_ _null_ _null_ ));
DESCR("file path of relation");
-DATA(insert OID = 2316 ( postgresql_fdw_validator PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 16 "1009 26" _null_ _null_ _null_ _null_ postgresql_fdw_validator _null_ _null_ _null_));
-DESCR("(internal)");
-
DATA(insert OID = 2290 ( record_in PGNSP PGUID 12 1 0 0 0 f f f f t f s 3 0 2249 "2275 26 23" _null_ _null_ _null_ _null_ record_in _null_ _null_ _null_ ));
DESCR("I/O");
DATA(insert OID = 2291 ( record_out PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 2275 "2249" _null_ _null_ _null_ _null_ record_out _null_ _null_ _null_ ));
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 1bfdca1..0a5b525 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -13,6 +13,40 @@ CREATE ROLE regress_test_role2;
CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -1202,6 +1236,7 @@ DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
NOTICE: drop cascades to server s0
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
-- At this point we should have no wrappers, no servers, and no mappings.
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 38057db..46e358b 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -20,6 +20,41 @@ CREATE ROLE regress_test_role_super SUPERUSER;
CREATE ROLE regress_test_indirect;
CREATE ROLE unprivileged_role;
+-- create validator for this test
+CREATE OR REPLACE FUNCTION postgresql_fdw_validator(params text[], oid oid) RETURNS bool AS $$
+DECLARE
+ c text;
+ val text[];
+BEGIN
+ -- validate each option
+ FOR c IN SELECT unnest(params) LOOP
+ -- separate key and value
+ SELECT regexp_split_to_array(c, '=') INTO val;
+ RAISE DEBUG 'record %=%', val[1], val[2];
+
+ IF oid = 'pg_catalog.pg_foreign_server'::regclass THEN
+ IF val[1] NOT IN ('authtype', 'service', 'connect_timeout',
+ 'dbname', 'host', 'hostaddr', 'port', 'tty', 'options',
+ 'requiressl', 'sslmode', 'gsslib') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: authtype, service, connect_timeout, dbname, host, hostaddr, port, tty, options, requiressl, sslmode, gsslib';
+ END IF;
+ ELSIF oid = 'pg_catalog.pg_user_mapping'::regclass THEN
+ IF val[1] NOT IN ('user', 'password') THEN
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: user, password';
+ END IF;
+ ELSE
+ RAISE 'invalid option "%"', val[1]
+ USING HINT = 'Valid options in this context are: ';
+ END IF;
+
+ END LOOP;
+
+ RETURN true;
+END;
+$$ language plpgsql;
+
CREATE FOREIGN DATA WRAPPER dummy;
COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless';
CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator;
@@ -497,6 +532,7 @@ DROP ROLE unprivileged_role;
DROP ROLE regress_test_role2;
DROP FOREIGN DATA WRAPPER postgresql CASCADE;
DROP FOREIGN DATA WRAPPER dummy CASCADE;
+DROP FUNCTION postgresql_fdw_validator(text[], oid);
\c
DROP ROLE foreign_data_user;
Hanada-san,
It is fair enough for me.
So, I'd like to hand over this patch for committers.
Thanks,
2012/10/9 Shigeru HANADA <shigeru.hanada@gmail.com>:
(2012/10/09 0:30), Kohei KaiGai wrote:
The attached patch is a revised one according to my previous
suggestion. It re-defines "PQconninfoOption *options" as static
variable with NULL initial value, then, PQconndefaults() shall
be invoked at once. The default options never changed during
duration of the backend process, so here is no reason why we
allocate and free this object for each validator invocation.Sorry for delayed response. It seems reasonable, so I just fixed obsolete
comment and indent. Please see attached v3 patch which was rebased against
latest head of master.If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.Please go ahead. :-)
Regards,
--
Shigeru HANADA
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Shigeru HANADA <shigeru.hanada@gmail.com> writes:
(2012/10/09 0:30), Kohei KaiGai wrote:
If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.
Please go ahead. :-)
While reviewing this patch, I realized that we can't just summarily
delete the built-in postgresql_fdw_validator function: if we do, any
existing foreign-data wrapper definitions that depend on it will fail
to load into 9.3 servers. This would at least cause problems during
pg_upgrade for anyone who has set up dblink foreign servers according
to the current recommendation.
So I think we can't remove that functionality just yet. What we could
do is adjust postgresql_fdw_validator to throw a WARNING that it's
deprecated. This wouldn't prevent it from being used during dump/reload
scenarios, but it would put people on notice that the code will be
removed eventually. Without such a warning, it's not clear that we'll
ever be able to remove it without getting complaints.
However, I'm not sure where that leaves us with respect to the original
goal of getting rid of use of that function name. Thoughts?
For the moment I'm going to commit just the dblink changes in the patch,
and leave the core code alone pending agreement on what to do about the
upgrade-path issue.
regards, tom lane
Shigeru HANADA <shigeru.hanada@gmail.com> writes:
[ dblink_fdw_validator.v3.patch ]
I've committed the dblink portion of this with some mostly-cosmetic
adjustments. We still need a plan for getting to a point where it's
safe to remove postgresql_fdw_validator.
regards, tom lane
Sorry for delayed response.
On 2012/10/11, at 5:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So I think we can't remove that functionality just yet. What we could
do is adjust postgresql_fdw_validator to throw a WARNING that it's
deprecated. This wouldn't prevent it from being used during dump/reload
scenarios, but it would put people on notice that the code will be
removed eventually. Without such a warning, it's not clear that we'll
ever be able to remove it without getting complaints.
After reading discussion about deprecating RULE, I realized that we should
be conservative when changing existing user-visible behavior. Agreed that
we can't remove postgresql_fdw_validator without announcing for some
releases.
However, I'm not sure where that leaves us with respect to the original
goal of getting rid of use of that function name. Thoughts?
Sorry, I had misunderstood the problem :-(. In my proposal, postgresql_fdw
uses public schema, as other contrib modules do, so its validator can live
with existing pg_catalog.postgresql_fdw_validator. IMHO we should
remove postgresql_fdw_validator sooner or later, but we don't need to hurry
to remove existing postgresql_fdw_validator from core.
Of course we must ensure that postgresql_fdw never uses in-core validator,
and dblink and other product never use postgresql_fdw's validator. To
achieve this, how about to use a schema, say postgresql_fdw, for
postgresql_fdw by specifying "schema" option in extension control file?
We need to qualify function names, so relocatable should be false. This
requires users of postgresql_fdw to set search_path or qualify
postgresql_fdw's functions and views every time, but it seems acceptable.
In addition, this approach would prevent pollution of public schema.
Comments?
Regards,
--
Shigeru HANADA
shigeru.hanada@gmail.com
On Fri, Oct 19, 2012 at 7:17 AM, Shigeru HANADA
<shigeru.hanada@gmail.com> wrote:
However, I'm not sure where that leaves us with respect to the original
goal of getting rid of use of that function name. Thoughts?Sorry, I had misunderstood the problem :-(. In my proposal, postgresql_fdw
uses public schema, as other contrib modules do, so its validator can live
with existing pg_catalog.postgresql_fdw_validator. IMHO we should
remove postgresql_fdw_validator sooner or later, but we don't need to hurry
to remove existing postgresql_fdw_validator from core.Of course we must ensure that postgresql_fdw never uses in-core validator,
and dblink and other product never use postgresql_fdw's validator. To
achieve this, how about to use a schema, say postgresql_fdw, for
postgresql_fdw by specifying "schema" option in extension control file?
We need to qualify function names, so relocatable should be false. This
requires users of postgresql_fdw to set search_path or qualify
postgresql_fdw's functions and views every time, but it seems acceptable.In addition, this approach would prevent pollution of public schema.
It seems to me that this is a case of the tail wagging the dog. The
original reason we ran into this issue is because there were some
people (I forget who, sorry) who insisted that this had to be renamed
from pgsql_fdw to postgresql_fdw. That change then caused this naming
conflict. Now, normally what we do if we have a naming conflict is we
rename one of the two things so that we don't have a naming conflict.
If we've determined that we can't rename postgresql_fdw_validator for
reasons of backward compatibility, then we should rename this new
thing instead. We of course do not have to use the original
pgsql_fdw_validator name; it can be postgres_fdw_validator or
postgree_fdw_validator or prostgreskewell_fdw_validator or whatever
the consensus bikeshed position is. Moving it to another schema does
not particularly appeal to me as it seems that having two
identically-named validators in different schemas will be even more
confusing than having two similarly-named validators in the same
schema.
If we're unwilling to tolerate committing this under some other name,
and we're also unwilling to remove postgresql_fdw_validator, then
we're essentially asking that we wait 4 or 5 years (or however long it
will take to deprecate postgresql_fdw_validator) to commit this very
important functionality to the server on the basis of the fact that
we've got a trivial name collision. That seems excessive in the
extreme. I'm frankly sorta shocked how much delay this issue has
*already* caused. I suspect there are other issues with regard this
patch that are much more worthy of our attention.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2012/10/19 Robert Haas <robertmhaas@gmail.com>:
On Fri, Oct 19, 2012 at 7:17 AM, Shigeru HANADA
<shigeru.hanada@gmail.com> wrote:However, I'm not sure where that leaves us with respect to the original
goal of getting rid of use of that function name. Thoughts?Sorry, I had misunderstood the problem :-(. In my proposal, postgresql_fdw
uses public schema, as other contrib modules do, so its validator can live
with existing pg_catalog.postgresql_fdw_validator. IMHO we should
remove postgresql_fdw_validator sooner or later, but we don't need to hurry
to remove existing postgresql_fdw_validator from core.Of course we must ensure that postgresql_fdw never uses in-core validator,
and dblink and other product never use postgresql_fdw's validator. To
achieve this, how about to use a schema, say postgresql_fdw, for
postgresql_fdw by specifying "schema" option in extension control file?
We need to qualify function names, so relocatable should be false. This
requires users of postgresql_fdw to set search_path or qualify
postgresql_fdw's functions and views every time, but it seems acceptable.In addition, this approach would prevent pollution of public schema.
It seems to me that this is a case of the tail wagging the dog. The
original reason we ran into this issue is because there were some
people (I forget who, sorry) who insisted that this had to be renamed
from pgsql_fdw to postgresql_fdw. That change then caused this naming
conflict. Now, normally what we do if we have a naming conflict is we
rename one of the two things so that we don't have a naming conflict.
If we've determined that we can't rename postgresql_fdw_validator for
reasons of backward compatibility, then we should rename this new
thing instead. We of course do not have to use the original
pgsql_fdw_validator name; it can be postgres_fdw_validator or
postgree_fdw_validator or prostgreskewell_fdw_validator or whatever
the consensus bikeshed position is.
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Sorry for long absence.
On Sat, Oct 20, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?
AFAIR, in the discussion about naming of the new FDW, another
name postgres_fdw was suggested as well as postgresql_fdw, and
I chose longer one at that time. Perhaps only a few people
feel that "postgres" is shortened name of postgresql. How
about using postgres_fdw for PG-FDW?
Once we chose the different name, postgresql_fdw_validator can
be live with postgres_fdw, though their names seem little
confusing.
In addition, it would be worth mentioning that it's not
recommended to use postgresql_fdw_validator as validator of a
third-party's FDW to avoid dependency.
Regards,
--
Shigeru HANADA
Sorry for long absence.
On Sat, Oct 20, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?
AFAIR, in the discussion about naming of the new FDW, another
name postgres_fdw was suggested as well as postgresql_fdw, and I
chose the one more familiar to me at that time. I think that only few
people feel that "postgres" is shortened name of
postgresql.
How about using postgres_fdw for PG-FDW?
Regards,
--
Shigeru HANADA
On Thu, Nov 15, 2012 at 02:33:21PM +0900, Shigeru Hanada wrote:
On Sat, Oct 20, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?AFAIR, in the discussion about naming of the new FDW, another
name postgres_fdw was suggested as well as postgresql_fdw, and I
chose the one more familiar to me at that time. I think that only few
people feel that "postgres" is shortened name of
postgresql.How about using postgres_fdw for PG-FDW?
I couldn't agree more with Robert's comments[1]http://archives.postgresql.org/message-id/CA+TgmobzOCV9RWUXO=xM_NkzrmPYZ0LgVuWhxyzueThZEqJHqw@mail.gmail.com. Furthermore, this name only
shows up in calls to {CREATE|ALTER} FOREIGN DATA WRAPPER, which means 99.9% of
users would write "CREATE EXTENSION postgresql_fdw" and never even see the
name. I'd take "postgresql_fdw_whoops_names_are_a_big_commitment" if it meant
settling this issue 30 days earlier than we'd otherwise settle it.
Notwithstanding, I propose "postgresql.org/contrib/postgresql_fdw/validator".
Since the sole code that ought to reference the name lives in
contrib/postgresql_fdw/*.sql, the verbosity and double-quotation will cause no
appreciable harm. If anything, it will discourage ill-advised users.
Thanks,
nm
On 11/16/2012 08:08 AM, Noah Misch wrote:
On Thu, Nov 15, 2012 at 02:33:21PM +0900, Shigeru Hanada wrote:
On Sat, Oct 20, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?AFAIR, in the discussion about naming of the new FDW, another
name postgres_fdw was suggested as well as postgresql_fdw, and I
chose the one more familiar to me at that time. I think that only few
people feel that "postgres" is shortened name of
postgresql.How about using postgres_fdw for PG-FDW?
I couldn't agree more with Robert's comments[1]. Furthermore, this name only
shows up in calls to {CREATE|ALTER} FOREIGN DATA WRAPPER, which means 99.9% of
users would write "CREATE EXTENSION postgresql_fdw" and never even see the
name. I'd take "postgresql_fdw_whoops_names_are_a_big_commitment" if it meant
settling this issue 30 days earlier than we'd otherwise settle it.Notwithstanding, I propose "postgresql.org/contrib/postgresql_fdw/validator".
Since the sole code that ought to reference the name lives in
contrib/postgresql_fdw/*.sql, the verbosity and double-quotation will cause no
appreciable harm. If anything, it will discourage ill-advised users.
Was there any further progress on this? Committing of the postgresql_fdw
seems to be stalled on a naming issue that has a couple of reasonable
resolutions available, and it'd be nice to get it in as a contrib module.
https://commitfest.postgresql.org/action/patch_view?id=940
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
2013/1/18 Craig Ringer <craig@2ndquadrant.com>:
On 11/16/2012 08:08 AM, Noah Misch wrote:
On Thu, Nov 15, 2012 at 02:33:21PM +0900, Shigeru Hanada wrote:
On Sat, Oct 20, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
IIRC, the reason why postgresql_fdw instead of pgsql_fdw was
no other fdw module has shorten naming such as ora_fdw for
Oracle.
However, I doubt whether it is enough strong reason to force to
solve the technical difficulty; naming conflicts with existing user
visible features.
Isn't it worth to consider to back to the pgsql_fdw_validator
naming again?AFAIR, in the discussion about naming of the new FDW, another
name postgres_fdw was suggested as well as postgresql_fdw, and I
chose the one more familiar to me at that time. I think that only few
people feel that "postgres" is shortened name of
postgresql.How about using postgres_fdw for PG-FDW?
I couldn't agree more with Robert's comments[1]. Furthermore, this name
only
shows up in calls to {CREATE|ALTER} FOREIGN DATA WRAPPER, which means 99.9%
of
users would write "CREATE EXTENSION postgresql_fdw" and never even see the
name. I'd take "postgresql_fdw_whoops_names_are_a_big_commitment" if it
meant
settling this issue 30 days earlier than we'd otherwise settle it.Notwithstanding, I propose
"postgresql.org/contrib/postgresql_fdw/validator".
Since the sole code that ought to reference the name lives in
contrib/postgresql_fdw/*.sql, the verbosity and double-quotation will cause
no
appreciable harm. If anything, it will discourage ill-advised users.Was there any further progress on this? Committing of the postgresql_fdw
seems to be stalled on a naming issue that has a couple of reasonable
resolutions available, and it'd be nice to get it in as a contrib module.
The current patch adopts "postgres_fdw" as name; that does never conflict
with existing functions, and well means what does this extension provide.
Previously, it was named "pgsql_fdw" but it was unpopular because of some
reasons; such as we don't call Oracle as Ora, why we call postgresql as pgsql?
I think, both of naming are good. It will give right impression for users about
functionality of this extension, and also add a new killer feature to v9.3.
If we spent waste of time for this topic any more, nobody will get happy.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers