Re: postgres_fdw - cached connection leaks if the associated user mapping/foreign server is dropped

On Fri, Dec 18, 2020 at 6:46 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> On Fri, Dec 18, 2020 at 6:39 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 18, 2020 at 5:06 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> > > I have an issue about the existing testcase.
> > >
> > > """
> > > -- Test that alteration of server options causes reconnection
> > > 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$
> > > BEGIN
> > > EXECUTE $$ALTER SERVER loopback
> > > OPTIONS (SET dbname '$$||current_database()||$$')$$;
> > > END;
> > > $d$;
> > > SELECT c3, c4 FROM ft1 ORDER BY c3, c1 LIMIT 1; -- should work again
> > > """
> > >
> > > IMO, the above case is designed to test the following code[1]:
> > > With the patch, it seems the following code[1] will not work for this case, right?
> > > (It seems the connection will be disconnect in pgfdw_xact_callback)
> > >
> > > I do not know does it matter, or should we add a testcase to cover that?
> > >
> > > [1] /*
> > > * If the connection needs to be remade due to invalidation, disconnect as
> > > * soon as we're out of all transactions.
> > > */
> > > if (entry->conn != NULL && entry->invalidated && entry->xact_depth == 0)
> > > {
> > > elog(DEBUG3, "closing connection %p for option changes to take effect",
> > > entry->conn);
> > > disconnect_pg_server(entry);
> > > }
> >
> > Yes you are right. With the patch if the server is altered in the same
> > session in which the connection exists, the connection gets closed at
> > the end of that alter query txn, not at the beginning of the next
> > foreign tbl query. So, that part of the code in GetConnection()
> > doesn't get hit. Having said that, this code gets hit when the alter
> > query is run in another session and the connection in the current
> > session gets disconnected at the start of the next foreign tbl query.
> >
> > If we want to cover it with a test case, we might have to alter the
> > foreign server in another session. I'm not sure whether we can open
> > another session from the current psql session and run a sql command.

I further checked on how we can add/move the test case( that is
altering server options in a different session and see if the
connection gets disconnected at the start of the next foreign query in
the current session ) to cover the above code. Upon some initial
analysis, it looks like it is possible to add that under
src/test/isolation tests. Another way could be to add it using the TAP
framework under contrib/postgres_fdw. Having said that, currently
these two places don't have any postgres_fdw related tests, we will be
the first ones to add.

I'm not quite sure whether that's okay or is there any better way of
doing it. Thoughts?

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