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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
Date: 2020-11-23 16:27:18
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

+ 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)
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

> 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

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

Otherwise, both patches seem to be working as expected. I am going to
have a look on the last two patches a bit later.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Grigory Smolkin 2020-11-23 16:37:36 Re: pg_upgrade fails with non-standard ACL
Previous Message Alvaro Herrera 2020-11-23 16:24:30 Re: pg_proc.dat "proargmodes is not a 1-D char array"