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

On 2021/01/21 16:16, Bharath Rupireddy wrote:
> On Thu, Jan 21, 2021 at 12:17 PM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> 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.
>
> May I know if it's okay to have the boolean can_terminate_scan as shown in [1]?

My opinion is to check "!all", but if others prefer using such boolean flag,
I'd withdraw my opinion.

+ if ((all || entry->server_hashvalue == hashvalue) &&

What about making disconnect_cached_connections() accept serverid instead
of hashvalue, and perform the above comparison based on serverid? That is,
I'm thinking "if (all || entry->serverid == serverid)". If we do that, we can
simplify postgres_fdw_disconnect() a bit more by getting rid of the calculation
of hashvalue.

+ if ((all || entry->server_hashvalue == hashvalue) &&
+ entry->conn)

I think that it's better to make the check of "entry->conn" independent
like other functions in postgres_fdw/connection.c. What about adding
the following check before the above?

/* Ignore cache entry if no open connection right now */
if (entry->conn == NULL)
continue;

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

As this comment explains, even when the connection is used in the transaction,
its server can be dropped in the same transaction. The connection can remain
until the end of transaction even though its server has been already dropped.
I'm now wondering if this behavior itself is problematic and should be forbid.
Of course, this is separate topic from this patch, though..

BTW, my just idea for that is;
1. change postgres_fdw_get_connections() return also serverid and xact_depth.
2. make postgres_fdw define the event trigger on DROP SERVER command so that
an error is thrown if the connection to the server is still in use.
The event trigger function uses postgres_fdw_get_connections() to check
if the server connection is still in use or not.

I'm not sure if this just idea is really feasible or not, though...

Regards,

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