Horton

Thread

Commits

  1. Allow callback functions to deregister themselves during a call.

  1. Enables to call Unregister*XactCallback() in Call*XactCallback()

    Hao Wu <gfphoenix78@gmail.com> — 2022-03-29T06:48:54Z

    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
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.

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);
     }
 }
```

Regards,
Hao Wu

  2. Re: Enables to call Unregister*XactCallback() in Call*XactCallback()

    Andres Freund <andres@anarazel.de> — 2022-04-03T16:39:23Z

    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

  3. Re: Enables to call Unregister*XactCallback() in Call*XactCallback()

    Hao Wu <gfphoenix78@gmail.com> — 2022-04-05T23:27:21Z

    > 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
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

  4. Re: Enables to call Unregister*XactCallback() in Call*XactCallback()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-26T22:05:34Z

    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

  5. Re: Enables to call Unregister*XactCallback() in Call*XactCallback()

    Nathan Bossart <nathandbossart@gmail.com> — 2022-09-26T22:13:39Z

    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

  6. Re: Enables to call Unregister*XactCallback() in Call*XactCallback()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-28T15:28:33Z

    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