From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Variable-length FunctionCallInfoData |
Date: | 2019-01-25 20:51:02 |
Message-ID: | 20190125205102.ey5muymsazf4yon2@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2018-12-15 11:44:30 -0800, Andres Freund wrote:
> On 2018-12-15 10:45:21 -0500, Tom Lane wrote:
> > I also wonder if we should rename the type FunctionCallInfoData,
> > perhaps to FunctionCallInfo_Data, so as to intentionally break
> > code that hasn't been converted. On the other hand, that might
> > introduce too much useless code churn --- not sure how many live
> > references to that struct type will remain in place.
>
> Probably doable, there ought not to be many FunctionCallInfoData
> references afterwards.
I did not like FunctionCallInfo_Data, the _ grated
me. FunctionCallInfoBaseData isn't great, but seems better?
> > With or without that, I'm pretty sure you wanted the pad member to be
> > char fcinfo_data[SizeForFunctionCallInfoData(nargs)]; \
> > not
> > char *fcinfo_data[SizeForFunctionCallInfoData(nargs)]; \
Indeed. And that hid a bug or two, where not enough space for
arguments was allocated. I changed a few on-stack infos to be of
FUNC_MAX_ARGS length, because they're not known at compile time. Seems
better than unnecesarily introducing a dynamic allocation, and they're
not that hot locations.
> > One more naming thought: would "LOCAL_FCINFO(...)" be a better
> > name for that macro? I don't think FOR_ARGS is adding much in
> > any case.
>
> Hm, that works.
Done.
> > Why does struct agg_strict_input_check now have *both*
> > NullableDatum and "bool *nulls"? If that's not a typo,
> > it needs to be documented what the fields are for.
>
> I'll check whether it can be simplified.
It can't trivially: For tuplesort cases the null check points into
TupleTableSlot's isnull array, but for other aggs it points into
FunctionCallInfoData->args. If we change TupleTableSlots to use
NullableDatum as well - probably a good idea for efficiency reasons -
we could change that, but that's a separate reasonably large sided
patch. Added a comment to that end.
Updated patch attached. Besides the above changes, there's a fair bit
of comment changes, and I've implemented the necessary JIT changes.
Greetings,
Andres Freund
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Change-function-call-information-to-be-variable-l.patch | text/x-diff | 155.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2019-01-25 20:55:17 | Re: ATTACH/DETACH PARTITION CONCURRENTLY |
Previous Message | Merlin Moncure | 2019-01-25 20:21:55 | crosstab/repivot...any interest? |