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

On Thu, Jan 21, 2021 at 10:06 AM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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