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

On Wed, Dec 9, 2020 at 4:49 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I started reviewing 0001 patch.
>

Thanks!

> IMO the 0001 patch should be self-contained so that we can commit it at first. That is, I think that it's better to
movethe documents and tests for the functions 0001 patch adds from 0004 to 0001. 
>

+1. I will make each patch self-contained in the next version which I
plan to submit soon.

> Since 0001 introduces new user-visible functions into postgres_fdw, the version of postgres_fdw should be increased?
>

Yeah looks like we should do that, dblink has done that when it
introduced new functions. In case the new functions are not required
for anyone, they can choose to go back to 1.0.

Should we make the new version as 1.1 or 2.0? I prefer to make it 1.1
as we are just adding few functionality over 1.0. I will change the
default_version from 1.0 to the 1.1 and add a new
postgres_fdw--1.1.sql file.

If okay, I will make changes to 0001 patch.

> The similar code to get the server name from cached connection entry exists also in
pgfdw_reject_incomplete_xact_state_change().I'm tempted to make the "common" function for that code and use it both in
postgres_fdw_get_connections()and pgfdw_reject_incomplete_xact_state_change(), to simplify the code. 
>

+1. I will move the server name finding code to a new function, say
char *pgfdw_get_server_name(ConnCacheEntry *entry);

> +                       /* 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);

Can we reload the sys cache entries of USERMAPPINGOID (if there is a
way) for invalid connections in our new function and then do a look
up? If not, another way could be storing the associated server name or
oid in the ConnCacheEntry. Currently we store user mapping oid(in
key), its hash value(in mapping_hashvalue) and foreign server oid's
hash value (in server_hashvalue). If we have the foreign server oid,
then we can just look up for the server name, but I'm not quite sure
whether we get the same issue i.e. invalid tuples when the entry gets
invalided (via pgfdw_inval_callback) due to some change in foreign
server options.

IMHO, we can simply choose to show all the active, valid connections. 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 (all)
> +       {
> +               hash_destroy(ConnectionHash);
> +               ConnectionHash = NULL;
> +               result = true;
> +       }
>
> Could you tell me why ConnectionHash needs to be destroyed?
>

Say, in a session there are hundreds of different foreign server
connections made and if users want to disconnect all of them with the
new function and don't want any further foreign connections in that
session, they can do it. But then why keep the cache just lying around
and holding those many entries? Instead we can destroy the cache and
if necessary it will be allocated later on next foreign server
connections.

IMHO, it is better to destroy the cache in case of disconnect all,
hoping to save memory, thinking that (next time if required) the cache
allocation doesn't take much time. Thoughts?

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