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

On 2021/01/21 12:00, Bharath Rupireddy wrote:
> On Wed, Jan 20, 2021 at 6:58 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> + * It checks if the cache has a connection for the given foreign server that's
>> + * not being used within current transaction, then returns true. If the
>> + * connection is in use, then it emits a warning and returns false.
>>
>> The comment also should mention the case where no open connection
>> for the given server is found? What about rewriting this to the following?
>>
>> ---------------------
>> If the cached connection for the given foreign server is found and has not
>> been used within current transaction yet, close the connection and return
>> true. Even when it's found, if it's already used, keep the connection, emit
>> a warning and return false. If it's not found, return false.
>> ---------------------
>
> Done.
>
>> + * It returns true, if it closes at least one connection, otherwise false.
>> + *
>> + * It returns false, if the cache doesn't exit.
>>
>> The above second comment looks redundant.
>
> Yes. "otherwise false" means it.
>
>> + if (ConnectionHash)
>> + result = disconnect_cached_connections(0, true);
>>
>> Isn't it smarter to make disconnect_cached_connections() check
>> ConnectionHash and return false if it's NULL? If we do that, we can
>> simplify the code of postgres_fdw_disconnect() and _all().
>
> Done.
>
>> + * current transaction are disconnected. Otherwise, the unused entries with the
>> + * given hashvalue are disconnected.
>>
>> In the above second comment, a singular form should be used instead?
>> Because there must be no multiple entries with the given hashvalue.
>
> Rephrased the function comment a bit. Mentioned the _disconnect and
> _disconnect_all in comments because we have said enough what each of
> those two functions do.
>
> +/*
> + * Workhorse to disconnect cached connections.
> + *
> + * This function disconnects either all unused connections when called from
> + * postgres_fdw_disconnect_all or a given foreign server unused connection when
> + * called from postgres_fdw_disconnect.
> + *
> + * This function returns true if at least one connection is disconnected,
> + * otherwise false.
> + */
>
>> + server = GetForeignServer(entry->serverid);
>>
>> This seems to cause an error "cache lookup failed"
>> if postgres_fdw_disconnect_all() is called when there is
>> a connection in use but its server is dropped. To avoid this error,
>> GetForeignServerExtended() with FSV_MISSING_OK should be used
>> instead, like postgres_fdw_get_connections() does?
>
> +1. So, I changed it to GetForeignServerExtended, added an assertion
> for invalidation just like postgres_fdw_get_connections. I also added
> a test case for this, we now emit a slightly different warning for
> this case alone that is (errmsg("cannot close dropped server
> connection because it is still in use")));. This warning looks okay as
> we cannot show any other server name in the ouput and we know that
> this rare case can exist when someone drops the server in an explicit
> transaction.
>
>> + 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.

Regards,

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