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

On 2021/01/29 15:44, Bharath Rupireddy wrote:
> On Fri, Jan 29, 2021 at 11:54 AM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>> IIRC, when we were finding a way to close the invalidated connections
>>>> so that they don't leaked, we had two options:
>>>>
>>>> 1) let those connections (whether currently being used in the xact or
>>>> not) get marked invalidated in pgfdw_inval_callback and closed in
>>>> pgfdw_xact_callback at the main txn end as shown below
>>>>
>>>> if (PQstatus(entry->conn) != CONNECTION_OK ||
>>>> PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
>>>> entry->changing_xact_state ||
>>>> entry->invalidated). ----> by adding this
>>>> {
>>>> elog(DEBUG3, "discarding connection %p", entry->conn);
>>>> disconnect_pg_server(entry);
>>>> }
>>>>
>>>> 2) close the unused connections right away in pgfdw_inval_callback
>>>> instead of marking them invalidated. Mark used connections as
>>>> invalidated in pgfdw_inval_callback and close them in
>>>> pgfdw_xact_callback at the main txn end.
>>>>
>>>> We went with option (2) because we thought this would ease some burden
>>>> on pgfdw_xact_callback closing a lot of invalid connections at once.
>>>
>>> Also, see the original patch for the connection leak issue just does
>>> option (1), see [1]. But in [2] and [3], we chose option (2).
>>>
>>> I feel, we can go for option (1), with the patch attached in [1] i.e.
>>> having have_invalid_connections whenever any connection gets invalided
>>> so that we don't quickly exit in pgfdw_xact_callback and the
>>> invalidated connections get closed properly. Thoughts?
>>
>> Before going for (1) or something, I'd like to understand what the actual
>> issue of (2), i.e., the current code is. Otherwise other approaches might
>> have the same issue.
>
> The problem with option (2) is that because of CLOBBER_CACHE_ALWAYS,
> pgfdw_inval_callback is getting called many times and the connections
> that are not used i..e xact_depth == 0, are getting disconnected
> there, so we are not seeing the consistent results for
> postgres_fdw_get_connectionstest cases. If the connections are being
> used within the xact, then the valid option for those connections are
> being shown as false again making postgres_fdw_get_connections output
> inconsistent. This is what happened on the build farm member with
> CLOBBER_CACHE_ALWAYS build.

But if the issue is only the inconsistency of test results,
we can go with the option (2)? Even with (2), we can make the test
stable by removing "valid" column and executing
postgres_fdw_get_connections() within the transaction?

>
> So if we go with option (1), get rid of valid state from
> postgres_fdw_get_connectionstest and having the test cases inside an
> explicit xact block at the beginning of the postgres_fdw.sql test
> file, we don't see CLOBBER_CACHE_ALWAYS inconsistencies. I'm not sure
> if this is the correct way.
>
>> Regarding (1), as far as I understand correctly, even when the transaction
>> doesn't use foreign tables at all, it needs to scan the connection cache
>> entries if necessary. I was thinking to avoid this. I guess that this doesn't
>> work with at least the postgres_fdw 2PC patch that Sawada-san is proposing
>> because with the patch the commit/rollback callback is performed only
>> for the connections used in the transaction.
>
> You mean to say, pgfdw_xact_callback will not get called when the xact
> uses no foreign server connection or is it that pgfdw_xact_callback
> gets called but exits quickly from it? I'm not sure what the 2PC patch
> does.

Maybe it's chance to review the patch! ;P

BTW his patch tries to add new callback interfaces for commit/rollback of
foreign transactions, and make postgres_fdw use them instead of
XactCallback. And those new interfaces are executed only when
the transaction has started the foreign transactions.

Regards,

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