Re: SQL/MED compatible connection manager

Howdy,

Currently pl/proxy, dblink, DBI-link etc. each have their own remote connection
info management infrastructure (or none at all). It would certainly help if
they could use a common connection database -- with access control, pg_dump
support, etc. There have been hints that a SQL/MED compatible connection
manager would be desirable:
http://archives.postgresql.org/pgsql-hackers/2008-09/msg00314.php
http://archives.postgresql.org/pgsql-hackers/2008-09/msg00909.php

So the proposal is to implement a small subset of SQL/MED to cope with
connection info management -- connection manager. This will only manage the
connection metadata and provide the required system catalogs and commands for
maintaining them. The actual connection management (open/close etc.) is still
handled by the client modules.

I have put together a draft that describes a possible implementation:
http://wiki.postgresql.org/wiki/SqlMedConnectionManager

Tons of details have been omitted, but should be enough to start discussion.
What do you think, does this sound usable? Suggestions, objections?

thanks,
Martin

On Mon, Oct 27, 2008 at 10:06 AM, Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> wrote:
> So the proposal is to implement a small subset of SQL/MED to cope with
> connection info management -- connection manager. This will only manage the
> connection metadata and provide the required system catalogs and commands for
> maintaining them. The actual connection management (open/close etc.) is still
> handled by the client modules.

Per SQL:2008, there are no expected changes to SQL/MED from SQL:2003.
As such, two weeks ago, I completed a full review of SQL/MED and am
planning to fully implement it for 8.5. Currently, I'm working on a
proof of concept and have created a SQL/MED access method (sqlmed) as
well as started implementing the FDW API and hooks into the optimizer
to support remote capabilities, costing, and predicate pushdown. The
first wrappers I intend to support are ODBC and

This is a large project, and I'm certainly open to assistance :)

--
Jonah H. Harris, Senior DBA
myYearbook.com

On Mon, Oct 27, 2008 at 10:35 AM, Jonah H. Harris
<jonah(dot)harris(at)gmail(dot)com> wrote:
> The first wrappers I intend to support are ODBC and

Damn multiple windows :)

The first wrappers I intend to support are ODBC and CSV/fixed-width text.

--
Jonah H. Harris, Senior DBA
myYearbook.com

> Per SQL:2008, there are no expected changes to SQL/MED from SQL:2003.
> As such, two weeks ago, I completed a full review of SQL/MED and am
> planning to fully implement it for 8.5. Currently, I'm working on a
> proof of concept and have created a SQL/MED access method (sqlmed) as
> well as started implementing the FDW API and hooks into the optimizer
> to support remote capabilities, costing, and predicate pushdown. The
> first wrappers I intend to support are ODBC and

Cool. Have you published some notes on it (wiki etc)?

>
> This is a large project, and I'm certainly open to assistance :)
>

It certainly is an undertaking :) I'm mostly interested in the connection
management -- so hopefully I can help there.

regards,
Martin

On Mon, Oct 27, 2008 at 11:31 AM, Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> wrote:
> Cool. Have you published some notes on it (wiki etc)?

Not yet. Discussed it a little on irc, but nothing substantial. I'll
look at updating the Wiki hopefully today.

> It certainly is an undertaking :) I'm mostly interested in the connection
> management -- so hopefully I can help there.

That would be awesome!

--
Jonah H. Harris, Senior DBA
myYearbook.com

martin(dot)pihlak(at)gmail(dot)com (Martin Pihlak) writes:
> Tons of details have been omitted, but should be enough to start discussion.
> What do you think, does this sound usable? Suggestions, objections?

Slony-I does some vaguely similar stuff in its handling of "connection paths"; here's the schema:

create table @NAMESPACE(at)(dot)sl_path (
pa_server int4,
pa_client int4,
pa_conninfo text NOT NULL,
pa_connretry int4,

CONSTRAINT "sl_path-pkey"
PRIMARY KEY (pa_server, pa_client),
CONSTRAINT "pa_server-no_id-ref"
FOREIGN KEY (pa_server)
REFERENCES @NAMESPACE(at)(dot)sl_node (no_id),
CONSTRAINT "pa_client-no_id-ref"
FOREIGN KEY (pa_client)
REFERENCES @NAMESPACE(at)(dot)sl_node (no_id)
) WITHOUT OIDS;
comment on table @NAMESPACE(at)(dot)sl_path is 'Holds connection information for the paths between nodes, and the synchronisation delay';
comment on column @NAMESPACE(at)(dot)sl_path(dot)pa_server is 'The Node ID # (from sl_node.no_id) of the data source';
comment on column @NAMESPACE(at)(dot)sl_path(dot)pa_client is 'The Node ID # (from sl_node.no_id) of the data target';
comment on column @NAMESPACE(at)(dot)sl_path(dot)pa_conninfo is 'The PostgreSQL connection string used to connect to the source node.';
comment on column @NAMESPACE(at)(dot)sl_path(dot)pa_connretry is 'The synchronisation delay, in seconds';

