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

> On 11 Oct 2018, at 03:29, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

Hi!,

Thanks for reviewing this patch, and sorry for having been slow lately.

> 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 test was added to the patch as a reviewer found a bug in an early version
where it didn’t treat mb characters properly, it was using strlen() and prone
to clipping an mb char in half. The test was added to the patch to show the
fix to the patch review process, but that doesn’t necessarily mean it’s deemed
an interesting enough case to have a test in check/installcheck for. I don’t
have strong opinions, other than trying to not add uninteresting tests as they
do carry a cost.

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

Actually it isn’t, it was your suggestion =)

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

I’ve added elevel to the API, but I’m not sure if keeping an additional buffer
for errdetail is worth it since it probably wouldn’t be used that often?

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

Since I wrote the code, I think it’s worth it as a nice-to-have rather than as
a pure necessity. If noone else sees the value then we’ll rip it out of the
patch.

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

Is a hook for extensions needed, as they too can drain the message via the
Consume function? Where if so do you reckon it should go?

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

I’ve split the patch into two logical parts, the signalling functionality and
the userfacing terminate/cancel part. For extra clarity I’ve also included the
full v19 patch, in case you prefer that instead when seeing the two.

cheers ./daniel