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

Hi,

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.

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.

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

> Maybe fixing that end of things would be a simpler way of buying back
> the delta.

I don't think that'll do the trick - in the case I'm looking at none of the
other timers are active...

> > Or we could add a timeout.c API that specifies the timeout?
>
> Don't think that will help: it'd be morally equivalent to
> enable_timeout_at(), which also has to do GetCurrentTimestamp().

I should have been more precise - what I meant was a timeout.c API that allows
the caller to pass in "now", which in this case we'd get from
GetCurrentTransactionStopTimestamp(), which would avoid the additional
timestamp computation.

Greetings,

Andres Freund