Re: [HACKERS] Optional message to user when terminating/cancellingbackend - Mailing list pgsql-hackers

On Fri, Oct 05, 2018 at 10:11:45AM +0200, Daniel Gustafsson wrote:
> Thanks!  Attached is a v17 which rebases the former 0002 patch on top of
> current master, along with the test fix for Windows that Thomas reported
> upthread (no other changes introduced over earlier versions).

Thanks for the new version.

In order to make a test with non-ASCII characters in the error message,
we could use a trick similar to xml.sql: use a function which ignores
the test case if server_encoding is not UTF-8 and use something like
chr() to pass down a messages with non-ASCII characters.  Then if the
function catches the error we are good.

+       *pid = slot->src_pid;
+       slot->orig_length = 0;
+       slot->message[0] = '\0';
Message consumption should be the part using memset(0), no?  system
calls are avoided within a spinlock section, but memset and strlcpy
should be fast enough.  Those are used in WalReceiverMain() for
example.

The facility to store messages should be in its own file, and
signalfuncs.c should depend on it, something like signal_message.c?

+typedef struct
+{
+   pid_t   dest_pid;       /* The pid of the process being signalled */
+   pid_t   src_pid;        /* The pid of the processing signalling */
+   slock_t mutex;          /* Per-slot protection */
+   char    message[MAX_CANCEL_MSG]; /* Message to send to signalled backend */
+   int     orig_length;    /* Length of the message as passed by the user,
+                            * if this length exceeds MAX_CANCEL_MSG it will
+                            * be truncated but we store the original length
+                            * in order to be able to convey truncation
*/
+   int     sqlerrcode;     /* errcode to use when signalling backend */
+} BackendSignalFeedbackShmemStruct

A couple of thoughts here:
- More information could make this facility more generic: an elevel to
be able to fully manipulate the custom error message, and a breakpoint
definition.  As far as I can see you have two of them in the patch which
are the callers of ConsumeBackendSignalFeedback().  One event cancels
the query, and another terminates it.  In both cases, the breakpoint
implies the elevel.  So could we have more possible breakpoints possible
and should we try to make this API more pluggable from the start?
- Is orig_length really useful in the data stored?  Why not just
warning/noticing the caller defining the message and just store the
message.

So, looking at your patch, I am wondering also about splitting it into
a couple of pieces for clarity:
- The base facility to be able to register and consume messages which
can be attached to a backend slot, and then be accessed by other things.
Let's think about it as a base facility which is useful for extensions
and module developers.  If coupled with a hook, it would be actually
possible to scan a backend's slot for some information which could be
consumed afterwards for custom error messages...
- The set of breakpoints which can then be used to define if a given
code path should show up a custom error message.  I can think of three
of them: cancel, termination and extension, which is a custom path that
extensions can use.  The extension breakpoint would make sense as
something included from the start, could be stored as an integer and I
guess should not be an enum but a set of defined tables (see pgstat.h
for wait event categories for example).
- The set of SQL functions making use of it in-core, in your case these
are the SQL functions for cancellation and termination.
--
Michael