Andres Freund <[email protected]> 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;
}
