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

Hi,

On 2022-06-29 11:40:45 +1200, David Rowley wrote:
> On Sat, 18 Jun 2022 at 08:06, Andres Freund  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