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

On Mon, Dec 14, 2020 at 9:38 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
wrote:
> On 2020/12/12 15:05, Bharath Rupireddy wrote:
> > On Sat, Dec 12, 2020 at 12:19 AM Fujii Masao <
masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)com>> wrote:
> > > I was thinking that in the case of drop of user mapping or server,
hash_search(ConnnectionHash) in GetConnection() cannot find the cached
connection entry invalidated by that drop. Because "user->umid" used as
hash key is changed. So I was thinking that that invalidated connection
will not be closed nor reconnected.
> > >
> >
> > You are right in saying that the connection leaks.
> >
> > Use case 1:
> > 1) Run foreign query in session1 with server1, user mapping1
> > 2) Drop user mapping1 in another session2, invalidation message gets
logged which will have to be processed by other sessions
> > 3) Run foreign query again in session1, at the start of txn, the cached
entry gets invalidated via pgfdw_inval_callback(). Whatever may be the type
of foreign query (select, update, explain, delete, insert, analyze etc.),
upon next call to GetUserMapping() from postgres_fdw.c, the cache lookup
fails(with ERROR: user mapping not found for "XXXX") since the user
mapping1 has been dropped in session2 and the query will also fail before
reaching GetConnection() where the connections associated with invalidated
entries would have got disconnected.
> >
> > So, the connection associated with invalidated entry would remain until
the local session exits which is a problem to solve.
> >
> > Use case 2:
> > 1) Run foreign query in session1 with server1, user mapping1
> > 2) Try to drop foreign server1, then we would not be allowed to do so
because of dependency. If we use CASCADE, then the dependent user mapping1
and foreign tables get dropped too [1].
> > 3) Run foreign query again in session1, at the start of txn, the cached
entry gets invalidated via pgfdw_inval_callback(), it fails because there
is no foreign table and user mapping1.
> >
> > But, note that the connection remains open in session1, which is again
a problem to solve.
> >
> > To solve the above connection leak problem, it looks like the right
place to close all the invalid connections is pgfdw_xact_callback(), once
registered, which gets called at the end of every txn in the current
session(by then all the sub txns also would have been finished). Note that
if there are too many invalidated entries, then one of the following txn
has to bear running this extra code, but that's okay than having leaked
connections. Thoughts? If okay, I can code a separate patch.
>
> Thanks for further analysis! Sounds good. Also +1 for making it as
separate patch. Maybe only this patch needs to be back-patched.

Thanks. Yeah once agreed on the fix, +1 to back patch. Shall I start a
separate thread for connection leak issue and patch, so that others might
have different thoughts??

> > static void
> > pgfdw_xact_callback(XactEvent event, void *arg)
> > {
> > HASH_SEQ_STATUS scan;
> > ConnCacheEntry *entry;
> > * /* HERE WE CAN LOOK FOR ALL INVALIDATED ENTRIES AND DISCONNECT
THEM*/*
>
> This may cause the connection to be closed before sending COMMIT
TRANSACTION command to the foreign server, i.e., the connection is closed
in the middle of the transaction. So as you explained before, we should
avoid that? If this my understanding is right, probably the connection
should be closed after COMMIT TRANSACTION command is sent to the foreign
server. What about changing the following code in pgfdw_xact_callback() so
that it closes the connection even when it's marked as invalidated?

You are right! I'm posting what I have in my mind for fixing this
connection leak problem.

/* tracks whether any work is needed in callback functions */
static bool xact_got_connection = false;
/* tracks whether there exists at least one invalid connection in the
connection cache */
*static bool invalid_connections_exist = false;*

static void
pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue)
{

/* hashvalue == 0 means a cache reset, must clear all state */
if (hashvalue == 0 ||
(cacheid == FOREIGNSERVEROID &&
entry->server_hashvalue == hashvalue) ||
(cacheid == USERMAPPINGOID &&
entry->mapping_hashvalue == hashvalue))
{
entry->invalidated = true;
* invalid_connections_exist = true;*
}

static void
pgfdw_xact_callback(XactEvent event, void *arg)
{
HASH_SEQ_STATUS scan;
ConnCacheEntry *entry;

/* Quick exit if no connections were touched in this transaction or
there are no invalid connections in the cache. */
if (!xact_got_connection *&& !invalid_connections_exist)*
return;

/*
* If the connection isn't in a good idle state, discard it to
* recover. Next GetConnection will open a new connection.
*/
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
entry->changing_xact_state ||
* entry->invalidated)*
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);
}

