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

On Fri, Jan 29, 2021 at 11:38 AM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/01/29 14:53, Bharath Rupireddy wrote:
> > On Fri, Jan 29, 2021 at 10:55 AM Fujii Masao
> > <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >>>> BTW, even if we change pgfdw_inval_callback() so that it doesn't close
> >>>> the connection at all, ISTM that the results of postgres_fdw_get_connections()
> >>>> would not be stable because entry->invalidated would vary based on
> >>>> whether CLOBBER_CACHE_ALWAYS is used or not.
> >>>
> >>> Yes, after the above change (removing disconnect_pg_server in
> >>> pgfdw_inval_callback), our tests don't get stable because
> >>> postgres_fdw_get_connections shows the valid state of the connections.
> >>> I think we can change postgres_fdw_get_connections so that it only
> >>> shows the active connections server name but not valid state. Because,
> >>> the valid state is something dependent on the internal state change
> >>> and is not consistent with the user expectation but we are exposing it
> >>> to the user. Thoughts?
> >>
> >> I don't think that's enough because even the following simple
> >> queries return the different results, depending on whether
> >> CLOBBER_CACHE_ALWAYS is used or not.
> >>
> >> SELECT * FROM ft6; -- ft6 is the foreign table
> >> SELECT server_name FROM postgres_fdw_get_connections();
> >>
> >> When CLOBBER_CACHE_ALWAYS is used, postgres_fdw_get_connections()
> >> returns no records because the connection is marked as invalidated,
> >> and then closed at xact callback in SELECT query. Otherwise,
> >> postgres_fdw_get_connections() returns at least one connection that
> >> was established in the SELECT query.
> >
> > Right. In that case, after changing postgres_fdw_get_connections() so
> > that it doesn't output the valid state of the connections at all, we
>
> You're thinking to get rid of "valid" column? Or hide it from the test query
> (e.g., SELECT server_name from postgres_fdw_get_connections())?

I'm thinking we can get rid of the "valid" column from the
postgres_fdw_get_connections() function, not from the tests. Seems
like we are exposing some internal state(connection is valid or not)
which can change because of internal events. And also with the
existing postgres_fdw_get_connections(), the valid will always be true
if the user calls postgres_fdw_get_connections() outside an explicit
xact block, it can become false only when it's used in an explicit txn
block. So, the valid column may not be much useful for the user.
Thoughts?

> > can have all the new function test cases inside an explicit txn block.
> > So even if the clobber cache invalidates the connections, they don't
> > get closed until the end of main xact, the tests will be stable.
> > Thoughts?
>
> Also if there are cached connections before starting that transaction,
> they should be closed or established again before executing
> postgres_fdw_get_connections(). Otherwise, those connections are
> returned from postgres_fdw_get_connections() when
> CLOBBER_CACHE_ALWAYS is not used, but not when it's used.

Yes, we need to move the test to the place where cache wouldn't have
been initialized yet or no foreign connection has been made yet in the
session.

ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
\det+

<<<<<<<<<<<<MAY BE HERE>>>>>>>>>>>>

-- Test that alteration of server options causes reconnection
-- Remote's errors might be non-English, so hide them to ensure stable results
\set VERBOSITY terse
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work
ALTER SERVER loopback OPTIONS (SET dbname 'no such database');
SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should fail
DO $d$

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