Thread

Re: [PATCH] Avoid pallocs in async.c's SignalBackends critical section

Joel Jacobson <joel@compiler.org> — 2025-11-24T21:53:40Z

On Mon, Nov 24, 2025, at 17:06, Tom Lane wrote:
> I don't think Joel did anybody any favors by separating this patch
> fragment from its larger context [1].

I'm a bit surprised by this. My intention in splitting it out
was based on earlier advice in [1] that I think made a lot of sense:

>> [...my idea of a bgworker to kick lagging backends...]
> If you feel that that's not robust enough,
> you should split it out as a separate patch that's advertised as a
> robustness improvement not a performance improvement, and see if you
> can get anyone to bite.

In general, I think it's nice when a bigger change can be split into
smaller meaningful committable changes, which seemed possible in this
case.

Heikki also raised a point that seems worth exploring:

AtCommit_Notify currently calls asyncQueueAdvanceTail.

After PreCommit_Notify, we already know whether tryAdvanceTail is
needed, so it looks feasible to move the asyncQueueAdvanceTail call to
the end of PreCommit_Notify.

> Given the infrequency of
> complaints about failures in this area, I'm not sure that the
> notational pain of an actual critical section is justified.
>
> But I complained that the changes contemplated in [1] were raising
> the probability of failure, and while working on tamping that back
> down we decided to do something about this old gripe too.
>
> There's a relevant comment in CommitTransaction():
>
>      * This is all post-commit cleanup.  Note that if an error is raised here,
>      * it's too late to abort the transaction.  This should be just
>      * noncritical resource releasing.
>
> Unfortunately, releasing locks, sending notifies, etc is not all
> that "noncritical" if you want the DB to keep functioning well.
> But there's a good deal of code in there and making it all obey
> the critical-section rules looks painful.

I see why a critical-section is probably too painful. But since the
direction in [1] is to avoid adding new possibly risky operations to
AtCommit_Notify, I don't think it's completely unreasonable to consider
moving some existing ones into PreCommit_Notify, when feasible.

If it's preferable, I'm fine dropping this standalone patch and folding
any such adjustments into v29 in [1], or I can just leave the existing
code untouched?

/Joel