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

On Wed, Dec 30, 2020 at 5:20 PM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
>
> On 2020-12-30 09:10, Bharath Rupireddy wrote:
> > Hi,
> >
> > I'm posting a v4-0001 patch for the new functions
> > postgres_fdw_get_connections() and postgres_fdw_disconnect().  In this
> > patch, I tried to address the review comments provided upthread.
> >
> > Thoughts?
> >
>
> I still have some doubts that it is worth of allowing to call
> postgres_fdw_disconnect() in the explicit transaction block, since it
> adds a lot of things to care and check for. But otherwise current logic
> looks solid.
>
> +                                errdetail("Such connections get closed either in the next use or
> at the end of the current transaction.")
> +                                : errdetail("Such connection gets closed either in the next use or
> at the end of the current transaction.")));
>
> Does it really have a chance to get closed on the next use? If foreign
> server is dropped then user mapping should be dropped as well (either
> with CASCADE or manually), but we do need user mapping for a local cache
> lookup. That way, if I understand all the discussion up-thread
> correctly, we can only close such connections at the end of xact, do we?

The next use of such a connection in the following query whose foreign
server would have been dropped fails because of the cascading that can
happen to drop the user mapping and the foreign table as well. During
the start of the next query such connection will be marked as
invalidated because xact_depth of that connection is > 1 and when the
fail happens, txn gets aborted due to which pgfdw_xact_callback gets
called and in that the connection gets closed. To make it more clear,
please have a look at the scenarios [1].

I still feel the detailed message "Such connections get closed either
in the next use or at the end of the current transaction" is
appropriate. Please have a closer look at the possible use cases [1].

And IMO, anyone dropping a foreign server inside an explicit txn block
in which the foreign server was used is extremely rare, so still
showing this message and allowing postgres_fdw_disconnect() in
explicit txn block is useful. For all other cases the
postgres_fdw_disconnect behaves as expected.

Thoughts?

[1]
case 1:
BEGIN;
SELECT 1 FROM f1 LIMIT 1;  --> xact_depth becomes 1
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets invalidated in
pgfdw_inval_callback because xact_depth is 1
SELECT 1 FROM f1 LIMIT 1; --> since the failure occurs for this query
and txn is aborted, the connection gets closed in pgfdw_xact_callback.
SELECT * FROM postgres_fdw_get_connections(); --> txn was aborted
SELECT * FROM postgres_fdw_disconnect(); --> txn was aborted
COMMIT;

case 2:
BEGIN;
SELECT 1 FROM f1 LIMIT 1;  --> xact_depth becomes 1
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets invalidated in
pgfdw_inval_callback because xact_depth is 1
SELECT * FROM postgres_fdw_get_connections(); --> shows the above
warning because foreign server name can not be fetched
SELECT * FROM postgres_fdw_disconnect(); --> the connection can not be
closed here as well because xact_depth is 1, then it issues a warning
"cannot close any connection because they are still in use"
COMMIT; --> finally the connection gets closed here in pgfdw_xact_callback.

case 3:
SELECT 1 FROM f1 LIMIT 1;
BEGIN;
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets closed in
pgfdw_inval_callback because xact_depth is 0
SELECT 1 FROM f1 LIMIT 1; --> since the failure occurs for this query
and the connection was closed previously then the txn gets aborted
SELECT * FROM postgres_fdw_get_connections(); --> txn was aborted
SELECT * FROM postgres_fdw_disconnect(); --> txn was aborted
COMMIT;

> + * This function returns false if the cache doesn't exist.
> + * When the cache exists:
>
> I think that this will be corrected later by pg_indent, but still. In
> this comment section following points 1) and 2) have a different
> combination of tabs/spaces.

I can change that in the next version.

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