Dear Katsuragi-san,
Thank you for reviewing! New patch set can be available on [1].
> 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.
I think it may be better, so fixed.
But now we must consider another thing - will we support combination of conncheck
and {read|write} or timeout? Especially about timeout, if someone calls pqSocketPoll()
with forConnCheck = 1 and end_time = -1, the process may be stuck because it waits
till socket peer closed connection.
Currently the forConnCheck can be specified with other request, but timeout must be zero.
> 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?
Thank you for detailed explanation. I agreed your opinion and modified like that.
While making the patch, I come up with idea that postgres_fdw_verify_connection_states*
returns NULL if PQconnCheck() cannot work on this platform. This can be done by
adding PQconnCheckable(). It may be reasonable because the checking is not really
done on this environment. How do you think?
[1]:
https://www.postgresql.org/message-id/TYAPR01MB586664F5ED2128E572EA95B0F5AA9%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
