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

On 2021/02/03 13:56, Bharath Rupireddy wrote:
> On Tue, Feb 2, 2021 at 9:45 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> One merit of keep_connections that I found is that we can use it even
>> when connecting to the older PostgreSQL that doesn't support
>> idle_session_timeout. Also it seems simpler to use keep_connections
>> rather than setting idle_session_timeout in multiple remote servers.
>> So I'm inclined to add this feature, but I'd like to hear more opinions.
>
> Thanks.
>
>>> And, just using idle_session_timeout on a remote server may not help
>>> us completely. Because the remote session may go away, while we are
>>> still using that cached connection in an explicit txn on the local
>>> session. Our connection retry will also not work because we are in the
>>> middle of an xact, so the local explicit txn gets aborted.
>>
>> Regarding idle_in_transaction_session_timeout, this seems true. But
>> I was thinking that idle_session_timeout doesn't cause this issue because
>> it doesn't close the connection in the middle of transaction. No?
>
> You are right. idle_session_timeout doesn't take effect when in the
> middle of an explicit txn. I missed this point.
>
>> Here are some review comments.
>>
>> - (used_in_current_xact && !keep_connections))
>> + (used_in_current_xact &&
>> + (!keep_connections || !entry->keep_connection)))
>>
>> The names of GUC and server-level option should be the same,
>> to make the thing less confusing?
>
> We can have GUC name keep_connections as there can be multiple
> connections within a local session and I can change the server level
> option keep_connection to keep_connections because a single foreign
> server can have multiple connections as we have seen that in the use
> case identified by you. I will change that in the next patch set.
>
>> IMO the server-level option should override GUC. IOW, GUC setting
>> should be used only when the server-level option is not specified.
>> But the above code doesn't seem to do that. Thought?
>
> Note that default values for GUC and server level option are on i.e.
> connections are cached.
>
> The main intention of the GUC is to not set server level options to
> false for all the foreign servers in case users don't want to keep any
> foreign server connections. If the server level option overrides GUC,
> then even if users set GUC to off, they have to set the server level
> option to false for all the foreign servers.

Maybe my explanation in the previous email was unclear. What I think is; If the server-level option is explicitly specified, its setting is used whatever GUC is. On the other hand, if the server-level option is NOT specified, GUC setting is used. For example, if we define the server as follows, GUC setting is used because the server-level option is NOT specified.

CREATE SERVER loopback FOREIGN DATA WRAPPER postgres;

If we define the server as follows, the server-level setting is used.

CREATE SERVER loopback FOREIGN DATA WRAPPER postgres OPTIONS (keep_connections 'on');

For example, log_autovacuum_min_duration GUC and reloption work in the similar way. That is, reloption setting overrides GUC. If reltion is not specified, GUC is used.

Regards,

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