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

On 2020/11/27 11:12, Bharath Rupireddy wrote:
> 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.

Regarding the 0001 patch, we should add the function that returns
the information of cached connections like dblink_get_connections(),
together with 0001 patch? Otherwise it's not easy for users to
see how many cached connections are and determine whether to
disconnect them or not. Sorry if this was already discussed before.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION