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