Dear Katsuragi-san, Thank you for reviewing! PSA new version patches.
> 0001: > + while (result < 0 && errno == EINTR); > + > + if (result < 0) > + return -1; > > this `return -1` is not indented properly. This part is no longer needed. Please see another discussion[1]. > 0002: > + <term><function>postgres_fdw_verify_connection_states(server_name > text) returns boolean</function></term> > ... > + extension to the <symbol>poll</symbol> system call, including > Linux. This > + returns <literal>true</literal> if checked connections are still > valid, > + or the checking is not supported on this platform. > <literal>false</literal> > + is returned if the local session seems to be disconnected from > other > + servers. Example usage of the function: > > Here, 'still valid' seems a little bit confusing because this 'valid' is > not > the same as postgres_fdw_get_connections's 'valid' [1]. > > Should 'still valid' be 'existing connection is not closed by the remote > peer'? > But this description does not cover all the cases where this function > returns true... > I think one choice is to write all the cases like 'returns true if any > of the > following condition is satisfied. 1) existing connection is not closed > by the > remote peer 2) there is no connection for specified server yet 3) the > checking > is not supported...'. If my understanding is not correct, please point > it out. Modified like you pointed out. > BTW, is it reasonable to return true if ConnectionHash is not > initialized or > there is no ConnCacheEntry for specified remote server? What do you > think > about returning NULL in that case? I'm not sure which one is better, but modified accordingly. > 0003: > I think it is better that the test covers all the new functions. > How about adding a test for postgres_fdw_verify_connection_states_all? > > > +-- > =================================================== > ================ > +-- test for postgres_fdw_verify_foreign_servers function > +-- > =================================================== > ================ > > Writing all the functions' name like 'test for > postgres_fdw_verify_connection_states > and postgres_fdw_can_verify_connection_states' looks straightforward. > What do you think about this? Added. > 0004: > Sorry, I have not read 0004. I will read. No problem:-). [1]: https://www.postgresql.org/message-id/TYAPR01MB58664E039F45959AB321FA1FF5D99%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best Regards, Hayato Kuroda FUJITSU LIMITED
v31-0001-Add-PQconnCheck-and-PQcanConnCheck-to-libpq.patch
Description: v31-0001-Add-PQconnCheck-and-PQcanConnCheck-to-libpq.patch
v31-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
Description: v31-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
v31-0003-add-test.patch
Description: v31-0003-add-test.patch
v31-0004-add-kqueue-support-for-PQconnCheck-and-PQcanConn.patch
Description: v31-0004-add-kqueue-support-for-PQconnCheck-and-PQcanConn.patch
