Subject: RE: [Proposal] Add foreign-server health checks infrastructure

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 6 Mar 2023 11:56:42 +0000

Dear Katsuragi-san,

Thank you for reviewing! PSA new version.

> >> 4. the code of pqSocketPoll
> >> +#if defined(POLLRDHUP)
> >> +	if (forConnCheck)
> >> +		input_fd.events |= POLLRDHUP;
> >> +#endif
> >>
> >> I think it is better to use PQconnCheckable() to remove the macro.
> >
> > IIUC the macro is needed. In FreeBSD, macOS and other platforms do not
> > have the
> > macro POLLRDHUP so they cannot compile. I checked by my CI and got
> > following error.
> >
> > ```
> > ...
> > FAILED: src/interfaces/libpq/libpq.a.p/fe-misc.c.o
> > ...
> > ../src/interfaces/libpq/fe-misc.c:1149:22: error: use of undeclared
> > identifier 'POLLRDHUP'
> >                 input_fd.events |= POLLRDHUP;
> > ````
> >
> > It must be invisible from them.
> 
> Sorry, my mistake...

No issues :-).

> >> 9. the document of postgres_fdw
> >> The document of postgres_fdw_verify_connection_states_* is a little
> >> bit old. Could you update it?
> >
> > Updated. IIUC postgres_fdw_verify_connection_states returns
> >
> > * true, if the connection is verified.
> > * false, if the connection seems to be disconnected.
> > * NULL, if this is not the supported platform or connection has not
> > been established.
> >
> > And postgres_fdw_verify_connection_states_all returns
> >
> > * true if all the connections are verified.
> > * false, if one of connections seems to be disconnected.
> > * NULL, if this is not the supported platform or this backend has
> > never established connections
> 
> I think 'backend has never established connections' is a little bit
> strong.
> I think the following cases also return NULL. The case where a
> connection was established, however the connection is now closed
> by the postgres_fdw_disconnect() or something. NULL is also returned if
> the connection is invalidated. So, I think 'NULL, if no valid
> connection is established or the function is not supported on
> the platform.' is better. What do you think?

Disconnect functions have never been in my mind. Descriptions must be updated.

> Followings are my comments for v34. Please check.
> 
> 0001:
> 1. the document of pqConnCheck
> +       <literal>0</literal> if the socket is valid, and returns
> <literal>-1</literal>
> +       if the connection has already been closed or an error has
> occurred.
> 
> 1.1 if the socket is valid -> returns 0 if the 'connection is not
> closed'?

Fixed.

> 1.2 returns -1 if the connection has already been closed  <- Let me ask
> a question.
> Isn't this a situation where we would like to check using this
> function? Is 'error has occurred' insufficient?

Seems right, fixed.

> 2. the comment of pqSocketCheck
> + * means that the socket has not matched POLLRDHUP event and the socket
> has
> + * still survived.
> 
> socket has still survived -> connection is not closed by the socket
> peer?

Fixed.

> 3. the comment of pqSocketPoll
> + * returned and forConnCheck is requested, it means that the socket has
> not
> + * matched POLLRDHUP event and the socket has still survived.
> 
> socket has still survived -> connection is not closed by the socket
> peer?

Fixed.

> 0002:
> 4. the comment of verify_cached_connections
> + * This function emits warnings if a disconnection is found. This
> return true
> + * if disconnections cannot be found, otherwise return false.
> 
> return ture -> return's' true
> return false -> return's' false

Fixed.

> 5. the comment of postgres_fdw_verify_connection_states
> + * if the local session seems to be disconnected from other servers.
> NULL is
> + * returned if a connection to the specified foreign server has not
> been
> + * established yet, or this function is not available on this platform.
> 
> Considering the above discussion, 'NULL is returned if a valid
> connection to the specified foreign server is not established or
> this function...' seems better. What do you think?

Right, fixed.

> 6. the document of postgres_fdw_verify_connection_states
>   <literal>NULL</literal>
> +      is returned if a connection to the specified foreign server has
> not been
> +      established yet, or this function is not available on this
> platform
> 
> The same as comment no.5.

Right, fixed.

> 7. the document of postgres_fdw_verify_connection_states_all
> <literal>NULL</literal>
> +      is returned if the local session does not have connection caches,
> or this
> +      function is not available on this platform.
> 
> I think there is a case where a connection cache exists but valid
> connections do not exist and NULL is returned (disconnection case).
> Almost the same document as the postgres_fdw_verify_connection_states
> case (comment no.5) seems better.

Yes, but completely same statement cannot be used because these is not
specified foreign server. How about: 
NULL is returned if there are no established connections or this function ...

> 8. the document of postgres_fdw_can_verify_connection_states
> +      This function checks whether
> <function>postgres_fdw_verify_connection_states</function>
> +      and <function>postgres_fdw_verify_connection_states</function>
> work well
> 
> Should the latter (or former) postgres_fdw_verify_connection_states be
> postgres_fdw_verify_connection_states_all?

That was copy-and-paste error, fixed.

> regards,
> 
> --
> Katsuragi Yuta
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Tue, 07 Mar 2023 10:58:21 +0900

Hi Kuroda-san,

Thank you for updating the patch!

I think we can update the status to ready for committer after
this fix, if there is no objection.

> NULL is returned if there are no established connections or this 

Yes, to align with the postgres_fdw_verify_connection_states()
case, how about writing the connection is not valid. Like the
following?
'NULL is returned if no valid connections are established or
this function...'

This is my comment for v35. Please check.
0002:
1. the comment of verify_cached_connections (I missed one minor point.)
+ * This function emits warnings if a disconnection is found. This 
returns true
+ * if disconnections cannot be found, otherwise returns false.

I think false is returned only if disconnections are found and
true is returned in all other cases. So, modifying the description
like the following seems better.
'This returns false if disconnections are found, otherwise
returns true.'

regards,