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

On 2020-11-24 06:52, Bharath Rupireddy wrote:
> 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.
>

I thought about something a bit more sophisticated:

1) Return 'true' if there were open connections and we successfully
closed them.
2) Return 'false' in the no-op case, i.e. there were no open
connections.
3) Rise an error if something went wrong. And non-existing server case
belongs to this last category, IMO.

That looks like a semantically correct behavior, but let us wait for any
other opinion.

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

Yeah, I have got your point and motivation to add this argument, but how
we can use it? To disconnect all connections belonging to some specific
user mapping? But any user mapping is hard bound to some foreign server,
AFAIK, so we can pass serverid-based hash in this case.

In the case of pgfdw_inval_callback() this argument makes sense, since
syscache callbacks work that way, but here I can hardly imagine a case
where we can use it. Thus, it still looks as a preliminary complication
for me, since we do not have plans to use it, do we? Anyway, everything
seems to be working fine, so it is up to you to keep this additional
argument.

>
> v1-0003-postgres_fdw-server-level-option-keep_connection.patch
> This patch adds a new server level option, keep_connection, default
> being on, when set to off, the local session doesn't cache the
> connections associated with the foreign server.
>

This patch looks good to me, except one note:

(entry->used_in_current_xact &&
- !keep_connections))
+ (!keep_connections || !entry->keep_connection)))
{

Following this logic:

1) If keep_connections == true, then per-server keep_connection has a
*higher* priority, so one can disable caching of a single foreign
server.

2) But if keep_connections == false, then it works like a global switch
off indifferently of per-server keep_connection's, i.e. they have a
*lower* priority.

It looks fine for me, at least I cannot propose anything better, but
maybe it should be documented in 0004?

>
> v1-0004-postgres_fdw-connection-cache-discard-tests-and-documentation.patch
> This patch adds the tests and documentation related to this feature.
>

I have not read all texts thoroughly, but what caught my eye:

+ A GUC, <varname>postgres_fdw.keep_connections</varname>, default
being
+ <literal>on</literal>, when set to <literal>off</literal>, the local
session

I think that GUC acronym is used widely only in the source code and
Postgres docs tend to do not use it at all, except from acronyms list
and a couple of 'GUC parameters' collocation usage. And it never used in
a singular form there, so I think that it should be rather:

A configuration parameter,
<varname>postgres_fdw.keep_connections</varname>, default being...

+ <para>
+ Note that when <varname>postgres_fdw.keep_connections</varname>
is set to
+ off, <filename>postgres_fdw</filename> discards either the
connections
+ that are made previously and will be used by the local session or
the
+ connections that will be made newly. But the connections that are
made
+ previously and kept, but not used after this parameter is set to
off, are
+ not discarded. To discard them, use
+ <function>postgres_fdw_disconnect</function> function.
+ </para>

The whole paragraph is really difficult to follow. It could be something
like that:

<para>
Note that setting <varname>postgres_fdw.keep_connections</varname>
to
off does not discard any previously made and still open
connections immediately.
They will be closed only at the end of a future transaction, which
operated on them.

To close all connections immediately use
<function>postgres_fdw_disconnect</function> function.
</para>

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company