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

On Wed, Oct 10, 2018 at 02:20:53PM +0200, Daniel Gustafsson wrote:
>> On 9 Oct 2018, at 07:38, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> 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?

I am not exactly sure what you mean here.

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

Indeed, you could understand the past suggestion both ways. From what I
can see, there is merit in keeping the SQL-callable functions working on
signals into their own file. The message capacity that you are
proposing here should on their own, and...

> Moved the new functionality to signal_message.{ch}.

That's actually a much better name than anything I could think of.

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

Sure. From what I can see in your patch is that you want to generate in
some code paths an ereport() call with a given message string. As far
as I can see, you can decide three things with what's available:
- errcode
- errstring
- the moment you want the ereport() to be generated.

What I am suggesting here is to let callers extend the possibilities a
bit more with:
- elevel
- errdetail
This way, you can override code paths with a more custom experience.
The thing could break easily Postgres, as you should not really use a
non-FATAL elevel when terminating a session, but having a flexible API
will allow to not come back to it in the future. We may also want to
have ConsumeBackendSignalFeedback() call ereport() by itself with all
the details available in the data registered.

The main take on your feature is that it is more flexible than the elog
hook, as you can enforce custom strings at function call, and don't need
to do some weird hacks in plugins in need of manipulating the error.

>> - 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)));

The goal is to generate an elog() at the end, so I can see your point,
not sure if that's worth the complication though..

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

Those are my first impressions about the patch after looking at the
code so I have not done any code diving. The more pieces we are able to
break this stuff into, the more likely it is easy to review and to get
committed. From what I can see the main idea can be split into more
sub-concepts.
--
Michael