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

On 2021/02/01 16:39, Bharath Rupireddy wrote:
> On Mon, Feb 1, 2021 at 12:43 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> On 2021/01/27 10:06, Bharath Rupireddy wrote:
>>> On Tue, Jan 26, 2021 at 8:38 AM Bharath Rupireddy
>>> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>>> I will post "keep_connections" GUC and "keep_connection" server level
>>>> option patches later.
>>>
>>> Attaching v19 patch set for "keep_connections" GUC and
>>> "keep_connection" server level option. Please review them further.
>>
>> These options are no longer necessary because we now support idle_session_timeout? If we want to disconnect the foreign server connections that sit on idle to prevent them from eating up the connection capacities in the foriegn servers, we can just set idle_session_timeout in those foreign servers. If we want to avoid the cluster-wide setting of idle_session_timeout, we can set that per role. One issue for this approach is that the connection entry remains even after idle_session_timeout happens. So postgres_fdw_get_connections() returns that connection even though it's actually closed by the timeout. Which is confusing. But which doesn't cause any actual problem, right? When the foreign table is accessed the next time, that connection entry is dropped, an error is detected, and then new connection will be remade.
>
> First of all, idle_session_timeout is by default 0 i.e. disabled,
> there are chances that users may not use that and don't want to set it
> just for not caching any foreign server connection. A simple use case
> where server level option can be useful is that, users are accessing
> foreign tables (may be not that frequently, once in a while) from a
> long running local session using foreign servers and they don't want
> to keep the local session cache those connections, then setting this
> server level option, keep_connections to false makes their life
> easier, without having to depend on setting idle_session_timeout on
> the remote server.

Thanks for explaining this!

I understand that use case. But I still think that we can use
idle_session_timeout for that use case without keep_connections.
Per the past discussion, Robert seems to prefer controling the cached
connection by timeout rather than boolean, at [1]. Bruce seems to think
that idle_session_timeout is enough for the use case, at [2]. So I'm not
sure what the current consensus is...

Also Alexey seems to have thought that idle_session_timeout is not
suitable for cached connection because it's the cluster-wide option, at [3].
But since it's marked as PGC_USERSET, we can set it per-role, e.g.,
by using ALTER ROLE SET, so that it can affect only the foreign server
connections.

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.

[1]
https://www.postgresql.org/message-id/CA%2BTgmob_nF7NkBfVLUhmQ%2Bt8JGVV4hXy%2BzkuMUtTSd-%3DHPBeuA%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/20200714165822.GE7628%40momjian.us

[3]
https://www.postgresql.org/message-id/6df6525ca7a4b54a4a39f55e4dd6b3e9%40postgrespro.ru

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

>
> So, IMO, we can still have both server level option as well as
> postgres_fdw contrib level GUC (to tell the local session that "I
> don't want to keep any foreign connections active" instead of setting
> keep_connection server level option for each foreign server).
>
>> Sorry I've not read the past long discussion about this feature. If there is the consensus that these options are still necessary and useful even when we have idle_session_timeout, please correct me.
>>
>> ISTM that it's intuitive (at least for me) to add this kind of option into the foreign server. But I'm not sure if it's good idea to expose the option as GUC. Also if there is the consensus about this, please correct me.
>
> See here [1].
>
> [1] - https://www.postgresql.org/message-id/f58d1df4ae58f6cf3bfa560f923462e0%40postgrespro.ru

Thanks!

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?

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?

Regards,

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