I wouldn't be surprised to find there being some value in using
something like SQL/MED.

One detail I'll point out, that I'm noticing from an application I'm
working on right now. We might want to have something like a "db
connection" data type; here's a prototype I put together:

slonyregress1=# create type dbconn as (port integer, dbname text, username text, password text, ssl boolean);
CREATE TYPE
slonyregress1=# create table dbconns (id serial primary key, db dbconn);
NOTICE: CREATE TABLE will create implicit sequence "dbconns_id_seq" for serial column "dbconns.id"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "dbconns_pkey" for table "dbconns"
CREATE TABLE
slonyregress1=# insert into dbconns (db) values ((5432, 'slonyregress1', 'slony', 'secret!', 'true'));
INSERT 0 1
slonyregress1=# select * from dbconns;
id | db
----+--------------------------------------
1 | (5432,slonyregress1,slony,secret!,t)
(1 row)

I'm not certain that this is forcibly the right representation, but I
think it is possible that we'd want a finer-grained representation
than merely a connection string.
--
(reverse (concatenate 'string "ofni.sesabatadxunil" "@" "enworbbc"))
http://linuxdatabases.info/info/finances.html
"DTDs are not common knowledge because programming students are not
taught markup. A markup language is not a programming language."
-- Peter Flynn <silmaril(at)m-net(dot)arbornet(dot)org>

Chris Browne wrote:

> Slony-I does some vaguely similar stuff in its handling of "connection paths"; here's the schema:
>
> create table @NAMESPACE(at)(dot)sl_path (
> pa_server int4,
> pa_client int4,
> pa_conninfo text NOT NULL,
> pa_connretry int4,
[snip ...]
> I wouldn't be surprised to find there being some value in using
> something like SQL/MED.
>

Here the pa_conninfo could be replaced with the connection name (actually
SERVER). For the complete connection definition a USER MAPPING (eg. remote
username and password) is also needed. But that can be fetched by the
connection connection lookup function

> One detail I'll point out, that I'm noticing from an application I'm
> working on right now. We might want to have something like a "db
> connection" data type; here's a prototype I put together:
>

> slonyregress1=# create type dbconn as (port integer, dbname text, username text, password text, ssl boolean);
> CREATE TYPE
[snip]
> slonyregress1=# select * from dbconns;
> id | db
> ----+--------------------------------------
> 1 | (5432,slonyregress1,slony,secret!,t)
> (1 row)
>
> I'm not certain that this is forcibly the right representation, but I
> think it is possible that we'd want a finer-grained representation
> than merely a connection string.

Yes -- the server, user mapping and FDW all take generic options. Some of them
might be part of the connect string, others could specify some hints of how the
connection should be handled (driver options etc). DBD-Excel has a particularly
nasty example of those. A fixed type would not be able to cover all of them.
This is where the SQL/MED stuff can help - all of this complexity can be reduced
to a single name. Though it adds the additional step of doing the lookup.

The dbconns example could be written like this:

test=# create table dbconns (id serial primary key, db regserver);
...
test=# insert into dbconns (db) values ('test');
INSERT 0 1
test=# select * from dbconns;
id | db
----+-------------
1 | public.test
(1 row)

And then for the connection details:

test=# select * from pg_get_remote_connection_info('test');
option | value
----------+--------
host | /tmp
port | 6543
dbname | foo
username | bob
password | secret
(5 rows)

This assumes that there is a server "public.test" and a user mapping for
the session user. The option/value pairs are outputted by the "dummy" FDW
that just dumps the generic options for the server and user mapping. A
"smart" FDW could turn this into just a connection string. Still, there
probably should be a set of functions for accessing the raw options/value
pairs as well

regards,
Martin

On Mon, 2008-10-27 at 16:50 -0400, Chris Browne wrote:
> martin(dot)pihlak(at)gmail(dot)com (Martin Pihlak) writes:
> > Tons of details have been omitted, but should be enough to start discussion.
> > What do you think, does this sound usable? Suggestions, objections?
>
> Slony-I does some vaguely similar stuff in its handling of "connection paths"; here's the schema:

I think the whole issue was initially raised by "insecurity", as dblink
conrib module exposed connection strings to all users, and SQL/MED was
seen as a "standard" way to hide it.

The simple credentials hiding could of course be achieved by having
something similar to pg_user/pg_shadow and some SECURITY DEFINER
functions for actually opening the connections, but probably it seemed
easier to at least base it on standards, so we can actually start with

pg_remote_server table public
pg_user_mapping_shadow table (restricted)/ pg_user_mapping view(public)

and some functions with proper grants to match the subset that Martin
outlined in http://wiki.postgresql.org/wiki/SqlMedConnectionManager

if we've got that working, then we could move to massaging it into the
parser to provide standard SQL/MED syntax.

so I think that first we should agree on functionality and get the few
system (?) tables and functions done, and worry about parser changes
once the actual functionality is field tested.

------------------------------------------
Hannu Krosing http://www.2ndQuadrant.com
PostgreSQL Scalability and Availability
Services, Consulting and Training

> I have put together a draft that describes a possible implementation:
> http://wiki.postgresql.org/wiki/SqlMedConnectionManager
>

I'll update this with an experimental patch. This implements:
* System catalogs for FDW, SERVER and USER MAPPING
* regserver data type for servers (convert from text to oid etc).
* FDW placeholders as shared libraries -- currently provides dummy
and pgsql wrappers.
* Connection lookup via FDW.
* SQL interface to the lookup function.

There is an example of how all this works at:
http://wiki.postgresql.org/wiki/SqlMedConnectionManager#Current_implementation

I was hoping to get the internal features done by the start of November
Commitfest. But right now this seems unlikely. Nevertheless, I'm all ears
for suggestions and criticism.

PS. Jonah, does this somehow coincide with your approach to FDW-s?

regards,
Martin

Here is an updated patch. I'm also submitting this to November Commitfest -
I believe it is possible to get this into shape for 8.4. This is still a WIP
but I really need a review before moving on with the syntax, pg_dump support
etc.

Currently the system catalogs and user accessible connection lookup function
have been implemented. Ships with 2 FOREIGN DATA WRAPPERS -- dummy and pgsql.
It is possible to define connections by inserting directly into the catalogs:

insert into pg_catalog.pg_foreign_server
select 'foo', 2200, 10, oid, null,
'{host=/tmp,port=6543,dbname=foo}'::text[]
from pg_foreign_data_wrapper
where fdwname='default';

insert into pg_catalog.pg_foreign_user_mapping
select 10, oid, '{user=bob,password=secret}'::text[]
from pg_foreign_server
where srvname='foo' ;

select * from pg_get_remote_connection_info('foo');

option | value
----------+--------
host | /tmp
port | 6543
dbname | foo
user | bob
password | secret
(5 rows)

regards,
Martin

On Fri, 2008-10-31 at 17:49 +0200, Martin Pihlak wrote:
> Here is an updated patch. I'm also submitting this to November Commitfest -
> I believe it is possible to get this into shape for 8.4. This is still a WIP
> but I really need a review before moving on with the syntax, pg_dump support
> etc.

Any hope of getting pl/proxy using this submitted for 8.4 ?

------------------------------------------
Hannu Krosing http://www.2ndQuadrant.com
PostgreSQL Scalability and Availability
Services, Consulting and Training

Attached is next revision of the connection manager, this is now nearly
feature complete -- the syntax is there, privileges are implemented.
Should apply cleanly to HEAD and pass regression. There are some usage
examples at: http://wiki.postgresql.org/wiki/SqlMedConnectionManager#Examples

Some issues that come to mind:
- Object naming: probably should drop "foreign_" prefix from user mapping
and server. This would leave us with pg_user_mapping and pg_server.
Maybe add a _shadow suffix to the hidden version of pg_user_mapping.
- MAPPING became a reserved keyword, this is not good (unreserved causes
shift/reduce conflicts).
- There is some more grammar hacking to be done - ALTER handling is not
complete (allows nop statements, impossible to reset options, cannot use
default as FDW name, ...).
- System catalog and connection lookup function privileges are revoked from
public in system_views.sql, is there an alternative?
- Worry about FDW library function pointers getting stale (library reloads)
- Do we need to support copyObject/equal for the fdw/server/user mapping
parse nodes?

Things to do:
- internal cleanup - add verbose commentary where needed, cleanup error
reporting.
- documentation
- more thorough regression tests
- psql support (tab completion, help, \dX commands)
- pg_dump support
- ecpg support?

regards,
Martin

Martin Pihlak <martin(dot)pihlak(at)gmail(dot)com> writes:
> - Do we need to support copyObject/equal for the fdw/server/user mapping
> parse nodes?

Yes.

regards, tom lane

Here's another revision of the connection manager. This adds:
* reference documentation
* psql, tab-completion, \dw, \dr, \dm commands.
* pg_dump support

Still todo:
* more comprehensive regression tests
* introductory documentation
* dblink support

I hope to finish these items during next week, and remove the WIP
status then.

regards,
Martin

Martin Pihlak wrote:
> Here's another revision of the connection manager. This adds:
> * reference documentation
> * psql, tab-completion, \dw, \dr, \dm commands.
> * pg_dump support
>
> Still todo:
> * more comprehensive regression tests
> * introductory documentation
> * dblink support
>
> I hope to finish these items during next week, and remove the WIP
> status then.

Looks very good, please continue.

Most of this is straight out of the standard, so there isn't much to
discuss on the interfaces. I have two small things to wonder about:

On your wiki page, you have this example:

CREATE SERVER userdb TYPE 'plproxy_cluster'
FOREIGN DATA WRAPPER plproxy
OPTIONS (
server='dbname=userdb_p0 host=127.0.0.1 port=6000',
server='dbname=userdb_p1 host=127.0.0.1 port=6000',
server='dbname=userdb_p2 host=127.0.0.1 port=6000',
server='dbname=userdb_p3 host=127.0.0.1 port=6000',
connection_lifetime=3600
);

If I read this right, SQL/MED requires option names to be unique for a
server. To this needs to be rethought.

Do we really need the function pg_get_remote_connection_info()? This
could be done directly with the information schema. E.g., your example

SELECT * FROM pg_get_remote_connection_info('userdb');

appears to be the same as

SELECT option_name, option_value
FROM information_schema.foreign_server_options
WHERE foreign_server_name = 'userdb';

This view also appears to have the necessary access control provisions.

And similarly, pg_get_user_mapping_options() is equivalent to
information_schema.user_mapping_options.

Peter Eisentraut wrote:
> Looks very good, please continue.
>
Thanks, will do :)

> On your wiki page, you have this example:
>
> CREATE SERVER userdb TYPE 'plproxy_cluster'
> FOREIGN DATA WRAPPER plproxy
> OPTIONS (
> server='dbname=userdb_p0 host=127.0.0.1 port=6000',
> server='dbname=userdb_p1 host=127.0.0.1 port=6000',
[snip]
> If I read this right, SQL/MED requires option names to be unique for a
> server. To this needs to be rethought.
>

Indeed, seems that I somehow managed to miss that. Additionally, according
to the standard the options should be specified as <name> <value>, instead
of <name> = <value>. Plus the possibility to alter individual options.
I'll look into that.

Updated the wiki to use unique option names.

> Do we really need the function pg_get_remote_connection_info()? This
> could be done directly with the information schema. E.g., your example
>
> SELECT * FROM pg_get_remote_connection_info('userdb');

The purpouse of the connection lookup function is to compose the the actual
connection string from generic options (adds user mapping if needed). This
aims to make it easier for the clients to perform connection lookups. The
idea is that the connection string should be composed by the foreign data
wrapper, instead of letting each client have it's own interpretation of the
options. Though, it is still possible to query the options directly.

> And similarly, pg_get_user_mapping_options() is equivalent to
> information_schema.user_mapping_options.
>

Hmm, the options are stored as text[], these need to be transformed to be
usable in the views. Seems that additional functions for foreign data wrapper
and server options are also needed. Will add those, along with the
information_schema views.

regards,
Martin

Peter Eisentraut wrote:
> If I read this right, SQL/MED requires option names to be unique for a
> server. To this needs to be rethought.
>

Attached is another revision of the connection manager, notable changes:
* Generic options are now standard compliant -- no duplicate option names,
possibility to alter individual options.
* information_schema views, added to the end of the file, using chapter
numbers from part 5 of the standard. Qualified names are used in fdw
and server names unless directly visible.
* Added documentation for the connection lookup functions in "System
Administration Functions". Also documented the new system catalogs.
* Example dblink implementation. Added functions dblink_connect_s,
dblink_exec_s, dblink_s that operate on predefined foreign servers.
No documentation or regression tests at the moment. Some examples at:
http://wiki.postgresql.org/wiki/SqlMedConnectionManager#dblink

I'll also change the commitfest status to "Pending review" -- the features
are complete and I'm not actively working on the code.

regards,
Martin

Martin Pihlak wrote:
> Peter Eisentraut wrote:
>> If I read this right, SQL/MED requires option names to be unique for a
>> server. To this needs to be rethought.
>>
>
> Attached is another revision of the connection manager, notable changes:

Attached is my current patch after surgery. I have mainly worked on
making naming better and more consistent.

Problem: You have implemented foreign-data wrappers and foreign servers
as schema-qualified objects, but the standard has them outside schemas,
qualified only optionally by catalogs (a.k.a. databases). I think that
should be fixed.

Peter Eisentraut wrote:
> Attached is my current patch after surgery. I have mainly worked on
> making naming better and more consistent.
>
Thanks.

> Problem: You have implemented foreign-data wrappers and foreign servers
> as schema-qualified objects, but the standard has them outside schemas,
> qualified only optionally by catalogs (a.k.a. databases). I think that
> should be fixed.

Darn. At least it is a lot easier to root out the schema support
than to add it ... Will look into it.

regards,
Martin

Peter Eisentraut wrote:
> Problem: You have implemented foreign-data wrappers and foreign servers
> as schema-qualified objects, but the standard has them outside schemas,
> qualified only optionally by catalogs (a.k.a. databases). I think that
> should be fixed.

Attached. Removed schema support for foreign data wrapper and server,
updated documentation and regression tests.

Also added has_foreign_data_wrapper_privilege and has_server_privilege
functions. information_schema views use those to determine if user
has usage on the foreign data wrapper or server.

regards,
Martin

Now I have a question about the FDW C interface. The way I understand
it, an SQL/MED-enabled server and a FDW each have a specific API by
which they communicate. Supposedly, each database vendor should be able
to ship a binary library for its FDW and each SQL/MED-enabled server
should be able to load and use it. (If you don't believe in binary
compatibility, then I think there should at least be source-level
interface compatibility.)

Now the way I read the FDWs you provide (default and pgsql), you are
creating your own API for initialization and options validation that is
not in the standard. That would appear to contradict the idea of a
standard interface. I understand that option validation is useful, and
I don't see anything about it in the standard, but should we break the
API like that? What are your designs about this?

Peter Eisentraut wrote:
> Now the way I read the FDWs you provide (default and pgsql), you are
> creating your own API for initialization and options validation that is
> not in the standard. That would appear to contradict the idea of a
> standard interface. I understand that option validation is useful, and
> I don't see anything about it in the standard, but should we break the
> API like that? What are your designs about this?
>

Hmm, in that perspective it would make sense to make the InitializeFdw
function optional (it was, before I got worried about library reloads).
If no InitializeFdw is present, connection lookup and option validation
are disabled. All of the standard defined FDW functions are fetched by
load_external_function. This way we could have the additional features
and still be able to load standard conforming FDW's.

Actually it would make sense to use _PG_init instead of InitializeFdw.
This way it'd be called automatically on library load, the parameter(s)
would be passed in globals though.

regards,
Martin

Peter Eisentraut wrote:
> Now the way I read the FDWs you provide (default and pgsql), you are
> creating your own API for initialization and options validation that is
> not in the standard. That would appear to contradict the idea of a
> standard interface.

This is now fixed -- the option validation and connection lookup functions
have been made optional. InitializeFDW and _PG_fini have been dropped,
functions are looked up in GetForeignDataWrapperLibrary(). I decided not to
worry too much about the function pointers getting stale due to library
changes and reloads, as that requires some deliberate actions as a superuser.

I also added _pg prefixes to the non-standard functions so that these are not
confused with the standard FDW functions.

psql describe commands to \dew, \des and \deu as discussed in
http://archives.postgresql.org/pgsql-hackers/2008-12/msg00888.php

PS. Would it be more convenient to use the ~user area at git.postgresql.org
for review? I haven't requested a user account yet, but will do so if it
simplifies the review. The patches would still be posted to list as well.

regards,
Martin

On Fri, Dec 12, 2008 at 7:55 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Now I have a question about the FDW C interface. The way I understand it,
> an SQL/MED-enabled server and a FDW each have a specific API by which they
> communicate. Supposedly, each database vendor should be able to ship a
> binary library for its FDW and each SQL/MED-enabled server should be able to
> load and use it. (If you don't believe in binary compatibility, then I
> think there should at least be source-level interface compatibility.)

Yes, all FDWs should be similar to ODBC drivers in that they are
self-contained and interface with the database through a defined API.
What happens inside them should be irrelevant to PG.

--
Jonah H. Harris, Senior DBA
myYearbook.com

On Monday 15 December 2008 22:30:19 Jonah H. Harris wrote:
> On Fri, Dec 12, 2008 at 7:55 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > Now I have a question about the FDW C interface. The way I understand
> > it, an SQL/MED-enabled server and a FDW each have a specific API by which
> > they communicate. Supposedly, each database vendor should be able to
> > ship a binary library for its FDW and each SQL/MED-enabled server should
> > be able to load and use it. (If you don't believe in binary
> > compatibility, then I think there should at least be source-level
> > interface compatibility.)
>
> Yes, all FDWs should be similar to ODBC drivers in that they are
> self-contained and interface with the database through a defined API.
> What happens inside them should be irrelevant to PG.

What we are currently trying to figure out is the best method to introduce
extensions to the API.

Martin Pihlak wrote:
> This is now fixed -- the option validation and connection lookup functions
> have been made optional. InitializeFDW and _PG_fini have been dropped,
> functions are looked up in GetForeignDataWrapperLibrary(). I decided not to
> worry too much about the function pointers getting stale due to library
> changes and reloads, as that requires some deliberate actions as a superuser.

I never understood that reload business complete anyway. If you think
there are issues at run time, they should be documented somewhere.

> I also added _pg prefixes to the non-standard functions so that these are not
> confused with the standard FDW functions.

Yes, I think something like that should be OK.

> PS. Would it be more convenient to use the ~user area at git.postgresql.org
> for review? I haven't requested a user account yet, but will do so if it
> simplifies the review. The patches would still be posted to list as well.

Well, maybe a month ago. ;-) We are getting pretty close to committing.

I am not satisfied with the custom SQL functions that you added:

| pg_get_foreign_data_wrapper_options(fdwid oid)
| pg_get_foreign_server_options(srvid oid)
| pg_get_user_mapping_options(umid oid)

I think these are basically just a way to parse apart {a=1,b=2} into a
table. We could get more bang out of it, if we provided one function
that can do that parsing for all of fdwoptions, srvoptions, umoptions,
reloptions, datconfig, useconfig, proconfig. (reloptions and *config
use different parsers internally, but maybe that is not so important for
this problem.)

The other thing that I am not settled on is the default FDW (I renamed
it to dummy). In principle, this thing should do nothing, so the source
file ought to empty. Well, _pg_validateOptionList *is* empty, but
_pg_GetConnectionInfo has an excuse implementation that makes me think
that the pg_get_remote_connection_info() function has a too specific
mission to be a general function. If we added, say, an XML-file FDW,
what sense would pg_get_remote_connection_info() make?

For the record, my current patch, which merges all your latest changes,
is attached.

I could offer, if it turns out to be possible, that I cut these
contentious parts out of the patch and commit the rest as soon as
possible, because I am growing weary of moving this big patch around. :-)

Peter Eisentraut wrote:
>> worry too much about the function pointers getting stale due to library
>> changes and reloads, as that requires some deliberate actions as a
>> superuser.
>
> I never understood that reload business complete anyway. If you think
> there are issues at run time, they should be documented somewhere.
>

Lets say a backend has the library loaded and the FDW function pointers
already initialized. Now the FDW library file is upgraded, and the user
issues a LOAD command to reload the library. The library is reloaded, but
the function pointers never get updated. Attempt to use the FDW functions
most likely crashes the server.

The options are:
- always look up functions immediately before use (performance penalty)
- use _PG_fini callback to register FDW unloads (needs cooperating library)
- document that reloading is not supported (ie. this is a feature)
- just ignore it, as there are probably a dozen more ways a superuser can
crash the server.

> I am not satisfied with the custom SQL functions that you added:
>
> | pg_get_foreign_data_wrapper_options(fdwid oid)
> | pg_get_foreign_server_options(srvid oid)
> | pg_get_user_mapping_options(umid oid)
>
> I think these are basically just a way to parse apart {a=1,b=2} into a
> table.

Hmm, I just realized that there are only OID variants of those -- indeed
those are not very useful. If names were used instead, the functions would
be a lot more useful. Especially so, if FDW doesn't provide connection
lookup.

> The other thing that I am not settled on is the default FDW (I renamed
> it to dummy). In principle, this thing should do nothing, so the source
> file ought to empty. Well, _pg_validateOptionList *is* empty, but
> _pg_GetConnectionInfo has an excuse implementation that makes me think
> that the pg_get_remote_connection_info() function has a too specific
> mission to be a general function. If we added, say, an XML-file FDW,
> what sense would pg_get_remote_connection_info() make?
>

It'd make more sense if we changed the name to pg_get_datasource ;)

We could make the pg_get_remote_connection_info Postgres specific -- in
this case it would be changed to return just the connect string text. NULL
for the other wrappers -- for these use the pg_get*options to construct
the connect strings. Comments?

One more thing just occured to me -- the dummy and postgresql wrappers are
currently installed by initdb. The majority of installations will probably
never use them. So I think it would make sense to ship with no predefined
FDW-s.

regards,
Martin

I have committed this without the functions. I have some thoughts on
what to do about that, but right now I'm exhausted. ;-)

Peter Eisentraut wrote:
> I have committed this without the functions. I have some thoughts on
> what to do about that, but right now I'm exhausted. ;-)
>
Great news :) Thanks a lot for your support and contributions!

