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

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.

So, the below code in the patch, first checks the GUC. If the GUC is
off, then discards the connections. If the GUC is on, then it further
checks the server level option. If it's off discards the connection,
otherwise not.

I would like it to keep this behaviour as is. Thoughts?

if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
entry->changing_xact_state ||
entry->invalidated ||
+ (used_in_current_xact &&
+ (!keep_connections || !entry->keep_connection)))
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);

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