Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Fri, Jan 22, 2021 at 6:43 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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