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

[Michael Paquier]

On Fri, Nov 30, 2018 at 06:02:15PM +0100, Dmitry Dolgov wrote:
> Just for the records, cfbot complains during the compilation:
> 
> option.c: In function ‘parseCommandLine’:
> option.c:265:8: error: ignoring return value of ‘getcwd’, declared
> with attribute warn_unused_result [-Werror=unused-result]
>   getcwd(default_sockdir, MAXPGPATH);
> ^

The patch has been marked as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

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

[Dmitry Dolgov]

On Mon, Nov 12, 2018 at 8:36 PM Tom Lane  wrote:
>
> Daniel Gustafsson  writes:
> > 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.

Just for the records, cfbot complains during the compilation:

option.c: In function ‘parseCommandLine’:
option.c:265:8: error: ignoring return value of ‘getcwd’, declared
with attribute warn_unused_result [-Werror=unused-result]
  getcwd(default_sockdir, MAXPGPATH);
^

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

[Tom Lane]

Daniel Gustafsson  writes:
> 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.

I'm a bit befuddled by this patch, or at least the proposed test cases.
Those show no proof that the feature actually works, ie, delivers a
message to the target backend.  Also, what am I to make of the test cases
involving NULL arguments?

Personally, rather than messing around with defaulted arguments and
detecting nulls at runtime, I'd make two C functions that are both strict.

The signal_message stuff seems both overly complicated and overly fragile.
You have, for example, largely ignored the coding rule that says to have
only straight-line code within a spinlock hold.  I am also not very
enamored of setting up Yet Another per-backend data structure in shared
memory, especially one that can so easily get out of sync with the
ProcArray.  If we're going to expend dedicated shared memory on this
feature, let's just add the necessary fields to the PGPROC structs.
That would also provide some opportunity to interlock things in a way that
would fix the race conditions, ie, once we've found the relevant PGPROC
entry, hold a lock on it till we've inserted the message and sent the
signal.

I don't especially like orig_length: that information is of no use
to the recipient backend, and what the field is actually being used
as, ie a boolean "slot is full" indicator, is nowhere to be guessed
from the field's documentation.

The current patch fails to apply against parallel_schedule, which
reminds me that you forgot to include an update to serial_schedule.

regards, tom lane

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

[Daniel Gustafsson]

> On 11 Oct 2018, at 03:29, Michael Paquier  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  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 

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

[Michael Paquier]

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

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

[Daniel Gustafsson]

