Hi,
On 2020-11-23 09:48, Bharath Rupireddy wrote:
Here is how I'm making 4 separate patches:
1. new function and it's documentation.
2. GUC and it's documentation.
3. server level option and it's documentation.
4. test cases for all of the above patches.
Hi, I'm attaching the patches here. Note that, though the code changes
for this feature are small, I divided them up as separate patches to
make review easy.
v1-0001-postgres_fdw-function-to-discard-cached-connections.patch
This patch looks pretty straightforward for me, but there are some
things to be addressed IMO:
+ server = GetForeignServerByName(servername, true);
+
+ if (server != NULL)
+ {
Yes, you return a false if no server was found, but for me it worth
throwing an error in this case as, for example, dblink does in the
dblink_disconnect().
+ result = disconnect_cached_connections(FOREIGNSERVEROID,
+ hashvalue,
+ false);
+ if (all || (!all && cacheid == FOREIGNSERVEROID &&
+ entry->server_hashvalue == hashvalue))
+ {
+ if (entry->conn != NULL &&
+ !all && cacheid == FOREIGNSERVEROID &&
+ entry->server_hashvalue == hashvalue)
These conditions look bulky for me. First, you pass FOREIGNSERVEROID to
disconnect_cached_connections(), but actually it just duplicates 'all'
flag, since when it is 'FOREIGNSERVEROID', then 'all == false'; when it
is '-1', then 'all == true'. That is all, there are only two calls of
disconnect_cached_connections(). That way, it seems that we should keep
only 'all' flag at least for now, doesn't it?
Second, I think that we should just rewrite this if statement in order
to simplify it and make more readable, e.g.:
if ((all || entry->server_hashvalue == hashvalue) &&
entry->conn != NULL)
{
disconnect_pg_server(entry);
result = true;
}
+ if (all)
+ {
+ hash_destroy(ConnectionHash);
+ ConnectionHash = NULL;
+ result = true;
+ }
Also, I am still not sure that it is a good idea to destroy the whole
cache even in 'all' case, but maybe others will have a different
opinion.
v1-0002-postgres_fdw-add-keep_connections-GUC-to-not-cache-connections.patch
+ entry->changing_xact_state) ||
+ (entry->used_in_current_xact &&
+ !keep_connections))
I am not sure, but I think, that instead of adding this additional flag
into ConnCacheEntry structure we can look on entry->xact_depth and use
local:
bool used_in_current_xact = entry->xact_depth > 0;
for exactly the same purpose. Since we set entry->xact_depth to zero at
the end of xact, then it was used if it is not zero. It is set to 1 by
begin_remote_xact() called by GetConnection(), so everything seems to be
fine.
Otherwise, both patches seem to be working as expected. I am going to
have a look on the last two patches a bit later.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
