Re: non-exclusive backup cleanup is mildly broken

At Tue, 17 Dec 2019 11:46:03 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in 
> On Tue, Dec 17, 2019 at 4:19 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Sun, Dec 15, 2019 at 8:44 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > > However I don't object to the restriction, couldn't we allow the
> > > cancel_before_shmem_exit to search for the given entry looping over
> > > the before_shmem_exit array? If we don't do that, an assrtion is needed
> > > instead.
> > >
> > > Since pg_stop_backup_v2 is the only caller to the function in the
> > > whole server code, making cancel_before_shmem_exit a bit wiser (and
> > > slower) cannot hurt anyone.
> >
> > That's actually not true. It's called from
> > PG_END_ENSURE_ERROR_CLEANUP. Still, it wouldn't cost a lot to fix this
> > that way. However, I think that it's better to fix it the other way,
> > as I mentioned in my original email.

Sorry. I knew that.

> +1
> 
> Not only PREPARE but also other commands that we may add in the future
> can cause the same issue, so it's better to address the root cause rather
> than working around by disallowing PREPARE.

I stand on that side. I'm not sure what we are thinking as the root
cause, but PREPARE is avoiding duplicate registration using the static
bool twophaseExitRegistered and the most reasonable way to fix the
crash of the current patch would be to do the same thing as
PREPARE. The attached does that and changes the if condition of
cancel_before_shmem_exit into assertion.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index cac5854fcf..6f13084c43 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -80,6 +80,7 @@ pg_start_backup(PG_FUNCTION_ARGS)
     }
     else
     {
+        static bool exit_handler_registered = false;
         MemoryContext oldcontext;
 
         /*
@@ -91,7 +92,12 @@ pg_start_backup(PG_FUNCTION_ARGS)
         tblspc_map_file = makeStringInfo();
         MemoryContextSwitchTo(oldcontext);
 
-        before_shmem_exit(do_pg_abort_backup, BoolGetDatum(true));
+        /* We don't remove this handler so register it only once */
+        if (!exit_handler_registered)
+        {
+            before_shmem_exit(do_pg_abort_backup, BoolGetDatum(true));
+            exit_handler_registered = true;
+        }
 
         startpoint = do_pg_start_backup(backupidstr, fast, NULL, label_file,
                                         NULL, tblspc_map_file, false, true);
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 05d02c23f5..44a1b24009 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -389,11 +389,11 @@ on_shmem_exit(pg_on_exit_callback function, Datum arg)
 void
 cancel_before_shmem_exit(pg_on_exit_callback function, Datum arg)
 {
-    if (before_shmem_exit_index > 0 &&
-        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;
+    Assert(before_shmem_exit_index > 0 &&
+           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;
 }
 
 /* ----------------------------------------------------------------