Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <dgrowleyml@gmail.com>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Nikita Glukhov <n.gluhov@postgrespro.ru>, Andrew Dunstan <andrew@dunslane.net>
Date: 2022-06-17T17:30:55Z
Lists: pgsql-hackers 
Hi,

On 2022-06-17 10:33:08 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.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

Commits

  1. JSON_TABLE: Add support for NESTED paths and columns

  2. Add basic JSON_TABLE() functionality

  3. Add SQL/JSON query functions

  4. Add soft error handling to some expression nodes

  5. Adjust populate_record_field() to handle errors softly

  6. Refactor code used by jsonpath executor to fetch variables

  7. Add more SQL/JSON constructor functions

  8. SQL/JSON: support the IS JSON predicate

  9. SQL/JSON: add standard JSON constructor functions

  10. Add static assertion ensuring sizeof(ExprEvalStep) <= 64 bytes

  11. Remove size increase in ExprEvalStep caused by hashed saops

  12. pgstat: reduce timer overhead by leaving timer running.

  13. expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.

  14. SQL/JSON query functions

  15. Speedup ScalarArrayOpExpr evaluation