On 2021/01/22 1:17, Bharath Rupireddy wrote:
On Thu, Jan 21, 2021 at 8:58 PM Fujii Masao <masao.fu...@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?
Maybe my explanation was not clear... I was thinking to prevent the server
whose connection is used *within the current transaction* from being dropped.
IOW, I was thinking to forbid the drop of server if xact_depth of its
connection is more than one. So one session can drop the server even when its
connection is open in other session if it's not used within the transaction
(i.e., xact_depth == 0).
BTW, for now, if the connection is used within the transaction, other session
cannot drop the corresponding server because the transaction holds the lock on
the relations that depend on the server. Only the session running that
transaction can drop the server. This can cause the issue in discussion.
So, my just idea is to disallow even that session running the transaction to drop
the server. This means that no session can drop the server while its connection is
used within the transaction (xact_depth > 0).
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?
I will consider more before starting separate discussion!
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.
Thanks! Will review that later.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