/*
* Regardless of the event type, we can now mark ourselves as out of the
* transaction. (Note: if we are here during PRE_COMMIT or PRE_PREPARE,
* this saves a useless scan of the hashtable during COMMIT or PREPARE.)
*/
xact_got_connection = false;

/* We are done with closing all the invalidated connections so reset. */
*invalid_connections_exist = false;*
}

> > And we can also extend postgres_fdw_disconnect() something like.
> >
> > postgres_fdw_disconnect(bool invalid_only) --> default for invalid_only
false. disconnects all connections. when invalid_only is set to true then
disconnects only invalid connections.
> > postgres_fdw_disconnect('server_name') --> disconnections connections
associated with the specified foreign server
> >
> > Having said this, I'm not in favour of invalid_only flag, because if we
choose to change the code in pgfdw_xact_callback to solve connection leak
problem, we may not need this invalid_only flag at all, because at the end
of txn (even for the txns in which the queries fail with error,
pgfdw_xact_callback gets called), all the existing invalid connections get
disconnected. Thoughts?
>
> +1 not to have invalid_only flag. On the other hand, I think that
postgres_fdw_get_connections() should return all the cached connections
including invalidated ones. Otherwise, the number of connections observed
via postgres_fdw_get_connections() may be different from the number of
connections actually established, and which would be confusing to users.
>

If postgres_fdw_get_connections() has to return invalidated connections, I
have few things mentioned in [1] to be clarified. Thoughts? Please have a
look at the below comment before we decide to show up the invalid entries
or not.

[1] -
https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com

> BTW, even after fixing the connection-leak issue,
postgres_fdw_get_connections() may see invalidated cached connections when
it's called during the transaction.

We will not output if the invalidated entry has no active connection[2], so
if we fix the connection leak issue with the above discussed fix i.e
closing all the invalidated connections at the end of next xact, there are
less chances that we will output invalidated entries in the
postgres_fdw_get_connections() output. Only case we may show up invalidated
connections(which have active connections entry->conn) in the
postgres_fdw_get_connections() output is as follows:

1) say we have few cached active connections exists in session 1
2) drop the user mapping (in another session) associated with any of the
cached connections to make that entry invalid
3) run select * from postgres_fdw_get_connections(); in session 1. At the
start of the xact, the invalidation message gets processed and the
corresponding entry gets marked as invalid. If we allow invalid connections
(that have entry->conn) to show up in the output, then we show them in the
result of the query. At the end of xact, we close these invalid
connections, in this case, user might think that he still have invalid
connections active.

If the query ran in 3) is not postgres_fdw_get_connections() and something
else, then postgres_fdw_get_connections() will never get to show invalid
connections as they would have closed the connections.

IMO, better not choose the invalid connections to show up in the
postgres_fdw_get_connections() output, if we fix the connection leak issue
with the above discussed fix i.e closing all the invalidated connections at
the end of next xact

[2]
+Datum
+postgres_fdw_get_connections(PG_FUNCTION_ARGS)
+{
+ ArrayBuildState *astate = NULL;
+
+ if (ConnectionHash)
+ {
+ HASH_SEQ_STATUS scan;
+ ConnCacheEntry *entry;
+
+ hash_seq_init(&scan, ConnectionHash);
+ while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
+ {
+ Form_pg_user_mapping umap;
+ HeapTuple umaptup;
+ Form_pg_foreign_server fsrv;
+ HeapTuple fsrvtup;
+
+ /* We only look for active and open remote connections. */
+ if (!entry->conn)
+ continue;

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