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

On Thu, Dec 10, 2020 at 7:14 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> > +                       /* We only look for active and open remote connections. */
> > +                       if (entry->invalidated || !entry->conn)
> > +                               continue;
> >
> > We should return even invalidated entry because it has still cached connection?
> >
>
> I checked this point earlier, for invalidated connections, the tuple
> returned from the cache is also invalid and the following error will
> be thrown. So, we can not get the server name for that user mapping.
> Cache entries too would have been invalidated after the connection is
> marked as invalid in pgfdw_inval_callback().
>
> umaptup = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(entry->key));
> if (!HeapTupleIsValid(umaptup))
>       elog(ERROR, "cache lookup failed for user mapping with OID %u",
> entry->key);
>

I further checked on returning invalidated connections in the output
of the function. Actually, the reason I'm seeing a null tuple from sys
cache (and hence the error "cache lookup failed for user mapping with
OID xxxx") for an invalidated connection is that the user mapping
(with OID entry->key that exists in the cache) is getting dropped, so
the sys cache returns null tuple. The use case is as follows:

1) Create a server, role, and user mapping of the role with the server
2) Run a foreign table query, so that the connection related to the
server gets cached
3) Issue DROP OWNED BY for the role created, since the user mapping is
dependent on that role, it gets dropped from the catalogue table and
an invalidation message will be pending to clear the sys cache
associated with that user mapping.
4) Now, if I do select * from postgres_fdw_get_connections() or for
that matter any query, at the beginning the txn
AtStart_Cache()-->AcceptInvalidationMessages()-->pgfdw_inval_callback()
gets called and marks the cached entry as invalidated. Remember the
reason for this invalidation message is that the user mapping with the
OID entry->key is dropped from 3). Later in
postgres_fdw_get_connections(), when we search the sys cache with
entry->key for that invalidated connection, since the user mapping is
dropped from the system, null tuple is returned.

If we were to show invalidated connections in the output of
postgres_fdw_get_connections(), we can ignore the entry and continue
further if the user mapping sys cache search returns null tuple:

umaptup = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(entry->key));

if (!HeapTupleIsValid(umaptup))
   continue;

Thoughts?

> > Also this makes me wonder if we should return both the server name and boolean flag indicating whether it's
invalidatedor not. If so, users can easily find the invalidated connection entry and disconnect it because there is no
needto keep invalidated connection. 
> >
>
> Currently we are returning a list of foreing server names with whom
> there exist active connections. If we somehow address the above
> mentioned problem for invalid connections and choose to show them as
> well, then how should our output look like? Is it something like we
> prepare a list of pairs (servername, validflag)?

If agreed on above point, we can output something like: (myserver1,
valid), (myserver2, valid), (myserver3, invalid), (myserver4, valid)
....

Thoughts?

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