Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
От | Fujii Masao |
---|---|
Тема | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit |
Дата | |
Msg-id | 8a6d3620-11de-30c8-7647-375cf92f442a@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Ответы |
Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
(Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
|
Список | pgsql-hackers |
On 2021/01/29 16:12, Bharath Rupireddy wrote: > On Fri, Jan 29, 2021 at 12:36 PM Fujii Masao > <masao.fujii@oss.nttdata.com> wrote: >> On 2021/01/29 15:44, Bharath Rupireddy wrote: >>> On Fri, Jan 29, 2021 at 11:54 AM Fujii Masao >>> <masao.fujii@oss.nttdata.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 callingpostgres_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
В списке pgsql-hackers по дате отправления:
Следующее
От: Bharath RupireddyДата:
Сообщение: Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit