Re: Enables to call Unregister*XactCallback() in Call*XactCallback() - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> On 2022-03-29 14:48:54 +0800, Hao Wu wrote:
>> It's a natural requirement to unregister the callback for transaction or
>> subtransaction when the callback is invoked, so we don't have to
>> unregister the callback somewhere.

> You normally shouldn'd need to do this frequently - what's your use case?
> UnregisterXactCallback() is O(N), so workloads registering / unregistering a
> lot of callbacks would be problematic.

It'd only be slow if you had a lot of distinct callbacks registered
at the same time, which doesn't sound like a common situation.

>> Luckily, we just need a few lines of code to support this feature,
>> by saving the next pointer before calling the callback.

> That seems reasonable...

Yeah.  Whether it's efficient or not, seems like it should *work*.
I'm a bit inclined to call this a bug-fix and backpatch it.

I went looking for other occurrences of this code in places that have
an unregister function, and found one in ResourceOwnerReleaseInternal,
so I think we should fix that too.  Also, a comment seems advisable;
that leads me to the attached v2.

            regards, tom lane

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 2bb975943c..c1ffbd89b8 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3656,9 +3656,14 @@ static void
 CallXactCallbacks(XactEvent event)
 {
     XactCallbackItem *item;
+    XactCallbackItem *next;

-    for (item = Xact_callbacks; item; item = item->next)
+    for (item = Xact_callbacks; item; item = next)
+    {
+        /* allow callbacks to unregister themselves when called */
+        next = item->next;
         item->callback(event, item->arg);
+    }
 }


@@ -3713,9 +3718,14 @@ CallSubXactCallbacks(SubXactEvent event,
                      SubTransactionId parentSubid)
 {
     SubXactCallbackItem *item;
+    SubXactCallbackItem *next;

-    for (item = SubXact_callbacks; item; item = item->next)
+    for (item = SubXact_callbacks; item; item = next)
+    {
+        /* allow callbacks to unregister themselves when called */
+        next = item->next;
         item->callback(event, mySubid, parentSubid, item->arg);
+    }
 }


diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index ece5d98261..37b43ee1f8 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -501,6 +501,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
     ResourceOwner child;
     ResourceOwner save;
     ResourceReleaseCallbackItem *item;
+    ResourceReleaseCallbackItem *next;
     Datum        foundres;

     /* Recurse to handle descendants */
@@ -701,8 +702,12 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
     }

     /* Let add-on modules get a chance too */
-    for (item = ResourceRelease_callbacks; item; item = item->next)
+    for (item = ResourceRelease_callbacks; item; item = next)
+    {
+        /* allow callbacks to unregister themselves when called */
+        next = item->next;
         item->callback(phase, isCommit, isTopLevel, item->arg);
+    }

     CurrentResourceOwner = save;
 }