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

On Wed, Dec 23, 2020 at 7:31 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> I agree to make pgfdw_xact_callback() close the connection when
> entry->invalidated == true. But I think that it's better to get rid of
> have_invalid_connections flag and make pgfdw_inval_callback() close
> the connection immediately if entry->xact_depth == 0, to avoid unnecessary
> scan of the hashtable during COMMIT of transaction not accessing to
> foreign servers. Attached is the POC patch that I'm thinking. Thought?

We could do that way as well. It seems okay to me. Now the disconnect
code is spread in pgfdw_inval_callback() and pgfdw_xact_callback().
There's also less burden on pgfdw_xact_callback() to close a lot of
connections on a single commit. The behaviour is like this - If
entry->xact_depth == 0, then the entries wouldn't have got any
connection in the current txn so they can be immediately closed in
pgfdw_inval_callback() and pgfdw_xact_callback() can exit immediately
as xact_got_connection is false. If entry->xact_depth > 0 which means
that probably pgfdw_inval_callback() came from a sub txn, we would
have got a connection in the txn i.e. xact_got_connection becomes true
due to which it can get invalidated in pgfdw_inval_callback() and get
closed in pgfdw_xact_callback() at the end of the txn.

And there's no chance of entry->xact_depth > 0 and xact_got_connection false.

I think we need to change the comment before pgfdw_inval_callback() a bit:

* After a change to a pg_foreign_server or pg_user_mapping catalog entry,
* mark connections depending on that entry as needing to be remade.
* We can't immediately destroy them, since they might be in the midst of
* a transaction, but we'll remake them at the next opportunity.

to

* After a change to a pg_foreign_server or pg_user_mapping catalog entry,
* try to close the cached connections associated with them if they
are not in the
* midst of a transaction otherwise mark them as invalidated. We will
destroy the
* invalidated connections in pgfdw_xact_callback() at the end of the main xact.
* Closed connections will be remade at the next opportunity.

Any thoughts on the earlier point in [1] related to a test case(which
becomes unnecessary with this patch) coverage?

[1] - https://www.postgresql.org/message-id/CALj2ACXymb%3Dd4KeOq%2Bjnh_E6yThn%2Bcf1CDRhtK_crkj0_hVimQ%40mail.gmail.com

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