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

From: "Joel Jacobson" <joel(at)compiler(dot)org>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Heikki Linnakangas" <hlinnaka(at)iki(dot)fi>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Avoid pallocs in async.c's SignalBackends critical section
Date: 2025-11-24 21:53:40
Message-ID: 403ba8bd-820d-47ae-b676-3bdedfa4cfce@app.fastmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2025-11-24 21:59:21 Re: Speed up COPY FROM text/CSV parsing using SIMD
Previous Message Nathan Bossart 2025-11-24 21:50:26 Re: [PATCH] Add error hints for invalid COPY options