On Fri, Aug 7, 2020 at 11:09 PM Robert Haas <robertmh...@gmail.com> wrote:
>
> On Fri, Aug 7, 2020 at 1:12 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> > That's a meaningless statement for any one caller. So it needs to be more
> > like "we expect callers to add and remove temporary before_shmem_exit
> > callbacks in strict LIFO order".
>
> Sure, that seems fine.
>
v2 patch has the comments modified.
>
> > I wonder whether we ought to change the function to complain if the
> > last list entry doesn't match. We'd have caught this bug sooner
> > if it did, and it's not very clear why silently doing nothing is
> > a good idea when there's no match.
>
> +1.
>
This is a good idea. v3 patch has both the modified comments(from v2)
as well as a DEBUG3 (DEBUG3 level, because the other
non-error/non-fatal logs in ipc.c are using the same level) log to
report when the latest entry for removal is not matched with the one
the caller cancel_before_shmem_exit() is looking for and a hint on how
to safely use temporary before_shmem_exit() callbacks. In v3 patch,
function pointer is being printed, I'm not sure how much it is helpful
to have function pointers in the logs though there are some other
places printing pointers into the logs, I wish I could print function
names. (Is there a way we could get function names from function
pointers?).
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 7cdea387c2e18eb537077ccf518747b9cffb405c Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 10 Aug 2020 10:42:23 +0530
Subject: [PATCH v2] 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 point that, there is still
scope for improving the way cancel_before_shmem_exit() looks for callback
to 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.
---
src/backend/storage/ipc/ipc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index bdbc2c3ac4..3ca5e7c89c 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
--
2.25.1
From b41e4fa116f1e6eb5d1cee25f9fc6b0fe020a8af Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 10 Aug 2020 12:13:31 +0530
Subject: [PATCH v3] 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 point that, there is still
scope for improving the way cancel_before_shmem_exit() looks for callback
to 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.
---
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..3cc5cf9b9b 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(DEBUG3,
+ (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
