On Mon, Sep 7, 2020 at 8:50 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> I think there is agreement that we're not going to change
> cancel_before_shmem_exit's restriction to only allow LIFO popping.
> So we should improve its comment to explain why. The other thing
> that seems legitimately on-the-table for this CF entry is whether
> we should change cancel_before_shmem_exit to complain, rather than
> silently do nothing, if it fails to pop the stack. Bharath's
> last patchset proposed to add an elog(DEBUG3) complaint, which
> seems to me to be just about entirely useless. I'd make it an
> ERROR, or maybe an Assert.
>
Attaching a patch with both the comments modification and changing
DEBUG3 to ERROR. make check and make world-check passes on this patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 45cb0ada52eba25ce83985cc23edab0d34be9e4a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 7 Sep 2020 23:46:26 -0700
Subject: [PATCH v4] Modify cancel_before_shmem_exit() comments
Currently, there is no mention of safe usage of cancel_before_shmem_exit()
in the function comment and also it has a mis-directing point that, there
is still scope for improving the way cancel_before_shmem_exit() looks
for callback to be removed from before_shmem_exit_list. So, this patch
modifies the comment to reflect how the caller needs to use
before_shmem_exit_list mechanism.
This patch also adds an error in case the before_shmem_exit
function is not found at the latest entry.
---
src/backend/storage/ipc/ipc.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index bdbc2c3ac4..3c2c30c189 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -381,9 +381,9 @@ on_shmem_exit(pg_on_exit_callback function, Datum arg)
* cancel_before_shmem_exit
*
* this function removes a previously-registered before_shmem_exit
- * callback. For simplicity, only the latest entry can be
- * removed. (We could work harder but there is no need for
- * current uses.)
+ * callback. We only look at the latest entry for removal, as we
+ * expect callers to add and remove temporary before_shmem_exit
+ * callbacks in strict LIFO order.
* ----------------------------------------------------------------
*/
void
@@ -393,7 +393,18 @@ cancel_before_shmem_exit(pg_on_exit_callback function, Datum arg)
before_shmem_exit_list[before_shmem_exit_index - 1].function
== function &&
before_shmem_exit_list[before_shmem_exit_index - 1].arg == arg)
+ {
--before_shmem_exit_index;
+ }
+ else if (before_shmem_exit_index > 0)
+ {
+ ereport(ERROR,
+ (errmsg("before_shmem_exit callback %p cannot be removed from the callback list as it doesn't match with the latest entry %p at index %d",
+ function,
+ before_shmem_exit_list[before_shmem_exit_index - 1].function,
+ before_shmem_exit_index),
+ errhint("Make sure the temporary before_shmem_exit callbacks are added and removed in strict Last-In-First-Out(LIFO) order.")));
+ }
}
/* ----------------------------------------------------------------
--
2.25.1
