Re: [HACKERS] Optional message to user when terminating/cancelling backend

> On 9 Oct 2018, at 07:38, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

Thanks for reviewing!

> 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.

Thats one approach, do you think it’s worth adding to ensure clipping during
truncation?

> + *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.

Good point. IIRC I added it in the setting codepath to avoid memsetting in
case there were no more messages set. That’s an incorrect optimization, so
fixed.

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

It was originally in backend_signal.c, and was moved into (what is now)
signalfuncs.c in the refactoring that Alvaro suggested. Re-reading his email
I’m not sure he actually proposed merging this code with the signal code.

Moved the new functionality to signal_message.{ch}.

> +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?

I’m not sure I follow, can you elaborate on this?

> - Is orig_length really useful in the data stored? Why not just
> warning/noticing the caller defining the message and just store the
> message.

The caller is being notified, but the original length is returned such that the
consumer can format the message with the truncation in mind. In postgres.c we
currently do:

ereport(FATAL,
(errcode(sqlerrcode),
errmsg("%s%s",
buffer, (len > sizeof(buffer) ? "..." : "")),
errdetail("terminating connection due to administrator command by process %d",
pid)));

If that’s not deemed of value to keep, then orig_length can be dropped.

> 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.

This does not sound like a bad idea to me. Is that something you are planning
to do or do you want me to cut the patch up into pieces? Just want to avoid
stepping on each others toes if you are already thinking/poking at this.

Attached is a v18 with the above changes.

cheers ./daniel