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

On 2021/01/29 16:12, Bharath Rupireddy wrote:
> On Fri, Jan 29, 2021 at 12:36 PM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> 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?
>
> Hmmm, and we should have the tests at the start of the file
> postgres_fdw.sql before even we make any foreign server connections.

We don't need to move the test if we always call postgres_fdw_disconnect_all() just before starting new transaction and calling postgres_fdw_get_connections() as follows?

SELECT 1 FROM postgres_fdw_disconnect_all();
BEGIN;
...
SELECT * FROM postgres_fdw_get_connections();
...

>
> If okay, I can prepare the patch and run with clobber cache build locally.

Many thanks!

>
>>>
>>> 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.
>
> IMHO, it's better to keep it as a separate discussion.

Yes, of course!

Regards,

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