On 2021/01/23 13:40, Bharath Rupireddy wrote:
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:
Also like pg_terminate_backend(), we should disallow non-superuser to
disconnect the connections established by other non-superuser if the requesting
user is not a member of the other? Or that's overkill because the target to
discard is just a connection and it can be established again if necessary?
For now I'm thinking that it might better to add the restriction like
pg_terminate_backend() at first and relax that later if possible. But I'd like
hear more opinions about this.
+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.
Also non-superuser (set by SET ROLE or SET SESSION AUTHORIZATION) seems to be
able to run SQL using the dblink connection established by superuser. If we
didn't think that this is a problem, we also might not need to care about issue
even for postgres_fdw.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
