|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
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,
+ 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)
+ 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
+ entry->changing_xact_state) ||
+ (entry->used_in_current_xact &&
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.
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
|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"|