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-17T20:06:05Z
Lists: pgsql-hackers

Attachments

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

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

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