> On 9 Oct 2018, at 07:38, Michael Paquier  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 */
> +   charmessage[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

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

[Michael Paquier]

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 */
+   charmessage[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


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 4 Oct 2018, at 13:00, Michael Paquier  wrote:
> 
> On Thu, Oct 04, 2018 at 10:06:06AM +0200, Daniel Gustafsson wrote:
>> Yes, you are correct, the signalfuncs.h includes in 0001 are a rebase error
>> from when I renamed the file.  They are not present in the v15 patch but got
>> introduced in v16 when I clearly wasn’t caffeinated enough to rebase.
> 
> No problem, thanks for confirming.  I have worked a bit more on the
> thing, reducing the headers of the new file to a bare minimum, and I
> have committed 0001.

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

cheers ./daniel



terminate_msg_v17.patch
Description: Binary data

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

[Michael Paquier]

On Thu, Oct 04, 2018 at 10:06:06AM +0200, Daniel Gustafsson wrote:
> Yes, you are correct, the signalfuncs.h includes in 0001 are a rebase error
> from when I renamed the file.  They are not present in the v15 patch but got
> introduced in v16 when I clearly wasn’t caffeinated enough to rebase.

No problem, thanks for confirming.  I have worked a bit more on the
thing, reducing the headers of the new file to a bare minimum, and I
have committed 0001.
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 4 Oct 2018, at 09:59, Michael Paquier  wrote:
> 
> On Wed, Oct 03, 2018 at 12:09:54PM +1300, Thomas Munro wrote:
>> It looks like you missed another case that needs tolerance for late
>> signal delivery on Windows:
>> 
>> +select pg_cancel_backend(pg_backend_pid());
>> 
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15263
> 
> Looking at 0001, why are the declarations needed in patch 0002 part of
> 0001 (see signalfuncs.h)?  I think that something like instead the
> attached is enough for this part.  Daniel, could you confirm?

Yes, you are correct, the signalfuncs.h includes in 0001 are a rebase error
from when I renamed the file.  They are not present in the v15 patch but got
introduced in v16 when I clearly wasn’t caffeinated enough to rebase.

cheers ./daniel

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

[Michael Paquier]

On Wed, Oct 03, 2018 at 12:09:54PM +1300, Thomas Munro wrote:
> It looks like you missed another case that needs tolerance for late
> signal delivery on Windows:
> 
> +select pg_cancel_backend(pg_backend_pid());
> 
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15263

Looking at 0001, why are the declarations needed in patch 0002 part of
0001 (see signalfuncs.h)?  I think that something like instead the
attached is enough for this part.  Daniel, could you confirm?
--
Michael
diff --git a/src/backend/storage/ipc/Makefile b/src/backend/storage/ipc/Makefile
index 9dbdc26c9b..bf4619d5fd 100644
--- a/src/backend/storage/ipc/Makefile
+++ b/src/backend/storage/ipc/Makefile
@@ -8,8 +8,8 @@ subdir = src/backend/storage/ipc
 top_builddir = ../../../..
 include $(top_builddir)/src/Makefile.global
 
-OBJS = barrier.o dsm_impl.o dsm.o ipc.o ipci.o latch.o pmsignal.o procarray.o \
-	procsignal.o  shmem.o shmqueue.o shm_mq.o shm_toc.o sinval.o \
-	sinvaladt.o standby.o
+OBJS = signalfuncs.o barrier.o dsm_impl.o dsm.o ipc.o ipci.o latch.o \
+	pmsignal.o procarray.o procsignal.o shmem.o shmqueue.o shm_mq.o \
+	shm_toc.o sinval.o sinvaladt.o standby.o
 
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
new file mode 100644
index 00..c09a047127
--- /dev/null
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -0,0 +1,216 @@
+/*-
+ *
+ * signalfuncs.c
+ *	  Functions for signalling backends
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/backend/storage/ipc/signalfuncs.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include 
+
+#include "catalog/pg_authid.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "postmaster/syslogger.h"
+#include "storage/ipc.h"
+#include "storage/pmsignal.h"
+#include "storage/proc.h"
+#include "storage/procarray.h"
+#include "utils/acl.h"
+#include "utils/builtins.h"
+
+/*
+ * Send a signal to another backend.
+ *
+ * The signal is delivered if the user is either a superuser or the same
+ * role as the backend being signaled. For "dangerous" signals, an explicit
+ * check for superuser needs to be done prior to calling this function.
+ *
+ * Returns 0 on success, 1 on general failure, 2 on normal permission error
+ * and 3 if the caller needs to be a superuser.
+ *
+ * In the event of a general failure (return code 1), a warning message will
+ * be emitted. For permission errors, doing that is the responsibility of
+ * the caller.
+ */
+#define SIGNAL_BACKEND_SUCCESS 0
+#define SIGNAL_BACKEND_ERROR 1
+#define SIGNAL_BACKEND_NOPERMISSION 2
+#define SIGNAL_BACKEND_NOSUPERUSER 3
+static int
+pg_signal_backend(int pid, int sig)
+{
+	PGPROC	   *proc = BackendPidGetProc(pid);
+
+	/*
+	 * BackendPidGetProc returns NULL if the pid isn't valid; but by the time
+	 * we reach kill(), a process for which we get a valid proc here might
+	 * have terminated on its own.  There's no way to acquire a lock on an
+	 * arbitrary process to prevent that. But since so far all the callers of
+	 * this mechanism involve some request for ending the process anyway, that
+	 * it might end on its own first is not a problem.
+	 */
+	if (proc == NULL)
+	{
+		/*
+		 * This is just a warning so a loop-through-resultset will not abort
+		 * if one backend terminated on its own during the run.
+		 */
+		ereport(WARNING,
+(errmsg("PID %d is not a PostgreSQL server process", pid)));
+		return SIGNAL_BACKEND_ERROR;
+	}
+
+	/* Only allow superusers to signal superuser-owned backends. */
+	if (superuser_arg(proc->roleId) && !superuser())
+		return SIGNAL_BACKEND_NOSUPERUSER;
+
+	/* Users can signal backends they have role membership in. */
+	if (!has_privs_of_role(GetUserId(), proc->roleId) &&
+		!has_privs_of_role(GetUserId(), DEFAULT_ROLE_SIGNAL_BACKENDID))
+		return SIGNAL_BACKEND_NOPERMISSION;
+
+	/*
+	 * Can the process we just validated above end, followed by the pid being
+	 * recycled for a new process, before reaching here?  Then we'd be trying
+	 * to kill the wrong thing.  Seems near impossible when sequential pid
+	 * assignment and wraparound is used.  Perhaps it could happen on a system
+	 * where pid re-use is randomized.  That race condition possibility seems
+	 * too unlikely to worry about.
+	 */
+
+	/* If we have setsid(), signal the backend's whole process group */
+#ifdef HAVE_SETSID
+	if (kill(-pid, sig))
+#else
+	if (kill(pid, sig))
+#endif
+	{
+		/* Again, just a warning to allow loops */
+		ereport(WARNING,
+(errmsg("could not send signal to process %d: %m", pid)));
+		return SIGNAL_BACKEND_ERROR;
+	}
+	return SIGNAL_BACKEND_SUCCESS;
+}
+
+/*
+ * Signal to cancel a backend process.  This is allowed if you are 

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

[Thomas Munro]

On Tue, Oct 2, 2018 at 1:37 AM Daniel Gustafsson  wrote:
> > On 1 Oct 2018, at 01:19, Michael Paquier  wrote:
> > On Sun, Sep 30, 2018 at 10:51:44PM +0900, Michael Paquier wrote:
> >> You could have chosen something less complicated, like "ホゲ", which is
> >> the equivalent of "foo" in English.  Anyway, non-ASCII characters should
> >> not be included in the final patch.
>
> Fixed in the attached v16 revision.

Hi Daniel,

It looks like you missed another case that needs tolerance for late
signal delivery on Windows:

+select pg_cancel_backend(pg_backend_pid());

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15263

-- 
Thomas Munro
http://www.enterprisedb.com

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

[Michael Paquier]

On Mon, Oct 01, 2018 at 02:37:42PM +0200, Daniel Gustafsson wrote:
>> On 1 Oct 2018, at 01:19, Michael Paquier  wrote:
>> Looking at the refactoring patch 0001, wouldn't signalfuncs.c make a
>> better name for the new file?  There are already multiple examples of
>> this type, like logicalfuncs.c, slotfuncs.c, etc.
> 
> I have no strong feelings on this, I was merely using the name that Alvaro
> suggested when he brought up the refactoring as an extension of this patch.
> signalfuncs.c is fine by me, so I did this rename in the attached revision.

Indeed, I missed the previous part posted here:
https://www.postgresql.org/message-id/20180124154548.gmpyvkzlsijren7u@alvherre.pgsql

Alvaro, do you have a strong preference over one or the other?
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 1 Oct 2018, at 01:19, Michael Paquier  wrote:
> 
> On Sun, Sep 30, 2018 at 10:51:44PM +0900, Michael Paquier wrote:
>> You could have chosen something less complicated, like "ホゲ", which is
>> the equivalent of "foo" in English.  Anyway, non-ASCII characters should
>> not be included in the final patch.

Fixed in the attached v16 revision.

> Looking at the refactoring patch 0001, wouldn't signalfuncs.c make a
> better name for the new file?  There are already multiple examples of
> this type, like logicalfuncs.c, slotfuncs.c, etc.

I have no strong feelings on this, I was merely using the name that Alvaro
suggested when he brought up the refactoring as an extension of this patch.
signalfuncs.c is fine by me, so I did this rename in the attached revision.

> I have moved this patch set to the next commit fest for now.

Thanks.

cheers ./daniel



0001-Refactor-backend-signalling-code-v16.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v16.patch
Description: Binary data

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

[Michael Paquier]

On Sun, Sep 30, 2018 at 10:51:44PM +0900, Michael Paquier wrote:
> You could have chosen something less complicated, like "ホゲ", which is
> the equivalent of "foo" in English.  Anyway, non-ASCII characters should
> not be included in the final patch.

Looking at the refactoring patch 0001, wouldn't signalfuncs.c make a
better name for the new file?  There are already multiple examples of
this type, like logicalfuncs.c, slotfuncs.c, etc.

I have moved this patch set to the next commit fest for now.
--
Michael


signature.asc
Description: PGP signature

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

[Michael Paquier]

On Mon, Aug 27, 2018 at 12:06:18PM +0200, Daniel Gustafsson wrote:
> I like that idea, so I’ve updated the patch to see what the cfbot
> thinks of it.

+   when pg_cancel_backend(pg_backend_pid(), 'ソケットをブロッキングモードに設定できませんでした')

You could have chosen something less complicated, like "ホゲ", which is
the equivalent of "foo" in English.  Anyway, non-ASCII characters should
not be included in the final patch.
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 23 Aug 2018, at 20:53, Tom Lane  wrote:

> I think this is just a timing problem: the signal gets sent,
> but it might or might not get received before the current statement ends.
> It's possible that a signal-sent-to-self can be expected to be received
> synchronously on all Unix platforms, but I wouldn't entirely bet on that
> (in particular, the POSIX text for kill(2) does NOT guarantee it); and
> our Windows signal implementation certainly doesn't guarantee anything
> of the sort.

That makes a lot of sense, I was thinking about it in too simplistic terms.
Thanks for the clarification.

> I don't think there's necessarily anything wrong with the code, but
> you can't test it with such a simplistic test as this and expect
> stable results.  If you feel an urgent need to have a test case,
> try building an isolationtester script.

During hacking on it I preferred to have tests for it.  Iff this makes it in,
the tests can be omitted per the judgement of the committers of course.  Since
the printed pid will vary between runs it’s hard to test more interesting
scenarios so they are of questionable worth.

cheers ./daniel

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

[Daniel Gustafsson]

> On 24 Aug 2018, at 03:37, Tom Lane  wrote:
> 
> Thomas Munro  writes:
>> On Fri, Aug 24, 2018 at 6:53 AM, Tom Lane  wrote:
>>> I think this is just a timing problem: the signal gets sent,
>>> but it might or might not get received before the current statement ends.
> 
>> How about we just wait forever if the function manages to return?
>> select case when pg_cancel_backend(pg_backend_pid(), '...') then
>> pg_sleep('infinity') end;
> 
> Hm, that might work.  I'd pick a long but not infinite timeout --- maybe
> 60 sec would be good.

I like that idea, so I’ve updated the patch to see what the cfbot thinks of it.

cheers ./daniel



0001-Refactor-backend-signalling-code-v15.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v15.patch
Description: Binary data

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

[Tom Lane]

Thomas Munro  writes:
> On Fri, Aug 24, 2018 at 6:53 AM, Tom Lane  wrote:
>> I think this is just a timing problem: the signal gets sent,
>> but it might or might not get received before the current statement ends.

> How about we just wait forever if the function manages to return?
> select case when pg_cancel_backend(pg_backend_pid(), '...') then
> pg_sleep('infinity') end;

Hm, that might work.  I'd pick a long but not infinite timeout --- maybe
60 sec would be good.

regards, tom lane

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

[Thomas Munro]

On Fri, Aug 24, 2018 at 6:53 AM, Tom Lane  wrote:
> Thomas Munro  writes:
>> On Wed, Jul 25, 2018 at 7:27 PM, Daniel Gustafsson  wrote:
>>> Seems the build of the updated patch built and tested Ok.  Still have no 
>>> idea
>>> why the previous one didn’t.
>
>> That problem apparently didn't go away.  cfbot tested it 7 times in
>> the past week, and it passed only once on Windows:
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9691
>> The other times all failed like this:
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9833
>
> I think this is just a timing problem: the signal gets sent,
> but it might or might not get received before the current statement ends.
> It's possible that a signal-sent-to-self can be expected to be received
> synchronously on all Unix platforms, but I wouldn't entirely bet on that
> (in particular, the POSIX text for kill(2) does NOT guarantee it); and
> our Windows signal implementation certainly doesn't guarantee anything
> of the sort.

How about we just wait forever if the function manages to return?

select case when pg_cancel_backend(pg_backend_pid(), '...') then
pg_sleep('infinity') end;

-- 
Thomas Munro
http://www.enterprisedb.com

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

[Tom Lane]

Thomas Munro  writes:
> On Wed, Jul 25, 2018 at 7:27 PM, Daniel Gustafsson  wrote:
>> Seems the build of the updated patch built and tested Ok.  Still have no idea
>> why the previous one didn’t.

> That problem apparently didn't go away.  cfbot tested it 7 times in
> the past week, and it passed only once on Windows:
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9691
> The other times all failed like this:
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9833

I think this is just a timing problem: the signal gets sent,
but it might or might not get received before the current statement ends.
It's possible that a signal-sent-to-self can be expected to be received
synchronously on all Unix platforms, but I wouldn't entirely bet on that
(in particular, the POSIX text for kill(2) does NOT guarantee it); and
our Windows signal implementation certainly doesn't guarantee anything
of the sort.

I don't think there's necessarily anything wrong with the code, but
you can't test it with such a simplistic test as this and expect
stable results.  If you feel an urgent need to have a test case,
try building an isolationtester script.

regards, tom lane

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

[Thomas Munro]

On Wed, Jul 25, 2018 at 7:27 PM, Daniel Gustafsson  wrote:
>> On 24 Jul 2018, at 22:57, Daniel Gustafsson  wrote:
>>
>>> On 6 Jul 2018, at 02:18, Thomas Munro  wrote:
>>>
>>> On Fri, Jun 15, 2018 at 9:56 AM, Daniel Gustafsson  wrote:
 attached
>>>
>>> Hi Daniel,
>>>
>>> 6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>>> changes');
>>> 6119  select pg_cancel_backend(pg_backend_pid(), NULL);
>>> 6120! ERROR:  canceling statement due to user request
>>> 6121--- 25,32 
>>> 6122
>>> 6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>>> changes');
>>> 6124  select pg_cancel_backend(pg_backend_pid(), NULL);
>>> 6125!  pg_cancel_backend
>>> 6126! ---
>>> 6127!  t
>>>
>>> Apparently Windows can take or leave it as it pleases.
>>
>> Well played =)
>>
>>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488
>>
>> That reads to me like it’s cancelling another backend than the current one,
>> which clearly isn’t right as we’re passing pg_backend_pid().  I can’t really
>> see what Windows specific bug was introduced by this patch though (or well, 
>> the
>> bug exhibits itself on Windows but it may well be generic of course).
>>
>> Will continue to hunt.
>
> Seems the build of the updated patch built and tested Ok.  Still have no idea
> why the previous one didn’t.

That problem apparently didn't go away.  cfbot tested it 7 times in
the past week, and it passed only once on Windows:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9691

The other times all failed like this:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.9833

-- 
Thomas Munro
http://www.enterprisedb.com

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

[Daniel Gustafsson]

> On 12 Aug 2018, at 11:01, Andres Freund  wrote:
> 
> On August 12, 2018 12:17:59 AM GMT+02:00, Daniel Gustafsson  
> wrote:
>>> On 6 Aug 2018, at 09:47, Heikki Linnakangas  wrote:
>>> 
>>> Has there been any consideration to encodings?
>> 
>> Thats a good point, no =/
>> 
>>> What happens if the message contains non-ASCII characters, and the
>> sending backend is connected to database that uses a different encoding
>> than the backend being signaled?
>> 
>> In the current state of the patch, instead of the message you get:
>> 
>> FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding "UTF8"
>> has
>>  no equivalent in encoding “ISO_8859_5"
>> 
>> Thats clearly not good enough, but I’m not entirely sure what would be
>> the best
>> way forward.  Restrict messages to only be in SQL_ASCII?  Store the
>> encoding of
>> the message and check the encoding of the receiving backend before
>> issuing it
>> for a valid conversion, falling back to no message in case there is
>> none?
>> Neither seems terribly appealing, do you have any better suggestions?
> 
> Restricting to ASCII seems reasonable.

It’s quite restrictive, but it’s the safe option.  I’ve hacked this into the
updated patch, but kept the backend_feedback() function using pg_mbstrlen() at
least for now since it seems the safe option should this be relaxed at some
point.  Also added a small test by copying text from a ja.po file in the tree.

> But note that sqlascii isn't that (it's essentially just arbitrary null 
> terminated data). Easier to relax later.

Yeah, my fingers and brain were not in sync during typing, I meant to say ASCII
there. I blame a lack of coffee.

cheers ./daniel



0001-Refactor-backend-signalling-code-v14.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v14.patch
Description: Binary data

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

[Andres Freund]

On August 12, 2018 12:17:59 AM GMT+02:00, Daniel Gustafsson  
wrote:
>> On 6 Aug 2018, at 09:47, Heikki Linnakangas  wrote:
>> 
>> Has there been any consideration to encodings?
>
>Thats a good point, no =/
>
>> What happens if the message contains non-ASCII characters, and the
>sending backend is connected to database that uses a different encoding
>than the backend being signaled?
>
>In the current state of the patch, instead of the message you get:
>
>FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding "UTF8"
>has
>   no equivalent in encoding “ISO_8859_5"
>
>Thats clearly not good enough, but I’m not entirely sure what would be
>the best
>way forward.  Restrict messages to only be in SQL_ASCII?  Store the
>encoding of
>the message and check the encoding of the receiving backend before
>issuing it
>for a valid conversion, falling back to no message in case there is
>none?
>Neither seems terribly appealing, do you have any better suggestions?

Restricting to ASCII seems reasonable. But note that sqlascii isn't that (it's 
essentially just arbitrary null terminated data). Easier to relax later.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Pavel Stehule]

2018-08-12 10:29 GMT+02:00 Daniel Gustafsson :

> > On 12 Aug 2018, at 07:42, Pavel Stehule  wrote:
> > 2018-08-12 0:17 GMT+02:00 Daniel Gustafsson  dan...@yesql.se>>:
> > > On 6 Aug 2018, at 09:47, Heikki Linnakangas  hlinn...@iki.fi>> wrote:
>
> > > What happens if the message contains non-ASCII characters, and the
> sending backend is connected to database that uses a different encoding
> than the backend being signaled?
> >
> > In the current state of the patch, instead of the message you get:
> >
> > FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding
> "UTF8" has
> >no equivalent in encoding “ISO_8859_5"
> >
> > Where this code fails? Isn't somewhere upper where string literals are
> translated? Then this message is ok.
>
> This happens for example when a UTF-8 backend sends a message with japanese
> characters to a backend using ISO_8859_5.  So the code works as expected,
> but
> it’s not a very good user experience.
>

It is same like any other using of string literal.

Personally, I don't think so these functions should be a exception.

Pavel


> cheers ./daniel

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

[Daniel Gustafsson]

> On 12 Aug 2018, at 07:42, Pavel Stehule  wrote:
> 2018-08-12 0:17 GMT+02:00 Daniel Gustafsson  >:
> > On 6 Aug 2018, at 09:47, Heikki Linnakangas  > > wrote:

> > What happens if the message contains non-ASCII characters, and the sending 
> > backend is connected to database that uses a different encoding than the 
> > backend being signaled?
> 
> In the current state of the patch, instead of the message you get:
> 
> FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding "UTF8" has
>no equivalent in encoding “ISO_8859_5"
> 
> Where this code fails? Isn't somewhere upper where string literals are 
> translated? Then this message is ok.

This happens for example when a UTF-8 backend sends a message with japanese
characters to a backend using ISO_8859_5.  So the code works as expected, but
it’s not a very good user experience.

cheers ./daniel

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

[Pavel Stehule]

2018-08-12 0:17 GMT+02:00 Daniel Gustafsson :

> > On 6 Aug 2018, at 09:47, Heikki Linnakangas  wrote:
> >
> > Has there been any consideration to encodings?
>
> Thats a good point, no =/
>
> > What happens if the message contains non-ASCII characters, and the
> sending backend is connected to database that uses a different encoding
> than the backend being signaled?
>
> In the current state of the patch, instead of the message you get:
>
> FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding "UTF8"
> has
>no equivalent in encoding “ISO_8859_5"
>

Where this code fails? Isn't somewhere upper where string literals are
translated? Then this message is ok.


>
> Thats clearly not good enough, but I’m not entirely sure what would be the
> best
> way forward.  Restrict messages to only be in SQL_ASCII?  Store the
> encoding of
> the message and check the encoding of the receiving backend before issuing
> it
> for a valid conversion, falling back to no message in case there is none?
> Neither seems terribly appealing, do you have any better suggestions?
>

The client<->server encoding translation should do this work no?

Regards

Pavel


> cheers ./daniel

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

[Daniel Gustafsson]

> On 6 Aug 2018, at 09:47, Heikki Linnakangas  wrote:
> 
> Has there been any consideration to encodings?

Thats a good point, no =/

> What happens if the message contains non-ASCII characters, and the sending 
> backend is connected to database that uses a different encoding than the 
> backend being signaled?

In the current state of the patch, instead of the message you get:

FATAL: character with byte sequence 0xe3 0x82 0xbd in encoding "UTF8" has
   no equivalent in encoding “ISO_8859_5"

Thats clearly not good enough, but I’m not entirely sure what would be the best
way forward.  Restrict messages to only be in SQL_ASCII?  Store the encoding of
the message and check the encoding of the receiving backend before issuing it
for a valid conversion, falling back to no message in case there is none?
Neither seems terribly appealing, do you have any better suggestions?

cheers ./daniel

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

[Heikki Linnakangas]

Has there been any consideration to encodings? What happens if the 
message contains non-ASCII characters, and the sending backend is 
connected to database that uses a different encoding than the backend 
being signaled?


- Heikki

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

[Daniel Gustafsson]

> On 24 Jul 2018, at 22:57, Daniel Gustafsson  wrote:
> 
>> On 6 Jul 2018, at 02:18, Thomas Munro  wrote:
>> 
>> On Fri, Jun 15, 2018 at 9:56 AM, Daniel Gustafsson  wrote:
>>> attached
>> 
>> Hi Daniel,
>> 
>> 6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>> changes');
>> 6119  select pg_cancel_backend(pg_backend_pid(), NULL);
>> 6120! ERROR:  canceling statement due to user request
>> 6121--- 25,32 
>> 6122
>> 6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>> changes');
>> 6124  select pg_cancel_backend(pg_backend_pid(), NULL);
>> 6125!  pg_cancel_backend
>> 6126! ---
>> 6127!  t
>> 
>> Apparently Windows can take or leave it as it pleases.
> 
> Well played =)
> 
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488
> 
> That reads to me like it’s cancelling another backend than the current one,
> which clearly isn’t right as we’re passing pg_backend_pid().  I can’t really
> see what Windows specific bug was introduced by this patch though (or well, 
> the
> bug exhibits itself on Windows but it may well be generic of course).
> 
> Will continue to hunt.

Seems the build of the updated patch built and tested Ok.  Still have no idea
why the previous one didn’t.

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.6695

cheers ./daniel

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

[Daniel Gustafsson]

> On 6 Jul 2018, at 02:18, Thomas Munro  wrote:
> 
> On Fri, Jun 15, 2018 at 9:56 AM, Daniel Gustafsson  wrote:
>> attached
> 
> Hi Daniel,
> 
> 6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
> changes');
> 6119  select pg_cancel_backend(pg_backend_pid(), NULL);
> 6120! ERROR:  canceling statement due to user request
> 6121--- 25,32 
> 6122
> 6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
> changes');
> 6124  select pg_cancel_backend(pg_backend_pid(), NULL);
> 6125!  pg_cancel_backend
> 6126! ---
> 6127!  t
> 
> Apparently Windows can take or leave it as it pleases.

Well played =)

> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488

That reads to me like it’s cancelling another backend than the current one,
which clearly isn’t right as we’re passing pg_backend_pid().  I can’t really
see what Windows specific bug was introduced by this patch though (or well, the
bug exhibits itself on Windows but it may well be generic of course).

Will continue to hunt.

cheers ./daniel

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

[Daniel Gustafsson]

> On 6 Jul 2018, at 03:47, Michael Paquier  wrote:
> 
> On Fri, Jul 06, 2018 at 12:18:02PM +1200, Thomas Munro wrote:
>> 6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>> changes');
>> 6119  select pg_cancel_backend(pg_backend_pid(), NULL);
>> 6120! ERROR:  canceling statement due to user request
>> 6121--- 25,32 
>> 6122
>> 6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
>> changes');
>> 6124  select pg_cancel_backend(pg_backend_pid(), NULL);
>> 6125!  pg_cancel_backend
>> 6126! ---
>> 6127!  t
>> 
>> Apparently Windows can take or leave it as it pleases.
>> 
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488
> 
> The test coverage looks adapted if it is possible to catch such
> failures, so that's nice.
> 
> +select pg_cancel_backend();
> +ERROR:  function pg_cancel_backend() does not exist
> +LINE 1: select pg_cancel_backend();
> This negative test is not really necessary.

I guess you’re right, it’s a bit superfluous.  Removed in the last sent
patchset.

cheers ./daniel

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

[Daniel Gustafsson]

> On 6 Jul 2018, at 14:12, Pavel Stehule  wrote:

> I am testing last patch and looks so truncating message and appending "..." 
> doesn't work.

Thanks for testing, and sorry about the late response.

> The slot->len hold trimmed length, not original length.

Fixed in the attached rebased patchset together with a few small touch-ups such
as fixed documentation.

cheers ./daniel



0001-Refactor-backend-signalling-code-v13.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v13.patch
Description: Binary data

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

[Pavel Stehule]

Hi

I am testing last patch and looks so truncating message and appending "..."
doesn't work.

The slot->len hold trimmed length, not original length.

Regards

Pavel

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

[Michael Paquier]

On Fri, Jul 06, 2018 at 12:18:02PM +1200, Thomas Munro wrote:
> 6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
> changes');
> 6119  select pg_cancel_backend(pg_backend_pid(), NULL);
> 6120! ERROR:  canceling statement due to user request
> 6121--- 25,32 
> 6122
> 6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many 
> changes');
> 6124  select pg_cancel_backend(pg_backend_pid(), NULL);
> 6125!  pg_cancel_backend
> 6126! ---
> 6127!  t
> 
> Apparently Windows can take or leave it as it pleases.
> 
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488

The test coverage looks adapted if it is possible to catch such
failures, so that's nice.

+select pg_cancel_backend();
+ERROR:  function pg_cancel_backend() does not exist
+LINE 1: select pg_cancel_backend();
This negative test is not really necessary.
--
Michael


signature.asc
Description: PGP signature

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

[Thomas Munro]

On Fri, Jun 15, 2018 at 9:56 AM, Daniel Gustafsson  wrote:
> attached

Hi Daniel,

6118  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many changes');
6119  select pg_cancel_backend(pg_backend_pid(), NULL);
6120! ERROR:  canceling statement due to user request
6121--- 25,32 
6122
6123  --select pg_cancel_backend(pg_backend_pid(), 'it brings on many changes');
6124  select pg_cancel_backend(pg_backend_pid(), NULL);
6125!  pg_cancel_backend
6126! ---
6127!  t

Apparently Windows can take or leave it as it pleases.

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4488

-- 
Thomas Munro
http://www.enterprisedb.com

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

[Daniel Gustafsson]

> On 13 Jun 2018, at 21:16, Andres Freund  wrote:

Thanks for the review!

> I'm not sure I really like the appending bit. There's a security
> argument to be made about doing so, but from a user POV that mostly
> seems restrictive.  I wonder if that aspect would be better handled by
> adding an error context that contains information about which process
> did the cancellation (or DETAIL?)?

That doesn’t sound like a bad idea at all, I took a stab at that in the
attached patch to see what it would look like.  Is that along the lines of what
you had in mind?

It does however make testing harder as the .out file can’t have the matching
pid (commented out the test in the attached hoping that there is a way to test
it that I fail to see).

>> +/*
>> + * Structure for registering a feedback payload to be sent to a cancelled, 
>> or
>> + * terminated backend. Each backend is registered per pid in the array 
>> which is
>> + * indexed by Backend ID. Reading and writing the message is protected by a
>> + * per-slot spinlock.
>> + */
>> +typedef struct
>> +{
>> +pid_t   pid;
> 
> This is the pid of the receiving process? If so, why do we need this in
> here? That's just duplicated data, no?

Correct, we need that for finding the right slot in backend_feedback() unless
I’m missing something?

>> +slock_t mutex;
>> +charmessage[MAX_CANCEL_MSG];
>> +int len;
>> +int sqlerrcode;
> 
> I'd vote for including the pid of the process that did the cancelling in
> here.

Did that for the above discussed log message change.

>> [ .. ]
> 
> Why isn't this just one function? Given that you already have a PG_NARGS
> check, I don't quite see the point?

I was trying to retain STRICT on pg_cancel_backend(int4) for no food reason,
and I’m not entirely sure what the reason was anymore.  Fixed in the attached
version.

>> [ .. ]
> 
> I'm not quite seeing the point of these variants.  What advantage are
> they over just doing the same on the caller level?

It seemed a cleaner API to provide these for when the caller just want to
provide messaging (which is where I started this a long time ago).  I’m not wed
to the idea at all though, they are clearly just for convenience.

>> +MemSet(slot->message, '\0', sizeof(slot->message));
>> +memcpy(slot->message, message, len);
> 
> This seems unnecessarily expensive.

My goal was to safeguard against the risk of fragments from an undelivered
message being delivered to another backend, which might be overly cautious.
Using strlcpy() instead to guarantee termination is perhaps enough?

>> +slot->len = len;
>> +slot->sqlerrcode = sqlerrcode;
>> +SpinLockRelease(>mutex);
>> +
>> +if (len != strlen(message))
>> +ereport(NOTICE,
>> +(errmsg("message is too long 
>> and has been truncated")));
>> +return 0;
>> +}
>> +}
> 
> So we made cancellation a O(N) proposition :/. Probably not too bad, but
> still…

When setting a message to be passed to the backend yes, with an upper bound of
MaxBackends.

>> [ .. ]
> 
> So we now do log spam if processes exit in the wrong moment?

Yeah, that was rather pointless. Removed in the attached version.

> I'm somewhat uncomfortable with acquiring a mutex in an error path.  We
> used to prcoess at least some interrupts in signal handlers in some
> cases - I think I removed all of them, but if not, we'd be in deep
> trouble here.  Wonder if we shouldn't try to get around needing that…

I’m all ears if you have a better idea, this was the only option I could come
up with.

cheers ./daniel



0001-Refactor-backend-signalling-code-v12.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v12.patch
Description: Binary data

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

[Andres Freund]

Hi,

Onder, I CCed you because it seems worthwhile to ensure that the
relevant Citus code could use this instead of the gross hack you and I
committed...

On 2018-06-13 20:54:03 +0200, Daniel Gustafsson wrote:
> > On 9 Apr 2018, at 23:55, Andres Freund  wrote:
> > On 2017-06-20 13:01:35 -0700, Andres Freund wrote:
> >> For extensions it'd also be useful if it'd be possible to overwrite the
> >> error code.  E.g. for citus there's a distributed deadlock detector,
> >> running out of process because there's no way to interrupt lock waits
> >> locally, and we've to do some ugly hacking to generate proper error
> >> messages and code from another session.
> > 
> > What happened to this request?  Seems we're out of the crunch mode and
> > could round the feature out a littlebit more…
> 
> Revisiting old patches, I took a stab at this request.
> 
> Since I don’t really have a use case for altering the sqlerrcode other than 
> the
> on that Citus..  cited, I modelled the API around that.

Cool. Onder?


> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index b851fe023a..ad9763698f 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -18540,7 +18540,7 @@ SELECT set_config('log_statement_stats', 'off', 
> false);
>   
>
> 
> -pg_cancel_backend(pid 
> int)
> +pg_cancel_backend(pid 
> int [, message 
> text])
>  
> boolean
> Cancel a backend's current query.  This is also allowed if the
> @@ -18565,7 +18565,7 @@ SELECT set_config('log_statement_stats', 'off', 
> false);
>
>
> 
> -pg_terminate_backend(pid 
> int)
> +pg_terminate_backend(pid 
> int [, message 
> text])
>  
> boolean
> Terminate a backend.  This is also allowed if the calling role
> @@ -18596,6 +18596,14 @@ SELECT set_config('log_statement_stats', 'off', 
> false);
>  The role of an active backend can be found from the
>  usename column of the
>  pg_stat_activity view.
> +If the optional message parameter is set, the text
> +will be appended to the error message returned to the signalled backend.
> +message is limited to 128 bytes, any longer text
> +will be truncated. An example where we cancel our own backend:
> +
> +postgres=# SELECT pg_cancel_backend(pg_backend_pid(), 'Cancellation message 
> text');
> +ERROR:  canceling statement due to user request: Cancellation message text
> +
> 

I'm not sure I really like the appending bit. There's a security
argument to be made about doing so, but from a user POV that mostly
seems restrictive.  I wonder if that aspect would be better handled by
adding an error context that contains information about which process
did the cancellation (or DETAIL?)?


> +/*
> + * Structure for registering a feedback payload to be sent to a cancelled, or
> + * terminated backend. Each backend is registered per pid in the array which 
> is
> + * indexed by Backend ID. Reading and writing the message is protected by a
> + * per-slot spinlock.
> + */
> +typedef struct
> +{
> + pid_t   pid;

This is the pid of the receiving process? If so, why do we need this in
here? That's just duplicated data, no?


> + slock_t mutex;
> + charmessage[MAX_CANCEL_MSG];
> + int len;
> + int sqlerrcode;

I'd vote for including the pid of the process that did the cancelling in
here.

> +Datum
> +pg_cancel_backend(PG_FUNCTION_ARGS)
> +{
> + PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
> +}
> +
> +Datum
> +pg_cancel_backend_msg(PG_FUNCTION_ARGS)
> +{
> + pid_t   pid;
> + char   *msg = NULL;
> +
> + if (PG_ARGISNULL(0))
> + PG_RETURN_NULL();
> +
> + pid = PG_GETARG_INT32(0);
> +
> + if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
> + msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> + PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
> +}

Why isn't this just one function? Given that you already have a PG_NARGS
check, I don't quite see the point?

>  /*
>   * Signal to terminate a backend process.  This is allowed if you are a 
> member
>   * of the role whose process is being terminated.
>   *
>   * Note that only superusers can signal superuser-owned processes.
>   */
> -Datum
> -pg_terminate_backend(PG_FUNCTION_ARGS)
> +static bool
> +pg_terminate_backend_internal(pid_t pid, char *msg)
>  {
> - int r = pg_signal_backend(PG_GETARG_INT32(0), 
> SIGTERM);
> + int r = pg_signal_backend(pid, SIGTERM, msg);
>  
>   if (r == SIGNAL_BACKEND_NOSUPERUSER)
>   ereport(ERROR,
> @@ -146,7 +203,30 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
>   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>(errmsg("must be a member of the role whose 
> process is being terminated or member of pg_signal_backend";
>  
> - PG_RETURN_BOOL(r == 

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

[Daniel Gustafsson]

> On 9 Apr 2018, at 23:55, Andres Freund  wrote:
> On 2017-06-20 13:01:35 -0700, Andres Freund wrote:
>> For extensions it'd also be useful if it'd be possible to overwrite the
>> error code.  E.g. for citus there's a distributed deadlock detector,
>> running out of process because there's no way to interrupt lock waits
>> locally, and we've to do some ugly hacking to generate proper error
>> messages and code from another session.
> 
> What happened to this request?  Seems we're out of the crunch mode and
> could round the feature out a littlebit more…

Revisiting old patches, I took a stab at this request.

Since I don’t really have a use case for altering the sqlerrcode other than the
on that Citus..  cited, I modelled the API around that.  The slot now holds a
sqlerrcode as well as a message, with functions to just set the message keeping
the default sqlerrcode for when that is all one wants to do.  There is no
function for just altering the sqlerrcode without a message as that seems not
useful to me.

The combination of sqlerrcode and message was dubbed SignalFeedback for lack of
a better term.  With this I also addressed something that annoyed me; I had
called all the functions Cancel* which technically isn’t true since we also
support termination.

There are no new user facing changes in patch compared to the previous version.

This patchset still has the refactoring that Alvaro brought up upthread.

Parking this again the commitfest as it was returned with feedback from the
last one it was in (all review comments addressed, see upthread).

cheers ./daniel



0001-Refactor-backend-signalling-code-v11.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v11.patch
Description: Binary data

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

[Andres Freund]

Hi,

On 2017-06-20 13:01:35 -0700, Andres Freund wrote:
> For extensions it'd also be useful if it'd be possible to overwrite the
> error code.  E.g. for citus there's a distributed deadlock detector,
> running out of process because there's no way to interrupt lock waits
> locally, and we've to do some ugly hacking to generate proper error
> messages and code from another session.

What happened to this request?  Seems we're out of the crunch mode and
could round the feature out a littlebit more...

Greetings,

Andres Freund

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

[Daniel Gustafsson]

> On 09 Apr 2018, at 02:47, Michael Paquier  wrote:
> 
> On Fri, Apr 06, 2018 at 11:18:34AM +0200, Daniel Gustafsson wrote:
>> Yep, I completely agree.  Attached are patches with the quotes removed and
>> rebased since Oids were taken etc.
> 
> I still find this idea interesting for plugin authors.  However, as
> feature freeze for v11 is in effect, I am marking the patch as returned
> with feedback.

Not sure what the feedback is though? The patch has been through thorough
review with all review comments addressed.

Will resubmit this for a v12 CF.

cheers ./daniel

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

[Michael Paquier]

On Fri, Apr 06, 2018 at 11:18:34AM +0200, Daniel Gustafsson wrote:
> Yep, I completely agree.  Attached are patches with the quotes removed and
> rebased since Oids were taken etc.

I still find this idea interesting for plugin authors.  However, as
feature freeze for v11 is in effect, I am marking the patch as returned
with feedback.
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 06 Apr 2018, at 04:49, Michael Paquier  wrote:
> 
> On Thu, Apr 05, 2018 at 11:33:57PM +0200, Daniel Gustafsson wrote:
>> It seemed like a good idea at the time to indicate which part was submitted 
>> by
>> the user, but looking at it now the colon sign is a pretty clear indicator
>> already.
> 
> Dropping the quotes is more consistent with other error messages.  We
> don't bother about quoting things for example with system-related errors
> generated by strerror().

Yep, I completely agree.  Attached are patches with the quotes removed and
rebased since Oids were taken etc.

cheers ./daniel



0001-Refactor-backend-signalling-code-v10.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v10.patch
Description: Binary data

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

[Michael Paquier]

On Thu, Apr 05, 2018 at 11:33:57PM +0200, Daniel Gustafsson wrote:
> It seemed like a good idea at the time to indicate which part was submitted by
> the user, but looking at it now the colon sign is a pretty clear indicator
> already.

Dropping the quotes is more consistent with other error messages.  We
don't bother about quoting things for example with system-related errors
generated by strerror().
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 05 Apr 2018, at 23:16, Peter Eisentraut  
> wrote:
> 
> The documentation change suggests the error will be
> 
> ERROR:  canceling statement due to user request: "Cancellation message text"
> 
> Why the quotes?

It seemed like a good idea at the time to indicate which part was submitted by
the user, but looking at it now the colon sign is a pretty clear indicator
already.

cheers ./daniel

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

[Peter Eisentraut]

The documentation change suggests the error will be

ERROR:  canceling statement due to user request: "Cancellation message text"

Why the quotes?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

[Eren Başak]

Hi,

I have reviewed the v8 patches.

I can confirm that the patches apply and pass the tests as of 03/27/2018
11:00am (UTC).

I didn't test whether the docs compile but they look good.

I have briefly tested the patch again and can confirm that the patch does
what is intended.

The v8 patches address almost all of my review notes on v7 patch, thanks
Daniel for that.

I have some more questions, notes and views on the patch but have no strong
opinions at this moment. So I am fine with whatever decision is made:

I see you have removed extra newlines from backend_signal.c. However, I
realized that there is still one extra newline after pg_reload_conf.

> In 20170620200134.ubnv4sked5seo...@alap3.anarazel.de, Andres suggested
the same
> thing.  I don’t disagree, but I’m also not sure what the API would look
like so
> I’d prefer to address that in a follow-up patch should this one get
accepted.

That's fine for me, although I would prefer to get the ability to specify
the error code sooner than later. My main question is that I am not sure
whether the community prefers to ship two similar (in use case) changes on
an API in a single patch or fine with two patches. Can that be a problem if
the subsequent patch is released in a different postgres version? I am not
sure.

> pg_cancel_backend() is defined proisstrict, while pg_cancel_backend_msg()
is
> not.  I think we must be able to perform the cancellation if the message
is
> NULL, so made it two functions.

I agree that we should preserve the old usage as well. What I don't
understand is that, can't we remove proisstrict from pg_cancel_backend and copy
the body of pg_cancel_backend_msg into pg_cancel_backend. This way, I think
we can accomplish the same thing without introducing two new functions.

+Datum
+pg_terminate_backend_msg(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ char*msg = NULL;
+
+ if (PG_ARGISNULL(0))
+ ereport(ERROR,
+ (errmsg("pid cannot be NULL")));

pg_terminate_backend_msg errors out if the pid is null but
pg_cancel_backend_msg returns null and I think we should have consistency
between these two in this regard.

-- 
Best,
Eren

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

[Daniel Gustafsson]

> On 20 Mar 2018, at 12:12, Eren Başak  wrote:

> I reviewed the patch. Here are my notes:

Thanks!

> The patch does not add new tests (maybe isolation tests) for the new feature. 
> Are there any opportunities to have tests for confirming the new behavior?

Good point, not sure why I’ve forgotten to add this.  No existing suite seemed
to match well, so I added a new one for system administration functions like
this one.

> Not introduced with this patch, but the spacing between the functions is not 
> consistent in src/backend/storage/ipc/backend_signal.c. There are 2 newlines 
> after some functions (like pg_cancel_backend_msg) and 1 newline after others 
> (like pg_terminate_backend_msg). It would be nice to fix these while 
> refactoring.

Fixed.

> Another thing is that, in a similar manner, we could allow changing the error 
> code which might be useful for extensions. For example, Citus could use it to 
> cancel remote backends when it detects a distributed deadlock and changes the 
> error code to something retryable while doing so. For reference, see the hack 
> that Citus is currently using: 
> https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237

In 20170620200134.ubnv4sked5seo...@alap3.anarazel.de, Andres suggested the same
thing.  I don’t disagree, but I’m also not sure what the API would look like so
I’d prefer to address that in a follow-up patch should this one get accepted.

> +If the optional message parameter is set, the text
> +will be appended to the error message returned to the signalled backend.
> 
> I think providing more detail would be useful. For example we can provide an 
> example about how the error message looks like in its final form or what will 
> happen if the message is too long.

Expanded the documentation a little and added a (contrived) example.

> -pg_signal_backend(int pid, int sig)
> +pg_signal_backend(int pid, int sig, char *msg)
> 
> The new parameter (msg) is not mentioned in the function header comment. This 
> applies to pg_signal_backend, pg_cancel_backend_internal, 
> pg_terminate_backend_internal functions.

Fixed.

> +Datum
> +pg_cancel_backend(PG_FUNCTION_ARGS)
> +{
> + PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
> +}
> +
> +Datum
> +pg_cancel_backend_msg(PG_FUNCTION_ARGS)
> +{
> + pid_t   pid;
> + char   *msg = NULL;
> +
> + if (PG_ARGISNULL(0))
> + ereport(ERROR,
> + (errmsg("pid cannot be NULL")));
> +
> + pid = PG_GETARG_INT32(0);
> +
> + if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
> + msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> + PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
> +}
> 
> The first function seem redundant here because the second one covers all the 
> cases.

pg_cancel_backend() is defined proisstrict, while pg_cancel_backend_msg() is
not.  I think we must be able to perform the cancellation if the message is
NULL, so made it two functions.

Thinking more about this, I think pg_cancel_backend_msg() should handle a NULL
pid in the same way as pg_cancel_backend() so fixed that as well.

> + memset(slot->message, '\0', sizeof(slot->message));
> 
> SetBackendCancelMessage uses memset while BackendCancelShmemInit uses MemSet. 
> Not a big deal but it would be nice to be consistent and use postgres macro 
> versions for such calls. 

Good point, moved to MemSet() for both.

> +int
> +SetBackendCancelMessage(pid_t backend, char *message)
> +{
> + BackendCancelShmemStruct *slot;
> 
> The variable "slot" is declared outside of the foor loop but only used in the 
> inside, therefore it would be nicer to declare it inside the loop.

Moved to inside the loop.

> + BackendCancelInit(MyBackendId);
> 
> Is the "normal multiuser case" the best place to initialize cancellation? For 
> example, can't we cancel background workers? If this is the right place, 
> maybe we should justify why this is the best place to initialize backend 
> cancellation memory part with a comment.

I didn’t envision this being in another setting than the multiuser case, but
it’s been clear throughout the thread that others have had good ideas around
extended uses of this.  Background workers can still be terminated/canceled,
this only affects setting the message, and that is to me a multiuser feature.

Renamed the functions BackendCancelMessage*() for clarity.

> + char   *buffer = palloc0(MAX_CANCEL_MSG);
> 
> Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing?

No specific reason, changed.

> +/*
> + * Return the configured cancellation message and its length. If the returned
> + * length is greater than the size of the passed buffer, truncation has been
> + * performed. The message is cleared on reading.
> + */
> +int
> +GetCancelMessage(char **buffer, 

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

[Christoph Berg]

Re: Eren Başak 2018-03-20 

> Another thing is that, in a similar manner, we could allow changing the
> error code which might be useful for extensions. For example, Citus could
> use it to cancel remote backends when it detects a distributed deadlock and
> changes the error code to something retryable while doing so.

Another useful thing to do on top of this patch would be to include
messages when the termination comes from postgres itself, e.g. on a
server shutdown. Possibly, the message for pg_terminate_backend() itself could
say that someone invoke that, unless overridden.

FATAL:  57P01: terminating connection due to administrator command: server 
shutting down
FATAL:  57P01: terminating connection due to administrator command: restarting 
because of a crash of another server process
FATAL:  57P01: terminating connection due to administrator command: terminated 
by pg_terminate_backend()

Christoph

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

[Eren Başak]

Hi,

I reviewed the patch. Here are my notes:

I can confirm that the patches apply and pass the tests as of 03/19/2018 @
11:00am (UTC).

I didn't test whether the docs compile or look good.

I have briefly tested and can confirm that the patch does what is intended.
Here are my comments about the patch:

The patch does not add new tests (maybe isolation tests) for the new
feature. Are there any opportunities to have tests for confirming the new
behavior? At least some regression tests like the following:

SELECT pg_cancel_backend(pg_backend_pid());
ERROR:  57014: canceling statement due to user request

SELECT pg_cancel_backend(pg_backend_pid(), 'message');
ERROR:  57014: canceling statement due to user request: "message"

Not introduced with this patch, but the spacing between the functions is
not consistent in src/backend/storage/ipc/backend_signal.c. There are 2
newlines after some functions (like pg_cancel_backend_msg) and 1 newline
after others (like pg_terminate_backend_msg). It would be nice to fix these
while refactoring.

I also thought about whether the patch should allow the message to be
completely overwritten, instead of appending to the existing one and I
think it is fine. Also, adding the admin message to the HINT or DETAIL part
of the error message would make sense but these are ignored by some clients
so it is fine this way.

Another thing is that, in a similar manner, we could allow changing the
error code which might be useful for extensions. For example, Citus could
use it to cancel remote backends when it detects a distributed deadlock and
changes the error code to something retryable while doing so. For
reference, see the hack that Citus is currently using:
https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237


+If the optional message parameter is set, the text
+will be appended to the error message returned to the signalled
backend.

I think providing more detail would be useful. For example we can provide
an example about how the error message looks like in its final form or what
will happen if the message is too long.

-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, char *msg)

The new parameter (msg) is not mentioned in the function header comment.
This applies to pg_signal_backend, pg_cancel_backend_internal,
pg_terminate_backend_internal functions.

+Datum
+pg_cancel_backend(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
+}
+
+Datum
+pg_cancel_backend_msg(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ char*msg = NULL;
+
+ if (PG_ARGISNULL(0))
+ ereport(ERROR,
+ (errmsg("pid cannot be NULL")));
+
+ pid = PG_GETARG_INT32(0);
+
+ if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
+ msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}

The first function seem redundant here because the second one covers all
the cases.

+ memset(slot->message, '\0', sizeof(slot->message));

SetBackendCancelMessage uses memset while BackendCancelShmemInit uses
MemSet. Not a big deal but it would be nice to be consistent and use
postgres macro versions for such calls.

+int
+SetBackendCancelMessage(pid_t backend, char *message)
+{
+ BackendCancelShmemStruct *slot;

The variable "slot" is declared outside of the foor loop but only used in
the inside, therefore it would be nicer to declare it inside the loop.

+ BackendCancelInit(MyBackendId);

Is the "normal multiuser case" the best place to initialize cancellation?
For example, can't we cancel background workers? If this is the right
place, maybe we should justify why this is the best place to initialize
backend cancellation memory part with a comment.

+ char   *buffer = palloc0(MAX_CANCEL_MSG);

Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing?

+/*
+ * Return the configured cancellation message and its length. If the
returned
+ * length is greater than the size of the passed buffer, truncation has
been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, size_t buf_len)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+
+ if (slot != NULL && slot->len > 0)
+ {
+ SpinLockAcquire(>mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);
+ msg_length = slot->len;
+ slot->len = 0;
+ slot->message[0] = '\0';
+ SpinLockRelease(>mutex);
+ }
+
+ return msg_length;
+}

We never change what the buffer points to, therefore is it necessary to
declare `buffer` as char** instead of char*?

+extern int SetBackendCancelMessage(pid_t backend, char *message);

Maybe rename backend to something like backend_pid.

+/*
+ * Return the configured cancellation message and its length. If the
returned
+ * length is greater than the size of the passed buffer, truncation has
been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, 

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

[Daniel Gustafsson]

> On 26 Jan 2018, at 00:05, Daniel Gustafsson  wrote:
> 
>> On 24 Jan 2018, at 16:45, Alvaro Herrera  wrote:

>> Maybe have two patches, 0001 creates the files moving the contents over,
>> then 0002 adds your new stuff on top.
> 
> The two attached patches implements this.

Attached are rebased patches to cope with the recent pgproc changes.  I also
made the function cope with NULL messages, not because it’s a sensible value
but I can see this function being fed from a sub-SELECT which could return
NULL.

As per the previous mail, 0001 refactors the signal code according to Alvaros
suggestion and 0002 implements $subject on top of the refactoring.

cheers ./daniel



0001-Refactor-backend-signalling-code-v7.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v7.patch
Description: Binary data

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

[Daniel Gustafsson]

> On 24 Jan 2018, at 16:45, Alvaro Herrera  wrote:
> 
> A quick suggestion from a passer-by -- would you put the new code in
> src/backend/storage/ipc/backend_signal.c instead?  Sounds like a better
> place than utils/misc/; and "signal" is more general in nature than just
> "cancel".  A bunch of stuff from utils/adt/misc.c (???) can migrate to
> the new file -- everything from line 201 to 362 at least, that is:
> 
> pg_signal_backend
> pg_cancel_backend
> pg_terminate_backend
> pg_reload_conf
> pg_rotate_logfile

+1, this makes a lot of sense.  When writing this I didn’t find a good fit
anywhere so I modelled it after utils/misc/backend_random.c, not because it was
a good fit but it was the least bad one I could come up with.  This seems a lot
cleaner.

> Maybe have two patches, 0001 creates the files moving the contents over,
> then 0002 adds your new stuff on top.

The two attached patches implements this.

> /me wonders if there's anything that would suggest to make this
> extensive to processes other than backends ...

Perhaps, do you have an API in mind?

cheers ./daniel



0001-Refactor-backend-signalling-code-v6.patch
Description: Binary data


0002-Support-optional-message-in-backend-cancel-terminate-v6.patch
Description: Binary data

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

[Michael Paquier]

On Wed, Jan 24, 2018 at 12:45:48PM -0300, Alvaro Herrera wrote:
> /me wonders if there's anything that would suggest to make this
> extensive to processes other than backends ...

That's a good thought. Now ProcessInterrupts() is not used by
non-backend processes. For example the WAL receiver has its own logic to
handle interrupts but I guess that we could have an interface which can
be plugged in for any processes, which is by default enabled for
bgworkers.
--
Michael


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 28 Sep 2017, at 14:55, Yugo Nagata  wrote:
> 
> On Sun, 3 Sep 2017 22:47:10 +0200
> Daniel Gustafsson  wrote:
> 
> I have reviewed your latest patch. 

Thanks!

> I can apply this to the master branch and build this successfully,
> and the behavior is as expected. 
> 
> However, here are some comments and suggestions.
> 
>> 135 +   char   *buffer = palloc0(MAX_CANCEL_MSG);
>> 136 +
>> 137 +   GetCancelMessage(, MAX_CANCEL_MSG);
>> 138 +   ereport(ERROR,
>> 139 +   (errcode(ERRCODE_QUERY_CANCELED),
>> 140 +errmsg("canceling statement due to user 
>> request: \"%s\"",
>> 141 +   buffer)));
> 
> The memory for buffer is allocated here, but I think we can do this
> in GetCancelMessage. Since the size of allocated memory is fixed
> to MAX_CANCEL_MSG, it isn't neccesary to pass this to the function.
> In addition, how about returning the message as the return value?
> That is, we can define GetCancelMessage as following;
> 
>  char * GetCancelMessage(void)

I agree that it would be a much neater API, but it would mean pallocing inside
the spinlock wouldn’t it? That would be a no-no.

>> 180 +   r = SetBackendCancelMessage(pid, msg);
>> 181 +
>> 182 +   if (r != -1 && r != strlen(msg))
>> 183 +   ereport(NOTICE,
>> 184 +   (errmsg("message is too long and has been 
>> truncated")));
>> 185 +   }
> 
> We can this error handling in SetBackendCancelMessage. I think this is better
> because the truncation of message is done in this function. In addition, 
> we should use pg_mbcliplen for this truncation as done in 
> truncate_identifier. 
> Else, multibyte character boundary is broken, and noises can slip in log
> messages.

Agreed on both points.  I did however leave the returnvalue as before even
though it’s not read in this coding.

>> 235 -   int r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
>> 236 +   int r = pg_signal_backend(pid, SIGTERM, msg);
> 
> This line includes an unnecessary indentation change.

That’s embarrasing, fixed.

>> 411 + * returns the length of message actually created. If the returned 
>> length
> 
> "the length of message" might be "the length of the message”

Fixed.

>> 413 + * If the backend wasn't found and no message was set, -1 is returned. 
>> If two
> 
> This comment is incorrect since this function returns 0 when no message was 
> set.

Fixed.

>> 440 +   strlcpy(slot->message, message, sizeof(slot->message));
>> 441 +   slot->len = strlen(slot->message);
>> 442 +   message_len = slot->len;
>> 443 +   SpinLockRelease(>mutex);
>> 444 +
>> 445 +   return message_len;
> 
> This can return slot->len directly and the variable message_len is
> unnecessary. However, if we handle the "too long message" error
> in this function as suggested above, this does not have to
> return anything.

That would mean returning a variable which was set while holding the lock, when
the lock has been released.  That doesn’t seem like a good idea, even though it
may be of more theoretical importance here.

Attached patch is rebased over HEAD and addresses the above review comments.
Adding to the next commitfest as it was returned in a previous one.

cheers ./daniel



terminate_msg_v5.patch
Description: Binary data