regards,
Martin

On Wednesday 17 December 2008 17:17:26 Martin Pihlak wrote:
> It'd make more sense if we changed the name to pg_get_datasource ;)
>
> We could make the pg_get_remote_connection_info Postgres specific -- in
> this case it would be changed to return just the connect string text. NULL
> for the other wrappers -- for these use the pg_get*options to construct
> the connect strings. Comments?

Well, what this function essentially does is a text transformation of the
options, something like this:

peter=# SELECT array_to_string(fdwoptions || srvoptions || umoptions, ' ')
FROM pg_foreign_data_wrapper fdw, pg_foreign_server srv, pg_user_mappings um
WHERE fdw.oid = srv.srvfdw AND srv.oid = um.srvid;
array_to_string
-----------------------------------------------------
host=localhost port=5432 user=peter password=seKret
(1 row)

(You can enhance this with quoting etc., but that's the essence.)

So, we could add a function whose job it is to convert all options to a
PostgreSQL connection string. I wouldn't worry about dealing with other
wrappers specifically. They could still use the function, but the result
would not make much sense.

I would call it something like

pg_postgresql_fdw_options_string(server, user) returns text

Not sure if a C implementation based on your old function or an SQL
implementation is better.

Peter Eisentraut wrote:
> Well, what this function essentially does is a text transformation of the
> options, something like this:
>
> peter=# SELECT array_to_string(fdwoptions || srvoptions || umoptions, ' ')
> FROM pg_foreign_data_wrapper fdw, pg_foreign_server srv, pg_user_mappings um
> WHERE fdw.oid = srv.srvfdw AND srv.oid = um.srvid;
> array_to_string
> -----------------------------------------------------
> host=localhost port=5432 user=peter password=seKret
> (1 row)
>
> (You can enhance this with quoting etc., but that's the essence.)

Essentially yes. Additional things include USAGE check on the server and user
mapping lookup (use public if no explicit mapping is specified). Without those
I'm not really sure this deserves a separate function at all. The main goal
is to provide standard semantics for the connection lookup, so that dblink,
plproxy, pl rpc etc. would not have to reinvent it.

> So, we could add a function whose job it is to convert all options to a
> PostgreSQL connection string. I wouldn't worry about dealing with other
> wrappers specifically. They could still use the function, but the result
> would not make much sense.
>
This works for me. I'd implement this as a C function so it is directly
callable from other C modules.

Another option is to implement it as a SRF, similar to what was initially in the
dummy wrapper. Just return all of the options for fdw, server and user mapping.
This is probably worth doing if there are any users for this. So far I haven't
noticed any enthusiasm, so it might be better to start with just the connection
string.

> I would call it something like
>
> pg_postgresql_fdw_options_string(server, user) returns text

Hmm, it is probably a good idea to avoid the fdw abbreviation -- the term
"foreign data wrapper" is already confusing enough. My suggestion:

pg_foreign_server_conninfo(server)
pg_foreign_server_conninfo(server,user)

If there are no objections, I'll whack those functions out, and bring the dblink
patch up to date.

regards,
Martin

On Thu, Jan 01, 2009 at 11:10:38PM +0200, Martin Pihlak wrote:
> Peter Eisentraut wrote:
> > Well, what this function essentially does is a text transformation of the
> > options, something like this:
> >
> > peter=# SELECT array_to_string(fdwoptions || srvoptions || umoptions, ' ')
> > FROM pg_foreign_data_wrapper fdw, pg_foreign_server srv, pg_user_mappings um
> > WHERE fdw.oid = srv.srvfdw AND srv.oid = um.srvid;
> > array_to_string
> > -----------------------------------------------------
> > host=localhost port=5432 user=peter password=seKret
> > (1 row)
> >
> > (You can enhance this with quoting etc., but that's the essence.)
>
> Essentially yes. Additional things include USAGE check on the server and user
> mapping lookup (use public if no explicit mapping is specified). Without those
> I'm not really sure this deserves a separate function at all. The main goal
> is to provide standard semantics for the connection lookup, so that dblink,
> plproxy, pl rpc etc. would not have to reinvent it.
>
> > So, we could add a function whose job it is to convert all options to a
> > PostgreSQL connection string. I wouldn't worry about dealing with other
> > wrappers specifically. They could still use the function, but the result
> > would not make much sense.
> >
> This works for me. I'd implement this as a C function so it is
> directly callable from other C modules.
>
> Another option is to implement it as a SRF, similar to what was
> initially in the dummy wrapper. Just return all of the options for
> fdw, server and user mapping. This is probably worth doing if there
> are any users for this. So far I haven't noticed any enthusiasm, so
> it might be better to start with just the connection string.

The connection string could be pretty different if it's not a
PostgreSQL database, so +1 on the SRF option :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Martin Pihlak wrote:
>> I would call it something like
>>
>> pg_postgresql_fdw_options_string(server, user) returns text
>
> Hmm, it is probably a good idea to avoid the fdw abbreviation -- the term
> "foreign data wrapper" is already confusing enough. My suggestion:
>
> pg_foreign_server_conninfo(server)
> pg_foreign_server_conninfo(server,user)
>
> If there are no objections, I'll whack those functions out, and bring the dblink
> patch up to date.

Sure, propose some code. (Note that you can use parameter default
values now.)

Peter Eisentraut wrote:
> Martin Pihlak wrote:
>>> I would call it something like
>>>
>>> pg_postgresql_fdw_options_string(server, user) returns text
>>
>> Hmm, it is probably a good idea to avoid the fdw abbreviation -- the term
>> "foreign data wrapper" is already confusing enough. My suggestion:
>>
>> pg_foreign_server_conninfo(server)
>> pg_foreign_server_conninfo(server,user)
>>
>> If there are no objections, I'll whack those functions out, and bring
>> the dblink
>> patch up to date.
>
> Sure, propose some code. (Note that you can use parameter default
> values now.)
>

Proposal attached. This adds two C functions:

List *GetForeignConnectionOptions(Oid serverid, Oid userid);
char *GetForeignConnectionString(Oid serverid, Oid userid);

One for obtaining all of the connection related options as a list, and
another for transforming these options into a libpq conninfo string.
The latter should be useful for dblink (although the userid and serverid
need to be obtained first).

On top of those there are two SQL accessible functions:

pg_foreign_connection_options(server name, user name = current_user,
OUT option_class text, OUT option_name text, OUT option_value text);

pg_foreign_connection_string(server name, user name = current_user);

These should initially be restricted from ordinary users -- grant explicitly
if the user should see the connect strings. Otherwise use from security definer
functions. The pg_foreign_connection_options() exposes all of the connection
options and can be used by clients such as DBI link to construct the connect
string or equivalent. pg_foreign_connection_string() can be used for instance
by plpythonu or plperlu functions to connect to remote postgres database.

Example:

select * from pg_foreign_connection_options('foo');

option_class | option_name | option_value
--------------+-------------+--------------
server | host | localhost
server | port | 54321
server | dbname | foo
user mapping | user | bob
user mapping | password | secret
(5 rows)

select * from pg_foreign_connection_string('foo');

pg_foreign_connection_string
-------------------------------------------------------------------------
host='localhost' port='54321' dbname='foo' user='bob' password='secret'
(1 row)

Will add regression and tests if this is acceptable.

PS. I'm not sure if I nailed the "proargdefaults" syntax correctly in pg_proc.h,
for now I just copied it out from a sample function with similar arguments.

regards,
Martin

Martin Pihlak wrote:
> Proposal attached. This adds two C functions:
>
> List *GetForeignConnectionOptions(Oid serverid, Oid userid);
> char *GetForeignConnectionString(Oid serverid, Oid userid);
>
> One for obtaining all of the connection related options as a list, and
> another for transforming these options into a libpq conninfo string.
> The latter should be useful for dblink (although the userid and serverid
> need to be obtained first).
>
> On top of those there are two SQL accessible functions:
>
> pg_foreign_connection_options(server name, user name = current_user,
> OUT option_class text, OUT option_name text, OUT option_value text);
>
> pg_foreign_connection_string(server name, user name = current_user);
>
> These should initially be restricted from ordinary users -- grant explicitly
> if the user should see the connect strings.

Back to this one ...

I have been thinking about this for a great while now. I am not yet
comfortable with how we manage the access rights here. We have
restricted access to the user mappings catalog to hide passwords, but it
is not entirely clear why a password must be stored in a user mapping.
It could also be stored with a server, if we only want to use one global
connection for everybody.

I think the proper way to handle it might be to introduce a new
privilege type -- call it SELECT if you like -- that determines
specifically whether you can *see* the options of a foreign-data
wrapper, foreign server, or user mapping, respectively. As opposed to
USAGE, which means you can use the object for connecting (in the
future). This might have other uses: The owner of a server might want
to hide the host name, but still let you connect.

Comments?

On Wed, Mar 04, 2009 at 03:26:36PM +0200, Peter Eisentraut wrote:
> Martin Pihlak wrote:
>> Proposal attached. This adds two C functions:
>>
>> List *GetForeignConnectionOptions(Oid serverid, Oid userid);
>> char *GetForeignConnectionString(Oid serverid, Oid userid);
>>
>> One for obtaining all of the connection related options as a list,
>> and another for transforming these options into a libpq conninfo
>> string. The latter should be useful for dblink (although the
>> userid and serverid need to be obtained first).
>>
>> On top of those there are two SQL accessible functions:
>>
>> pg_foreign_connection_options(server name, user name =
>> current_user, OUT option_class text, OUT option_name text, OUT
>> option_value text);
>>
>> pg_foreign_connection_string(server name, user name =
>> current_user);
>>
>> These should initially be restricted from ordinary users -- grant
>> explicitly if the user should see the connect strings.
>
> Back to this one ...
>
> I have been thinking about this for a great while now. I am not yet
> comfortable with how we manage the access rights here. We have
> restricted access to the user mappings catalog to hide passwords,
> but it is not entirely clear why a password must be stored in a
> user mapping. It could also be stored with a server, if we only
> want to use one global connection for everybody.
>
> I think the proper way to handle it might be to introduce a new
> privilege type -- call it SELECT if you like -- that determines
> specifically whether you can *see* the options of a foreign-data
> wrapper, foreign server, or user mapping, respectively. As opposed
> to USAGE, which means you can use the object for connecting (in the
> future). This might have other uses: The owner of a server might
> want to hide the host name, but still let you connect.
>
> Comments?

This could have a more general usage, too. Does SQL:2008 have
anything to say about such a capability, or is it already in the
column-level privileges, or...?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> I think the proper way to handle it might be to introduce a new
> privilege type -- call it SELECT if you like -- that determines
> specifically whether you can *see* the options of a foreign-data
> wrapper, foreign server, or user mapping, respectively. As opposed to
> USAGE, which means you can use the object for connecting (in the
> future). This might have other uses: The owner of a server might want
> to hide the host name, but still let you connect.

How would you implement/enforce that, in the absence of row-level
security on the catalogs involved?

regards, tom lane

Peter Eisentraut wrote:
> I have been thinking about this for a great while now. I am not yet
> comfortable with how we manage the access rights here. We have
> restricted access to the user mappings catalog to hide passwords, but it
> is not entirely clear why a password must be stored in a user mapping.
> It could also be stored with a server, if we only want to use one global
> connection for everybody.
>

Hmm, in this case one would probably create a PUBLIC user mapping and
store the password there. But indeed, there could be other aspects
of the server that need to be kept secret.

> I think the proper way to handle it might be to introduce a new
> privilege type -- call it SELECT if you like -- that determines
> specifically whether you can *see* the options of a foreign-data
> wrapper, foreign server, or user mapping, respectively.

How about providing an optional masking function for the foreign data
wrapper. The function would accept the generic options array and remove/mask
any undesired options. Ordinary users would access the catalogs by views, and
only see the filtered or masked options. The owner and superuser would
still have to get the full options though.

Just an idea.

regards,
Martin