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

On 2021/01/21 14:46, Bharath Rupireddy wrote:
> 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?

Thanks for thinking this. But at least for me, "if (!all)" looks not so confusing.
And the comment seems to explain why we can end the scan.

Regards,

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