On Thu, Jan 21, 2021 at 8:58 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> My opinion is to check "!all", but if others prefer using such boolean flag,
> I'd withdraw my opinion.
I'm really sorry, actually if (!all) is enough there, my earlier
understanding was wrong.
> + if ((all || entry->server_hashvalue == hashvalue) &&
>
> What about making disconnect_cached_connections() accept serverid instead
> of hashvalue, and perform the above comparison based on serverid? That is,
> I'm thinking "if (all || entry->serverid == serverid)". If we do that, we can
> simplify postgres_fdw_disconnect() a bit more by getting rid of the calculation
> of hashvalue.
That's a good idea. I missed this point. Thanks.
> + if ((all || entry->server_hashvalue == hashvalue) &&
> + entry->conn)
>
> I think that it's better to make the check of "entry->conn" independent
> like other functions in postgres_fdw/connection.c. What about adding
> the following check before the above?
>
> /* Ignore cache entry if no open connection right now */
> if (entry->conn == NULL)
> continue;
Done.
> + /*
> + * If the server has been dropped in the current explicit
> + * transaction, then this entry would have been invalidated
> + * in pgfdw_inval_callback at the end of drop sever
> + * command. Note that this connection would not have been
> + * closed in pgfdw_inval_callback because it is still being
> + * used in the current explicit transaction. So, assert
> + * that here.
> + */
> + Assert(entry->invalidated);
>
> As this comment explains, even when the connection is used in the transaction,
> its server can be dropped in the same transaction. The connection can remain
> until the end of transaction even though its server has been already dropped.
> I'm now wondering if this behavior itself is problematic and should be forbid.
> Of course, this is separate topic from this patch, though..
>
> BTW, my just idea for that is;
> 1. change postgres_fdw_get_connections() return also serverid and xact_depth.
> 2. make postgres_fdw define the event trigger on DROP SERVER command so that
> an error is thrown if the connection to the server is still in use.
> The event trigger function uses postgres_fdw_get_connections() to check
> if the server connection is still in use or not.
>
> I'm not sure if this just idea is really feasible or not, though...
I'm not quite sure if we can create such a dependency i.e. blocking
"drop foreign server" when at least one session has an in use cached
connection on it? What if a user wants to drop a server from one
session, all other sessions one after the other keep having in-use
connections related to that server, (though this use case sounds
impractical) will the drop server ever be successful? Since we can
have hundreds of sessions in real world postgres environment, I don't
know if it's a good idea to create such dependency.
As you suggested, this point can be discussed in a separate thread and
if any of the approaches proposed by you above is finalized we can
extend postgres_fdw_get_connections anytime.
Thoughts?
Attaching v16 patch set, addressing above review comments and also
added a test case suggested upthread that postgres_fdw_disconnect()
with existing server name returns false that is when the cache doesn't
have active connection.
Please review the v16 patch set further.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
