Thread: Enables to call Unregister*XactCallback() in Call*XactCallback()
Hi hackers,
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. If it's not allowed to do it
in CallXactCallback() or CallSubXactCallback(), we must find the
subtransaction when the callback is invoked, so we don't have to
unregister the callback somewhere. If it's not allowed to do it
in CallXactCallback() or CallSubXactCallback(), we must find the
right place and handle it carefully.
Luckily, we just need a few lines of code to support this feature,
by saving the next pointer before calling the callback.
by saving the next pointer before calling the callback.
The usage looks like:
```
static void
AtEOXact_cleanup_state(SubXactEvent event, SubTransactionId mySubid,
SubTransactionId parentSubid, void *arg)
{
SubXactCleanupItem *item = (SubXactCleanupItem *)arg;
Assert(FullTransactionIdEquals(item->fullXid, GetTopFullTransactionIdIfAny()));
if (item->mySubid == mySubid &&
(event == SUBXACT_EVENT_COMMIT_SUB || event == SUBXACT_EVENT_ABORT_SUB))
{
/* to do some cleanup for subtransaction */
...
UnregisterSubXactCallback(AtEOXact_cleanup_state, arg);
}
}
AtEOXact_cleanup_state(SubXactEvent event, SubTransactionId mySubid,
SubTransactionId parentSubid, void *arg)
{
SubXactCleanupItem *item = (SubXactCleanupItem *)arg;
Assert(FullTransactionIdEquals(item->fullXid, GetTopFullTransactionIdIfAny()));
if (item->mySubid == mySubid &&
(event == SUBXACT_EVENT_COMMIT_SUB || event == SUBXACT_EVENT_ABORT_SUB))
{
/* to do some cleanup for subtransaction */
...
UnregisterSubXactCallback(AtEOXact_cleanup_state, arg);
}
}
```
Regards,
Hao Wu
Attachment
Hi, 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. > 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... Greetings, Andres Freund
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's not about workloads or efficiency. Here is the use case:
I want to register a callback for some subtransaction, and only run this callback once
when the subtransaction ends, no matter if it was committed or cancelled.
It's reasonable to unregister the callback at the end of the callback, or I have to
call UnregisterSubXactCallback() somewhere. Because SubXact_callbacks/Xact_callbacks
call UnregisterSubXactCallback() somewhere. Because SubXact_callbacks/Xact_callbacks
is only set by Unregister*XactCallback(). The question now becomes
1. where to call UnregisterSubXactCallback()
2. ensure that calling UnregisterSubXactCallback() is not triggered in the current callback
This patch enables us to safely delete the current callback when the callback finishes to
implement run callback once and unregister in one place.
Regards,
Hao Wu
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;
}
On Mon, Sep 26, 2022 at 06:05:34PM -0400, Tom Lane wrote: > 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. LGTM. I have no opinion on back-patching. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Mon, Sep 26, 2022 at 06:05:34PM -0400, Tom Lane wrote:
>> 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.
> LGTM. I have no opinion on back-patching.
I had second thoughts about back-patching: doing so would encourage
extensions to rely on this working in pre-v16 branches, which they'd
better not since they might be in a not-up-to-date installation.
We could still squeeze this into v15 without creating such a hazard,
but post-rc1 doesn't seem like a good time for inessential tweaks.
Hence, pushed to HEAD only.
regards, tom lane