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

On Wed, Nov 25, 2020 at 12:13 AM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)ru> wrote:
>
> 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.
>

Done this way.

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

Done.

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

Removed the cacheid variable.

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

Done.

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

Done.

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

Done.

Attaching the v2 patch set. Please review it further.

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