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

Thanks for the review comments.

On Mon, Nov 23, 2020 at 9:57 PM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)ru> wrote:
>
> > 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().
>

dblink_disconnect() "Returns status, which is always OK (since any
error causes the function to throw an error instead of returning)."
This behaviour doesn't seem okay to me.

Since we throw true/false, I would prefer to throw a warning(with a
reason) while returning false over an error.

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

I added cachid as an argument to disconnect_cached_connections() for
reusability. Say, someone wants to use it with a user mapping then
they can pass cacheid USERMAPPINGOID, hash value of user mapping. The
cacheid == USERMAPPINGOID && entry->mapping_hashvalue == hashvalue can
be added to disconnect_cached_connections().

>
> 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;
> }
>

Yeah. I will add a cacheid check and change it to below.

if ((all || (cacheid == FOREIGNSERVEROID &&
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.
>

I think we should. When we disconnect all the connections, then no
point in keeping the connection cache hash data structure. If required
it gets created at the next first foreign server usage in the same
session. And also, hash_destroy() frees up memory context unlike
hash_search with HASH_REMOVE, so we can save a bit of memory.

> >
> > 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.
>

I missed this. Thanks, we can use the local variable as you suggested.
I will change it.

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

Thanks. I will work on the comments so far and post updated patches soon.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com