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

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.

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

> >
> > 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. I will try to
review that patch later.

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