Re: [PATCH] Cross-reference comments on signal handling logic

2021-09-27 Thread Daniel Gustafsson
> On 1 Mar 2021, at 19:22, Mark Dilger  wrote:

>> On Jan 17, 2021, at 11:51 PM, Craig Ringer  
>> wrote:
>> 
>> 
> 
> In src/backend/postmaster/interrupt.c:
> 
> + * These handlers are NOT used by normal user backends as they do not support
> 
> vs.
> 
> + * Most backends use this handler.
> 
> These two comments seem to contradict.  If interrupt.c contains handlers that 
> normal user backends to not use, then how can it be that most backends use 
> one of the handlers in the file?

I'm closing this as Returned with Feedback as it there has been no response to
the review comment during two commitfests.  Please reopen in a future
commitfest if you still would like to pursue this patch.

--
Daniel Gustafsson   https://vmware.com/





Re: [PATCH] Cross-reference comments on signal handling logic

2021-03-01 Thread Mark Dilger



> On Jan 17, 2021, at 11:51 PM, Craig Ringer  
> wrote:
> 
> 

In src/backend/postmaster/interrupt.c:

+ * These handlers are NOT used by normal user backends as they do not support

vs.

+ * Most backends use this handler.

These two comments seem to contradict.  If interrupt.c contains handlers that 
normal user backends to not use, then how can it be that most backends use one 
of the handlers in the file?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company







[PATCH] Cross-reference comments on signal handling logic

2021-01-17 Thread Craig Ringer
Hi all

The attached comments-only patch expands the signal handling section in
miscadmin.h a bit so that it mentions ProcSignal, deferred signal handling
during blocking calls, etc. It adds cross-refs between major signal
handling routines and the miscadmin comment to help readers track the
various scattered but inter-related code.

I hope this helps some new developers in future.
From a16d0a0f8f502ba3631d20d51c7bb50cedea6d57 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 18 Jan 2021 12:21:18 +0800
Subject: [PATCH v1 1/2] Comments and cross-references for signal handling

To help new developers come to terms with the complexity of signal
handling in PostgreSQL's various types backend, expand the
main comment on signal handling in miscadmin.h. Cover the ProcSignal
mechanism for SIGUSR1 multiplexing. Mention that blocking calls into 3rd
party code and uninterruptible syscalls should be avoided in backend
code because of Pg's deferred signal processing logic. Provide a set of
cross-references to key routines related to signal handling.

In various signal handling routines, cross-reference the high level
overview comment in miscadmin.c.

This should possibly become part of the developer documentation rather
than a comment in a header file, but since we already have the comment,
it seemed sensible to start by updating it and making it more
discoverable.
---
 src/backend/postmaster/interrupt.c | 21 
 src/backend/tcop/postgres.c| 26 -
 src/include/miscadmin.h| 84 --
 src/port/pgsleep.c |  2 +-
 4 files changed, 113 insertions(+), 20 deletions(-)

diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index dd9136a942..e5256179ec 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -10,6 +10,15 @@
  *	  src/backend/postmaster/interrupt.c
  *
  *-
+ *
+ * This file defines bare-bones interrupt handlers for secondary helper
+ * processes run by the postmaster - the walwriter and bgwriter, and
+ * potentially some background workers.
+ *
+ * These handlers are NOT used by normal user backends as they do not support
+ * interruption of normal execution to respond to a signal, query cancellation,
+ * etc. See miscadmin.h for details on interrupt handling used by normal
+ * postgres backends - CHECK_FOR_INTERRUPTS(), ProcessInterrupts(), die(), etc.
  */
 
 #include "postgres.h"
@@ -28,6 +37,9 @@ volatile sig_atomic_t ShutdownRequestPending = false;
 
 /*
  * Simple interrupt handler for main loops of background processes.
+ *
+ * See also CHECK_FOR_INTERRUPTS() and ProcessInterrupts() for the user-backend
+ * variant of this function.
  */
 void
 HandleMainLoopInterrupts(void)
@@ -51,6 +63,8 @@ HandleMainLoopInterrupts(void)
  * Normally, this handler would be used for SIGHUP. The idea is that code
  * which uses it would arrange to check the ConfigReloadPending flag at
  * convenient places inside main loops, or else call HandleMainLoopInterrupts.
+ *
+ * Most backends use this handler.
  */
 void
 SignalHandlerForConfigReload(SIGNAL_ARGS)
@@ -67,6 +81,9 @@ SignalHandlerForConfigReload(SIGNAL_ARGS)
  * Simple signal handler for exiting quickly as if due to a crash.
  *
  * Normally, this would be used for handling SIGQUIT.
+ *
+ * See also quickdie() and die() for the separate signal handling logic
+ * used by normal user backends.
  */
 void
 SignalHandlerForCrashExit(SIGNAL_ARGS)
@@ -99,6 +116,10 @@ SignalHandlerForCrashExit(SIGNAL_ARGS)
  *
  * ShutdownRequestPending should be checked at a convenient place within the
  * main loop, or else the main loop should call HandleMainLoopInterrupts.
+ *
+ * See also die() for the extended version of this handler that's used in
+ * backends that may need to be interrupted while performing long-running
+ * actions.
  */
 void
 SignalHandlerForShutdownRequest(SIGNAL_ARGS)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8cf1359e33..907b524e0f 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2852,8 +2852,15 @@ quickdie(SIGNAL_ARGS)
 }
 
 /*
- * Shutdown signal from postmaster: abort transaction and exit
- * at soonest convenient time
+ * Shutdown signal from postmaster: set flags to ask ProcessInterrupts() to
+ * abort the current transaction and exit at the soonest convenient time.
+ *
+ * This handler is used as the SIGTERM handler by most backend types that may
+ * run arbitrary backend code, user queries, etc. Some backends use different
+ * handlers and sometimes different flags.
+ *
+ * See "System interrupt and critical section handling" in miscadmin.h for a
+ * higher level explanation of signal handling.
  */
 void
 die(SIGNAL_ARGS)
@@ -2924,6 +2931,9 @@ FloatExceptionHandler(SIGNAL_ARGS)
  * handling following receipt of SIGUSR1. Designed to be similar to die()
  * and