On Fri, Jan 22, 2021 at 6:43 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > >> Please review the v16 patch set further. > > > > Thanks! Will review that later. > > + /* > + * For the given server, if we closed connection or > it is still in > + * use, then no need of scanning the cache further. > We do this > + * because the cache can not have multiple cache > entries for a > + * single foreign server. > + */ > > On second thought, ISTM that single foreign server can have multiple cache > entries. For example, > > CREATE ROLE foo1 SUPERUSER; > CREATE ROLE foo2 SUPERUSER; > CREATE EXTENSION postgres_fdw; > CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw OPTIONS (port > '5432'); > CREATE USER MAPPING FOR foo1 SERVER loopback OPTIONS (user 'postgres'); > CREATE USER MAPPING FOR foo2 SERVER loopback OPTIONS (user 'postgres'); > CREATE TABLE t (i int); > CREATE FOREIGN TABLE ft (i int) SERVER loopback OPTIONS (table_name 't'); > SET SESSION AUTHORIZATION foo1; > SELECT * FROM ft; > SET SESSION AUTHORIZATION foo2; > SELECT * FROM ft; > > > Then you can see there are multiple open connections for the same server > as follows. So we need to scan all the entries even when the serverid is > specified. > > SELECT * FROM postgres_fdw_get_connections(); > > server_name | valid > -------------+------- > loopback | t > loopback | t > (2 rows)
This is a great finding. Thanks a lot. I will remove hash_seq_term(&scan); in disconnect_cached_connections and add this as a test case for postgres_fdw_get_connections function, just to show there can be multiple connections with a single server name. > This means that user (even non-superuser) can disconnect the connection > established by another user (superuser), by using > postgres_fdw_disconnect_all(). > Is this really OK? Yeah, connections can be discarded by non-super users using postgres_fdw_disconnect_all and postgres_fdw_disconnect. Given the fact that a non-super user requires a password to access foreign tables [1], IMO a non-super user changing something related to a super user makes no sense at all. If okay, we can have a check in disconnect_cached_connections something like below: +static bool +disconnect_cached_connections(Oid serverid) +{ + HASH_SEQ_STATUS scan; + ConnCacheEntry *entry; + bool all = !OidIsValid(serverid); + bool result = false; + + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser to discard open connections"))); + + if (!ConnectionHash) Having said that, it looks like dblink_disconnect doesn't perform superuser checks. Thoughts? [1] SELECT * FROM ft1_nopw LIMIT 1; ERROR: password is required DETAIL: Non-superusers must provide a password in the user mapping. > + if (all || (OidIsValid(serverid) && entry->serverid == > serverid)) > + { > > I don't think that "OidIsValid(serverid)" condition is necessary here. > But you're just concerned about the case where the caller mistakenly > specifies invalid oid and all=false? One idea to avoid that inconsistent > combination of inputs is to change disconnect_cached_connections() > as follows. > > -disconnect_cached_connections(Oid serverid, bool all) > +disconnect_cached_connections(Oid serverid) > { > HASH_SEQ_STATUS scan; > ConnCacheEntry *entry; > + bool all = !OidIsValid(serverid); +1. Will change it. > + * in pgfdw_inval_callback at the end > of drop sever > > Typo: "sever" should be "server". +1. Will change it. > +-- =================================================================== > +-- test postgres_fdw_disconnect function > +-- =================================================================== > > This regression test is placed at the end of test file. But isn't it better > to place that just after the regression test "test connection invalidation > cases" because they are related? +1. Will change it. > + <screen> > +postgres=# SELECT * FROM postgres_fdw_disconnect('loopback1'); > + postgres_fdw_disconnect > > The tag <screen> should start from the beginning. +1. Will change it. > As I commented upthread, what about replacing the example query with > "SELECT postgres_fdw_disconnect('loopback1');" because it's more common? Sorry, I forgot to check that in the documentation earlier. +1. Will change it. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com