Hi Kuroda-san,
Thank you for updating the patch!
On 2023-02-20 15:42, Hayato Kuroda (Fujitsu) wrote:
> Dear Katsuragi-san,
>
> Thank you for reviewing! PSA new version.
I rethought the pqSocketPoll part. Current interpretation of
arguments seems a little bit confusing because a specific pattern
of arguments has a different meaning. What do you think about
introducing a new argument like `int forConnCheck`? This seems
straightforward and readable.
>> 0002:
>> As for the return value of postgres_fdw_verify_connection_states,
>> what do you think about returning NULL when connection-checking
>> is not performed? I think there are two cases 1) ConnectionHash
>> is not initialized or 2) connection is not found for specified
>> server name, That is, no entry passes the first if statement below
>> (case 2)).
>>
>> ```
>> if (all || entry->serverid == serverid)
>> {
>> if (PQconnCheck(entry->conn))
>> {
>> ```
>
> I think in that case we can follow postgres_fdw_disconnect().
> About postgres_fdw_disconnect(), if the given server_name does not
> exist,
> an error is reported.
Yes, I think this error is fine.
I think there are cases where the given server name does exist,
however the connection check is not performed (does not pass the
first if statement above). 1) a case where the server name exist,
however an open connection to the specified server is not found.
2) case where connection for specified server is invalidated.
3) case where there is not ConnectionHash entry for the specified
server. Current implementation returns true in that case, however
the check is not performed. Suppose the checking mechanism is
supported on the platform, it does not seem reasonable to return
true or false when the check is not performed. What do you think?
regards,
--
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
