Re: [Proposal] Add foreign-server health checks infrastructure

On Mon, Dec 11, 2023 at 2:08 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Fujii-san, Tom,
>
> Thank you for giving a suggestion! PSA new version.
>
> > Regarding 0001 patch, on second thought, to me, it seems odd to expose
> > a function that doesn't have anything to directly do with PostgreSQL
> > as a libpq function. The function simply calls poll() on the socket
> > with POLLRDHUP if it is supported. While it is certainly convenient to
> > have this function, I'm not sure that it fits within the scope of libpq.
> > Thought?
>
> Current style is motivated by Onder [1] and later discussions. I thought it might
> be useful for other developers, but OK, I can remove changes on libpq modules.
> Horiguchi-san has suggested [2] that it might be overkill to use WaitEventSet()
> mechanism, so I kept using poll().
> I reused the same naming as previous version because they actually do something
> Like libpq, but better naming is very welcome.
>
> > Regarding 0002 patch, the behavior of postgres_fdw_verify_connection_states()
> > in regards to multiple connections using different user mappings seems
> > not well documented. The function seems to return false if either of
> > those connections has been closed.
>
> I did not considered the situation because I have not came up with the situation
> that only one of connections to the same foreign server is broken.
>
> > This behavior means that it's difficult to identify which connection
> > has been closed when there are multiple ones to the given server.
> > To make it easier to identify that, it could be helpful to extend
> > the postgres_fdw_verify_connection_states() function so that it accepts
> > a unique connection as an input instead of just the server name.
> > One suggestion is to extend the function so that it accepts
> > both the server name and the user name used for the connection,
> > and checks the specified connection. If only the server name is specified,
> > the function should check all connections to the server and return false
> > if any of them are closed. This would be helpful since there is typically
> > only one connection to the server in most cases.
>
> Just to confirm, your point "user name" means a local user, right?
> I made a patch for addressing them.
>
> > Additionally, it would be helpful to extend the postgres_fdw_get_connections()
> > function to also return the user name used for each connection,
> > as currently there is no straightforward way to identify it.
>
> Added, See 0003. IIUC there is no good way to extract user mapping from its OID, so I have
> added an function to do that and used it.
>
> > The function name "postgres_fdw_verify_connection_states" may seem
> > unnecessarily long to me. A simpler name like
> > "postgres_fdw_verify_connection" may be enough?
>
> Renamed.
>
> > The patch may not be ready for commit due to the review comments,
> > and with the feature freeze approaching in a few days,
> > it may not be possible to include this feature in v16.
>
> It is sad for me, but it is more important for PostgreSQL to add nicer codes.
> I changed status to "Needs review" again.

I am failing to apply the latest
Patch-"v39-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch"
for review. Please find the error I am facing:
D:\Project\Postgres>git am
D:\Project\Patch\v39-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch
error: patch failed: contrib/postgres_fdw/connection.c:117
error: contrib/postgres_fdw/connection.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: postgres_fdw: add postgres_fdw_verify_connection variants
Patch failed at 0001 postgres_fdw: add postgres_fdw_verify_connection variants
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Please rebase and post an updated version of the Patch.

Thanks and Regards,
Shubham Khanna.