Re: Add client connection check during the execution of the query

On Thu, Apr 1, 2021 at 10:16 PM tsunakawa(dot)takay(at)fujitsu(dot)com
<tsunakawa(dot)takay(at)fujitsu(dot)com> wrote:
> From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> > I changed my mind. Let's commit the pleasingly simple Linux-only feature for
> > now, and extend to it to send some kind of no-op message in a later release.
> > So this is the version I'd like to go with.
> > Objections?
>
> +1, as some of our users experienced the problem that the server kept processing (IIRC, a buggy PL/pgSQL procedure that loops indefinitely) after they killed the client.

Cool. Yeah, I have seen a few variants of that, and several other
complaints on the lists.

> TBH, Linux and Windows will be sufficient. But I'm for providing a good feature on a specific OS first.

I discovered that at least one other OS has adopted POLLRDHUP, so I
changed the language to something slightly more general:

+ <para>
+ This option is currently available only on systems that support the
+ non-standard <symbol>POLLRDHUP</symbol> extension to the
+ <symbol>poll</symbol> system call, including Linux.
+ </para>

It seems like it must be quite easy for an OS to implement, since the
TCP stack surely has the information... it's just an API problem.
Hopefully that means that there aren't OSes that define the macro but
don't work the same way. (I read somewhere that the POSIX compliance
test suite explicitly tests this half-shutdown case and fails any OS
that returns SIGHUP "prematurely". Boo.)

> (1)
> + rc = poll(&pollfd, 1, 0);
> + if (rc < 0)
> + {
> + elog(COMMERROR, "could not poll socket: %m");
> + return false;
>
> I think it's customary to use ereport() and errcode_for_socket_access().

Fixed.

> (2)
> pq_check_connection()
>
> Following PostmasterIsAlive(), maybe it's better to name it as pq_connection_is_alive() pq_client_is_alive(), or pq_frontend_is_alive() like the pqcomm.c's head comment uses the word frontend?

I think it's OK, because it matches the name of the GUC. I'm more
concerned about the name of the GUC. Will we still be happy with this
name if a future releases sends a heartbeat message? I think that is
still OK, so I'm happy with these names for now, but if someone has a
better name, please speak up very soon.

> (3)
> #include <limits.h>
> +#include <poll.h>
> #ifndef WIN32
> #include <sys/mman.h>
> #endif
>
> poll.h should be included between #ifndef WIN32 and #endif?

Oops, I forgot to wrap that in #ifdef HAVE_POLL_H while moving stuff
around. Fixed.

> (4)
> I think the new GUC works for walsender as well. If so, how do we explain the relationship between the new GUC and wal_receiver_timeout and recommend the settings of them?

No, it only works while executing a query. (Is there something in
logical decoding, perhaps, that I have failed to consider?)

PS The "from" headers in emails received from Fujitsu seems to have
the names stripped, somewhere in the tubes of the internet. I see the
full version when people from Fujitsu quote other people from Fujitsu.
I copied one of those into the commit message, complete with its
magnificent kanji characters (perhaps these are the cause of the
filtering?), and I hope that's OK with you.