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

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