[PATCH] ProcessInterrupts_hook
Hi folks
A few times lately I've been doing things in extensions that've made me
want to be able to run my own code whenever InterruptPending is true and
CHECK_FOR_INTERRUPTS() calls ProcessInterrupts()
So here's a simple patch to add ProcessInterrupts_hook. It follows the
usual pattern like ProcessUtility_hook and standard_ProcessUtility.
Why? Because sometimes I want most of the behaviour of die(), but the
option to override it with some bgworker-specific choices occasionally.
HOLD_INTERRUPTS() is too big a hammer.
What I really want to go along with this is a way for any backend to
observe the postmaster's pmState and its "Shutdown" variable's value, so
any backend can tell if we're in FastShutdown, SmartShutdown, etc. Copies
in shmem only obviously. But I'm not convinced it's right to just copy
these vars as-is to shmem, and I don't want to use the memory for a
ProcSignal slot for something that won't be relevant for most backends for
most of the postmaster lifetime. Ideas welcomed.
Attachments:
v1-0001-Provide-a-hook-for-ProcessInterrupts.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Provide-a-hook-for-ProcessInterrupts.patchDownload
From 1c8c477a5814420011fa034323e82d8eabc6bc5f Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig.ringer@2ndquadrant.com>
Date: Mon, 18 Jan 2021 14:14:46 +0800
Subject: [PATCH v1 1/4] Provide a hook for ProcessInterrupts()
Code may now register a ProcessInterrupts_hook to fire its own
logic before and/or after the main ProcessInterrupts handler
for signal processing. The hook must call standard_ProcessInterrupts
to execute the normal interrupt handling or set InterruptsPending to
true to cause the interrupt to be re-processed at the next opportunity.
This follows the consistent pattern used by other PostgreSQL hooks like
the ProcessUtility_hook and standard_ProcessUtility() function.
The purpose of this hook is to allow extensions to react to their own
custom interrupt flags during CHECK_FOR_INTERRUPTS() calls invoked
in main PostgreSQL code paths.
---
src/backend/tcop/postgres.c | 20 ++++++++++++++++++++
src/include/miscadmin.h | 3 +++
2 files changed, 23 insertions(+)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 28055680aa..8cf1359e33 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -102,6 +102,8 @@ int max_stack_depth = 100;
/* wait N seconds to allow attach from a debugger */
int PostAuthDelay = 0;
+/* Intercept CHECK_FOR_INTERRUPTS() responses */
+ProcessInterrupts_hook_type ProcessInterrupts_hook = NULL;
/* ----------------
@@ -3064,8 +3066,26 @@ ProcessInterrupts(void)
/* OK to accept any interrupts now? */
if (InterruptHoldoffCount != 0 || CritSectionCount != 0)
return;
+
InterruptPending = false;
+ if (ProcessInterrupts_hook)
+ ProcessInterrupts_hook();
+ else
+ standard_ProcessInterrupts();
+}
+
+/*
+ * Implement the default signal handling behaviour for most backend types
+ * including user backends and bgworkers.
+ *
+ * This is where CHECK_FOR_INTERRUPTS() eventually lands up unless intercepted
+ * by ProcessInterrupts_hook.
+ */
+void
+standard_ProcessInterrupts(void)
+{
+
if (ProcDiePending)
{
ProcDiePending = false;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1bdc97e308..f082d04448 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -94,6 +94,9 @@ extern PGDLLIMPORT volatile uint32 CritSectionCount;
/* in tcop/postgres.c */
extern void ProcessInterrupts(void);
+typedef void (*ProcessInterrupts_hook_type)(void);
+extern ProcessInterrupts_hook_type ProcessInterrupts_hook;
+extern void standard_ProcessInterrupts(void);
#ifndef WIN32
--
2.29.2
v1-0002-Test-for-ProcessInterrupts_hook.patchtext/x-patch; charset=US-ASCII; name=v1-0002-Test-for-ProcessInterrupts_hook.patchDownload
From 58b9ac884ef5bd35a2aac9da85079e24097612be Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig.ringer@2ndquadrant.com>
Date: Mon, 18 Jan 2021 15:26:43 +0800
Subject: [PATCH v1 2/4] Test for ProcessInterrupts_hook
---
src/test/modules/Makefile | 3 +-
src/test/modules/test_hooks/.gitignore | 4 +
src/test/modules/test_hooks/Makefile | 28 ++++
.../expected/test_processinterrupt_hook.out | 0
src/test/modules/test_hooks/hooks.conf | 1 +
.../sql/test_processinterrupt_hook.sql | 16 ++
.../modules/test_hooks/test_hooks--1.0.sql | 8 +
src/test/modules/test_hooks/test_hooks.c | 37 +++++
.../modules/test_hooks/test_hooks.control | 4 +
src/test/modules/test_hooks/test_hooks.h | 20 +++
.../test_hooks/test_process_interrupts_hook.c | 140 ++++++++++++++++++
11 files changed, 260 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/test_hooks/.gitignore
create mode 100644 src/test/modules/test_hooks/Makefile
create mode 100644 src/test/modules/test_hooks/expected/test_processinterrupt_hook.out
create mode 100644 src/test/modules/test_hooks/hooks.conf
create mode 100644 src/test/modules/test_hooks/sql/test_processinterrupt_hook.sql
create mode 100644 src/test/modules/test_hooks/test_hooks--1.0.sql
create mode 100644 src/test/modules/test_hooks/test_hooks.c
create mode 100644 src/test/modules/test_hooks/test_hooks.control
create mode 100644 src/test/modules/test_hooks/test_hooks.h
create mode 100644 src/test/modules/test_hooks/test_process_interrupts_hook.c
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 59921b46cf..b6f1f25451 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -26,7 +26,8 @@ SUBDIRS = \
test_rls_hooks \
test_shm_mq \
unsafe_tests \
- worker_spi
+ worker_spi \
+ test_hooks
ifeq ($(with_openssl),yes)
SUBDIRS += ssl_passphrase_callback
diff --git a/src/test/modules/test_hooks/.gitignore b/src/test/modules/test_hooks/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_hooks/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_hooks/Makefile b/src/test/modules/test_hooks/Makefile
new file mode 100644
index 0000000000..50be27ea34
--- /dev/null
+++ b/src/test/modules/test_hooks/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/test_hooks/Makefile
+
+MODULE_big = test_hooks
+OBJS = \
+ $(WIN32RES) \
+ test_hooks.o \
+ test_process_interrupts_hook.o
+PGFILEDESC = "test_hooks - example use of assorted PostgreSQL hooks"
+
+EXTENSION = test_hooks
+DATA = test_hooks--1.0.sql
+
+REGRESS = test_processinterrupt_hook
+REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_hooks/hooks.conf
+# Disabled because these tests require "shared_preload_libraries=test_hooks",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_hooks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_hooks/expected/test_processinterrupt_hook.out b/src/test/modules/test_hooks/expected/test_processinterrupt_hook.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/src/test/modules/test_hooks/hooks.conf b/src/test/modules/test_hooks/hooks.conf
new file mode 100644
index 0000000000..d574606694
--- /dev/null
+++ b/src/test/modules/test_hooks/hooks.conf
@@ -0,0 +1 @@
+shared_preload_libraries = test_hooks
diff --git a/src/test/modules/test_hooks/sql/test_processinterrupt_hook.sql b/src/test/modules/test_hooks/sql/test_processinterrupt_hook.sql
new file mode 100644
index 0000000000..4de9712346
--- /dev/null
+++ b/src/test/modules/test_hooks/sql/test_processinterrupt_hook.sql
@@ -0,0 +1,16 @@
+CREATE EXTENSION test_hooks;
+
+SELECT get_times_processed_interrupts() > 0;
+
+SELECT test_hold_interrupts_hook();
+
+SELECT set_exit_deferred(true);
+SELECT pg_terminate_backend(pg_backend_pid());
+SELECT 1;
+SELECT set_exit_deferred(false);
+SELECT 1;
+
+SELECT pg_backend_pid();
+SELECT pg_terminate_backend(pg_backend_pid());
+SELECT pg_backend_pid();
+SELECT 1;
diff --git a/src/test/modules/test_hooks/test_hooks--1.0.sql b/src/test/modules/test_hooks/test_hooks--1.0.sql
new file mode 100644
index 0000000000..5a9ea757a7
--- /dev/null
+++ b/src/test/modules/test_hooks/test_hooks--1.0.sql
@@ -0,0 +1,8 @@
+CREATE FUNCTION get_times_processed_interrupts()
+RETURNS pg_catalog.int4 LANGUAGE c VOLATILE AS 'MODULE_PATHNAME';
+
+CREATE FUNCTION set_exit_deferred(pg_catalog.bool)
+RETURNS pg_catalog.void LANGUAGE c VOLATILE AS 'MODULE_PATHNAME';
+
+CREATE FUNCTION test_hold_interrupts_hook()
+RETURNS pg_catalog.void LANGUAGE c VOLATILE AS 'MODULE_PATHNAME';
diff --git a/src/test/modules/test_hooks/test_hooks.c b/src/test/modules/test_hooks/test_hooks.c
new file mode 100644
index 0000000000..9d5422e2ac
--- /dev/null
+++ b/src/test/modules/test_hooks/test_hooks.c
@@ -0,0 +1,37 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_hooks.c
+ * Code for testing RLS hooks.
+ *
+ * Copyright (c) 2015-2021, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_hooks/test_hooks.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "test_hooks.h"
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+void _PG_fini(void);
+
+/* Install hooks */
+void
+_PG_init(void)
+{
+ install_test_process_interrupts_hook();
+}
+
+/* Uninstall hooks */
+void
+_PG_fini(void)
+{
+ uninstall_test_process_interrupts_hook();
+}
diff --git a/src/test/modules/test_hooks/test_hooks.control b/src/test/modules/test_hooks/test_hooks.control
new file mode 100644
index 0000000000..539f5f0fa3
--- /dev/null
+++ b/src/test/modules/test_hooks/test_hooks.control
@@ -0,0 +1,4 @@
+comment = 'Test code for various PostgreSQL hooks'
+default_version = '1.0'
+module_pathname = '$libdir/test_hooks'
+relocatable = true
diff --git a/src/test/modules/test_hooks/test_hooks.h b/src/test/modules/test_hooks/test_hooks.h
new file mode 100644
index 0000000000..6879a0e3bd
--- /dev/null
+++ b/src/test/modules/test_hooks/test_hooks.h
@@ -0,0 +1,20 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_hooks.h
+ * Definitions for various PostgreSQL hooks
+ *
+ * Copyright (c) 2021, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_hooks/test_hooks.h
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#ifndef TEST_HOOKS_H
+#define TEST_HOOKS_H
+
+extern void install_test_process_interrupts_hook(void);
+extern void uninstall_test_process_interrupts_hook(void);
+
+#endif
diff --git a/src/test/modules/test_hooks/test_process_interrupts_hook.c b/src/test/modules/test_hooks/test_process_interrupts_hook.c
new file mode 100644
index 0000000000..8f39ad5e91
--- /dev/null
+++ b/src/test/modules/test_hooks/test_process_interrupts_hook.c
@@ -0,0 +1,140 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_process_interrupts_hook.c
+ * Code for testing RLS hooks.
+ *
+ * Copyright (c) 2021, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_hooks/test_process_interrupts_hook.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/latch.h"
+#include "miscadmin.h"
+#include "pgstat.h"
+#include "test_hooks.h"
+
+/* Saved hook values in case of unload */
+static ProcessInterrupts_hook_type original_ProcessInterrupts_hook = NULL;
+
+/* Next hooks to call in the chain */
+static ProcessInterrupts_hook_type next_ProcessInterrupts_hook = NULL;
+
+static int times_processed_interrupts;
+static bool exit_deferred;
+static bool saved_ProcDiePending;
+
+static void
+test_ProcessInterrupts_hook(void)
+{
+ /* Hook never gets to fire if interrupts are held */
+ Assert(InterruptHoldoffCount == 0);
+
+ /*
+ * Typically you'd HOLD_INTERRUPTS() instead of this hack to defer
+ * interrupts. But just as an example, this extension will defer
+ * processing of SIGTERM via die() while acting normally on other
+ * signals.
+ *
+ * It'll react to the die() when exit_deferred=false is set and its
+ * latch is set or it's woken for some other reason, since it does not
+ * restore InterruptsPending=true.
+ *
+ * This relies on running in a normal user backend that uses die()
+ * as its SIGTERM handler.
+ */
+ if (exit_deferred && ProcDiePending)
+ {
+ saved_ProcDiePending = true;
+ ProcDiePending = false;
+ }
+ else if (!exit_deferred && saved_ProcDiePending)
+ {
+ ProcDiePending = true;
+ saved_ProcDiePending = false;
+ }
+
+ if (next_ProcessInterrupts_hook)
+ next_ProcessInterrupts_hook();
+ else
+ standard_ProcessInterrupts();
+
+ times_processed_interrupts ++;
+}
+
+void
+install_test_process_interrupts_hook(void)
+{
+ /* Save values for unload */
+ original_ProcessInterrupts_hook = ProcessInterrupts_hook;
+
+ /* Set our hooks */
+ next_ProcessInterrupts_hook = ProcessInterrupts_hook;
+ ProcessInterrupts_hook = test_ProcessInterrupts_hook;
+
+ times_processed_interrupts = 0;
+ exit_deferred = false;
+}
+
+void
+uninstall_test_process_interrupts_hook(void)
+{
+ if (ProcessInterrupts_hook != test_ProcessInterrupts_hook)
+ elog(ERROR, "cannot unload test_hooks: another hook has been registered on the ProcessInterrupts_hook");
+
+ ProcessInterrupts_hook = original_ProcessInterrupts_hook;
+}
+
+PG_FUNCTION_INFO_V1(get_times_processed_interrupts);
+
+Datum
+get_times_processed_interrupts(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT32(times_processed_interrupts);
+}
+
+PG_FUNCTION_INFO_V1(set_exit_deferred);
+
+Datum
+set_exit_deferred(PG_FUNCTION_ARGS)
+{
+ if (PG_ARGISNULL(0))
+ elog(ERROR, "argument must be non-null");
+ exit_deferred = PG_GETARG_BOOL(0);
+ if (!exit_deferred)
+ InterruptPending = true;
+ PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(test_hold_interrupts_hook);
+
+/*
+ * Make sure the hook doesn't fire when interrupts are held
+ */
+Datum
+test_hold_interrupts_hook(PG_FUNCTION_ARGS)
+{
+ int rc;
+
+ HOLD_INTERRUPTS();
+
+ SetLatch(MyLatch);
+
+ CHECK_FOR_INTERRUPTS();
+
+ rc = WaitLatch(MyLatch, WL_LATCH_SET|WL_TIMEOUT|WL_EXIT_ON_PM_DEATH, 1000, PG_WAIT_EXTENSION);
+
+ RESUME_INTERRUPTS();
+
+ if (rc & WL_LATCH_SET)
+ elog(ERROR, "received latch set event with interrupts held");
+ if (rc & WL_TIMEOUT)
+ elog(INFO, "timed out as expected");
+
+ PG_RETURN_VOID();
+}
--
2.29.2
On Mon, Jan 18, 2021 at 3:00 AM Craig Ringer
<craig.ringer@enterprisedb.com> wrote:
A few times lately I've been doing things in extensions that've made me want to be able to run my own code whenever InterruptPending is true and CHECK_FOR_INTERRUPTS() calls ProcessInterrupts()
I've wanted this in the past, too, so +1 from me.
What I really want to go along with this is a way for any backend to observe the postmaster's pmState and its "Shutdown" variable's value, so any backend can tell if we're in FastShutdown, SmartShutdown, etc. Copies in shmem only obviously. But I'm not convinced it's right to just copy these vars as-is to shmem, and I don't want to use the memory for a ProcSignal slot for something that won't be relevant for most backends for most of the postmaster lifetime. Ideas welcomed.
I've wanted something along this line, too, but what I was thinking
about was more along the lines of having the postmaster signal the
backends when a smart shutdown happened. After all when a fast
shutdown happens the backends already get told to terminate, and that
seems like it ought to be enough: I'm not sure backends have any
business caring about why they are being asked to terminate. But they
might well want to know whether a smart shutdown is in progress, and
right now there's no way for them to know that.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jan 18, 2021 at 3:00 AM Craig Ringer
<craig.ringer@enterprisedb.com> wrote:A few times lately I've been doing things in extensions that've made me want to be able to run my own code whenever InterruptPending is true and CHECK_FOR_INTERRUPTS() calls ProcessInterrupts()
I've wanted this in the past, too, so +1 from me.
I dunno, this seems pretty scary and easily abusable. There's not all
that much that can be done safely in ProcessInterrupts(), and we should
not be encouraging extensions to think they can add random processing
there.
What I really want to go along with this is a way for any backend to observe the postmaster's pmState and its "Shutdown" variable's value, so any backend can tell if we're in FastShutdown, SmartShutdown, etc.
I've wanted something along this line, too, but what I was thinking
about was more along the lines of having the postmaster signal the
backends when a smart shutdown happened.
We're about halfway there already, see 7e784d1dc. I didn't do the
other half because it wasn't necessary to the problem, but exposing
the shutdown state more fully seems reasonable.
regards, tom lane
On Mon, Jan 18, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I've wanted this in the past, too, so +1 from me.
I dunno, this seems pretty scary and easily abusable. There's not all
that much that can be done safely in ProcessInterrupts(), and we should
not be encouraging extensions to think they can add random processing
there.
We've had this disagreement before about other things, and I just
don't agree. If somebody uses a hook for something wildly unsafe, that
will break their stuff, not ours. That's not to say I endorse adding
hooks for random purposes in random places. In particular, if it's
impossible to use a particular hook in a reasonably safe way, that's a
sign that the hook is badly-designed and that we should not have it.
But, that's not the case here. I care more about smart extension
authors being able to do useful things than I do about the possibility
that dumb extension authors will do stupid things. We can't really
prevent the latter anyway: this is open source.
We're about halfway there already, see 7e784d1dc. I didn't do the
other half because it wasn't necessary to the problem, but exposing
the shutdown state more fully seems reasonable.
Ah, I hadn't realized.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, 19 Jan 2021, 02:01 Robert Haas, <robertmhaas@gmail.com> wrote:
On Mon, Jan 18, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I've wanted this in the past, too, so +1 from me.
I dunno, this seems pretty scary and easily abusable. There's not all
that much that can be done safely in ProcessInterrupts(), and we should
not be encouraging extensions to think they can add random processing
there.We've had this disagreement before about other things, and I just
don't agree. If somebody uses a hook for something wildly unsafe, that
will break their stuff, not ours.
Generally yeah.
And we have no shortage of hooks with plenty of error or abuse potential
and few safeguards already. I'd argue that in C code any external code is
inherently unsafe anyway. So it's mainly down to whether the hook actively
encourages unsafe actions without providing commensurate benefits, and
whether there's a better/safer way to achieve the same thing.
That's not to say I endorse adding
hooks for random purposes in random places. In particular, if it's
impossible to use a particular hook in a reasonably safe way, that's a
sign that the hook is badly-designed and that we should not have it.
Yep. Agreed.
Any hook is possible to abuse or write incorrectly, from simple fmgr
loadable functions right on up.
The argument that a hook could be abused would apply just as well to
exposing pqsignal() itself to extensions. Probably more so. Also to
anything like ProcessUtility_hook.
We're about halfway there already, see 7e784d1dc. I didn't do the
other half because it wasn't necessary to the problem, but exposing
the shutdown state more fully seems reasonable.
Excellent, I'll take a look. Thanks.
On Tue, 19 Jan 2021 at 12:44, Craig Ringer <craig.ringer@enterprisedb.com>
wrote:
We're about halfway there already, see 7e784d1dc. I didn't do the
other half because it wasn't necessary to the problem, but exposing
the shutdown state more fully seems reasonable.Excellent, I'll take a look. Thanks.
That looks very handy already.
Extending it to be set before SIGTERM too would be handy.
My suggestion, which I'm happy to post in patch form if you think it's
reasonable:
* Change QuitSignalReason to ExitSignalReason to cover both SIGTERM (fast)
and SIGQUIT (immediate)
* Rename PMQUIT_FOR_STOP to PMEXIT_IMMEDIATE_SHUTDOWN
* Add enumeration values PMEXIT_SMART_SHUTDOWN and PMEXIT_FAST_SHUTDOWN
* For a fast shutdown, in pmdie()'s SIGINT case call
SetExitSignalReason(PMEXIT_FAST_SHUTDOWN), so that when
PostmasterStateMachine() calls SignalSomeChildren(SIGTERM, ...) in response
to PM_STOP_BACKENDS, the reason is already available.
* For smart shutdown, in pmdie()'s SIGTERM case call
SetExitSignalReason(PMEXIT_SMART_SHUTDOWN) and set the latch of every live
backend. There isn't any appropriate PROCSIG so unless we want to overload
PROCSIG_WALSND_INIT_STOPPING (ick), but I think it'd generally be
sufficient to check GetExitSignalReason() in backend main loops.
The fast shutdown case seems like a no-brainer extension of your existing
patch.
I'm not entirely sure about the smart shutdown case. I don't want to add a
ProcSignal slot just for this and the info isn't urgent anyway. I think
that by checking for postmaster shutdown in the backend main loop we'd be
able to support eager termination of idle backends on smart shutdown
(immediately, or after an idle grace period), which is something I've
wanted for quite some time. It shouldn't be significantly expensive
especially in the idle loop.
Thoughts?
(Also I want a hook in PostgresMain's idle loop for things like this).
On 1/19/21 1:42 AM, Craig Ringer wrote:
On Tue, 19 Jan 2021 at 12:44, Craig Ringer
<craig.ringer@enterprisedb.com <mailto:craig.ringer@enterprisedb.com>>
wrote:We're about halfway there already, see 7e784d1dc. I didn't
do the
other half because it wasn't necessary to the problem, but
exposing
the shutdown state more fully seems reasonable.
Excellent, I'll take a look. Thanks.
That looks very handy already.
Extending it to be set before SIGTERM too would be handy.
My suggestion, which I'm happy to post in patch form if you think it's
reasonable <snip>
Tom, Robert, and thoughts on the proposals in [1]/messages/by-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz+GZAQDAco1Fbb01w@mail.gmail.com?
Craig, based on the state of this proposal (i.e. likely not a candidate
for PG14) I think it makes sense to move it to the next CF.
Regards,
--
-David
david@pgmasters.net
[1]: /messages/by-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz+GZAQDAco1Fbb01w@mail.gmail.com
/messages/by-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz+GZAQDAco1Fbb01w@mail.gmail.com
David Steele <david@pgmasters.net> writes:
On 1/19/21 1:42 AM, Craig Ringer wrote:
My suggestion, which I'm happy to post in patch form if you think it's
reasonable <snip>
Tom, Robert, and thoughts on the proposals in [1]?
/messages/by-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz+GZAQDAco1Fbb01w@mail.gmail.com
No objection to generalizing the state passed through pmsignal.c.
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe from the
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.
regards, tom lane
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Steele <david@pgmasters.net> writes:
On 1/19/21 1:42 AM, Craig Ringer wrote:
My suggestion, which I'm happy to post in patch form if you think it's
reasonable <snip>Tom, Robert, and thoughts on the proposals in [1]?
/messages/by-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz+GZAQDAco1Fbb01w@mail.gmail.comNo objection to generalizing the state passed through pmsignal.c.
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe from the
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.
It should be unnecessary anyway. We changed it a while back to make
any SIGUSR1 set the latch ....
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe from the
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.
It should be unnecessary anyway. We changed it a while back to make
any SIGUSR1 set the latch ....
Hmm, so the postmaster could send SIGUSR1 without setting any particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.
regards, tom lane
On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe from the
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.It should be unnecessary anyway. We changed it a while back to make
any SIGUSR1 set the latch ....Hmm, so the postmaster could send SIGUSR1 without setting any particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.
I'd be fine with either way.
I don't expect to be able to get to working on a concrete patch for this
any time soon, so I'll be leaving it here unless someone else needs to pick
it up for their extension work. The in-principle agreement is there for
future work anyway.
On Tue, Jun 29, 2021 at 01:32:26PM +0800, Craig Ringer wrote:
On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe from the
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.It should be unnecessary anyway. We changed it a while back to make
any SIGUSR1 set the latch ....Hmm, so the postmaster could send SIGUSR1 without setting any particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.I'd be fine with either way.
I don't expect to be able to get to working on a concrete patch for this
any time soon, so I'll be leaving it here unless someone else needs to pick
it up for their extension work. The in-principle agreement is there for
future work anyway.
Hi Craig,
There is still a CF entry for this. Should we close it as withdrawn? or
maybe RwF?
--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
On Sat, 2 Oct 2021 at 01:24, Jaime Casanova <jcasanov@systemguards.com.ec>
wrote:
On Tue, Jun 29, 2021 at 01:32:26PM +0800, Craig Ringer wrote:
On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not very comfortable about the idea of having the postmaster set
child processes' latches ... that doesn't sound terribly safe fromthe
standpoint of not allowing the postmaster to mess with shared memory
state that could cause it to block or crash. If we already do that
elsewhere, then OK, but I don't think we do.It should be unnecessary anyway. We changed it a while back to make
any SIGUSR1 set the latch ....Hmm, so the postmaster could send SIGUSR1 without setting any
particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.I'd be fine with either way.
I don't expect to be able to get to working on a concrete patch for this
any time soon, so I'll be leaving it here unless someone else needs topick
it up for their extension work. The in-principle agreement is there for
future work anyway.Hi Craig,
There is still a CF entry for this. Should we close it as withdrawn? or
maybe RwF?
I'm not going to get time for it now, so I think marking it withdrawn is
reasonable.
I think it's well worth doing and Tom seems to think it's not a crazy idea,
but I'm no longer working on the software that needed it, and don't see a
lot of other people calling for it, so it can wait until someone else needs
it.