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: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Nikita Glukhov <n.gluhov@postgrespro.ru>, Andrew Dunstan <andrew@dunslane.net>
Date: 2022-07-06T00:32:47Z
Lists: pgsql-hackers 
Hi,

On 2022-06-29 11:40:45 +1200, David Rowley wrote:
> On Sat, 18 Jun 2022 at 08:06, Andres Freund <andres@anarazel.de> wrote:
> > 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.
> 
> I've now looked at the 0003 patch.  I like the idea you have about
> moving some of the additional fields into ScalarArrayOpExprHashTable.
> I think the patch can even go a little further and move the hash_finfo
> into there too. This means we don't need to dereference the "op" in
> saop_element_hash().

Makes sense.


> To make this work, I did need to tag the ScalarArrayOpExpr into the
> ExprEvalStep.  That's required now since some of the initialization of
> the hash function fields is delayed until
> ExecEvalHashedScalarArrayOp().  We need to know the
> ScalarArrayOpExpr's hashfuncid and inputcollid.

Makes sense.


> Another small thing which I considered doing was to put the
> hash_fcinfo_data field as the final field in
> ScalarArrayOpExprHashTable so that we could allocate the memory for
> the hash_fcinfo_data in the same allocation as the
> ScalarArrayOpExprHashTable.  This would reduce the pointer
> dereferencing done in saop_element_hash() a bit further.  I just
> didn't notice anywhere else where we do that for FunctionCallInfo, so
> I resisted doing this.

I think that'd make sense - it does add a bit of size calculation magic, but
it shouldn't be a problem. I'm fairly sure we do this in other parts of the
code.


> (There was also a small bug in your patch where you mistakenly cast to
> an OpExpr instead of ScalarArrayOpExpr when you were fetching the
> inputcollid)

Ooops.


Are you good pushing this? I'm fine with you doing so wether you adapt it
further or not.


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