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

On Thu, Jan 21, 2021 at 10:06 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
 > >> +                       if (entry->server_hashvalue == hashvalue &&
> >> +                               (entry->xact_depth > 0 || result))
> >> +                       {
> >> +                               hash_seq_term(&scan);
> >> +                               break;
> >>
> >> entry->server_hashvalue can be 0? If yes, since postgres_fdw_disconnect_all()
> >> specifies 0 as hashvalue, ISTM that the above condition can be true
> >> unexpectedly. Can we replace this condition with just "if (!all)"?
> >
> > I don't think so entry->server_hashvalue can be zero, because
> > GetSysCacheHashValue1/CatalogCacheComputeHashValue will not return 0
> > as hash value. I have not seen someone comparing hashvalue with an
> > expectation that it has 0 value, for instance see if (hashvalue == 0
> > || riinfo->oidHashValue == hashvalue).
> >
> >   Having if(!all) something like below there doesn't suffice because we
> > might call hash_seq_term, when some connection other than the given
> > foreign server connection is in use.
>
> No because we check the following condition before reaching that code. No?
>
> +               if ((all || entry->server_hashvalue == hashvalue) &&
>
>
> I was thinking that "(entry->xact_depth > 0 || result))" condition is not
> necessary because "result" is set to true when xact_depth <= 0 and that
> condition always indicates true.

I think that condition is too confusing. How about having a boolean
can_terminate_scan like below?

    while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
    {
        bool    can_terminate_scan = false;

        /*
         * Either disconnect given or all the active and not in use cached
         * connections.
         */
        if ((all || entry->server_hashvalue == hashvalue) &&
             entry->conn)
        {
            /* We cannot close connection that's in use, so issue a warning. */
            if (entry->xact_depth > 0)
            {
                ForeignServer *server;

                if (!all)
                    can_terminate_scan = true;

                server = GetForeignServerExtended(entry->serverid,
                                                  FSV_MISSING_OK);

                if (!server)
                {
                    /*
                     * If the server has been dropped in the current explicit
                     * transaction, then this entry would have been invalidated
                     * in pgfdw_inval_callback at the end of drop sever
                     * command. Note that this connection would not have been
                     * closed in pgfdw_inval_callback because it is still being
                     * used in the current explicit transaction. So, assert
                     * that here.
                     */
                    Assert(entry->invalidated);

                    ereport(WARNING,
                            (errmsg("cannot close dropped server
connection because it is still in use")));
                }
                else
                    ereport(WARNING,
                            (errmsg("cannot close connection for
server \"%s\" because it is still in use",
                             server->servername)));
            }
            else
            {
                elog(DEBUG3, "discarding connection %p", entry->conn);
                disconnect_pg_server(entry);
                result = true;

                if (!all)
                    can_terminate_scan = true;
            }

            /*
             * For the given server, if we closed connection or it is still in
             * use, then no need of scanning the cache further.
             */
            if (can_terminate_scan)
            {
                hash_seq_term(&scan);
                break;
            }
        }
    }

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