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

On 2020/12/14 14:36, Bharath Rupireddy wrote:
> On Mon, Dec 14, 2020 at 9:38 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto: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> <mailto: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??

Yes, of course!

>
> > > 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 <https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com>

I was thinking to display the records having the columns for server name and boolean flag indicating whether it's invalidated or not. But I'm not sure if this is the best design for now. Probably we should revisit this after determining how to fix the connection-leak issue.

>
> > 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.

What about the case where the transaction started at the above 1) at session 1, and postgres_fdw_get_connections() in the above 3) is called within that transaction at session 1? In this case, postgres_fdw_get_connections() can return even invalidated connections?

>
> 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 <http://www.enterprisedb.com>
>

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION