Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

Hi,

On 2022-06-17 10:30:55 -0700, Andres Freund wrote:
> On 2022-06-17 10:33:08 -0400, Tom Lane wrote:
> > Andres Freund <andres(at)anarazel(dot)de> writes:
> > > The remaining difference looks like it's largely caused by the
> > > enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, ...) introduced as part of the
> > > pgstats patch. It's only really visible when I pin a single connection pgbench
> > > to the same CPU core as the server (which gives a ~16% boost here).
> >
> > > It's not the timeout itself - that we amortize nicely (via 09cf1d522). It's
> > > that enable_timeout_after() does a GetCurrentTimestamp().
> >
> > > Not sure yet what the best way to fix that is.
> >
> > Maybe not queue a new timeout if the old one is still active?
>
> Right now we disable the timer after ReadCommand(). We can of course change
> that. At first I thought we might need more bookkeeping to do so, to avoid
> ProcessInterrupts() triggering pgstat_report_stat() when the timer fires
> later, but we probably can jury-rig something with DoingCommandRead &&
> IsTransactionOrTransactionBlock() or such.

Here's a patch for that.

One thing I noticed is that disable_timeout() calls do
schedule_alarm(GetCurrentTimestamp()) if there's any other active timeout,
even if the to-be-disabled timer is already disabled. Of course callers of
disable_timeout() can guard against that using get_timeout_active(), but that
spreads repetitive code around...

I opted to add a fastpath for that, instead of using
get_timeout_active(). Afaics that's safe to do without disarming the signal
handler, but I'd welcome a look from somebody that knows this code.

> I guess one advantage of something like this could be that we could possibly
> move the arming of the timeout to pgstat.c. But that looks like it might be
> more complicated than really worth it.

I didn't do that yet, but am curious whether others think this would be
preferrable.

> > BTW, it looks like that patch also falsified this comment
> > (postgres.c:4478):
> >
> > * At most one of these timeouts will be active, so there's no need to
> > * worry about combining the timeout.c calls into one.
>
> Hm, yea. I guess we can just disable them at once.

With the proposed change we don't need to change the separate timeout.c to
one, or update the comment, as it should now look the same as 14.

I also attached my heavily-WIP patches for the ExprEvalStep issues, I
accidentally had only included a small part of the contents of the json fix.

Greetings,

Andres Freund