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

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.kondratov@postgrespro.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