Thread: Skipping PgStat_FunctionCallUsage for many expressions
Hi, while working on my faster expression evaluation stuff I noticed that a lot of expression types that call functions don't call the necessary functions to make track_functions work. ExecEvalFunc/ExecEvalOper (via ExecMakeFunctionResultNoSets) call pgstat_init_function_usage/pgstat_end_function_usage, but others like ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf, ExecEvalDistinct, ExecEvalScalarArrayOp (and indirectly ExecEvalArrayCoerceExpr) don't. Similarly InvokeFunctionExecuteHook isn't used very thoroughly. Are these worth fixing? I suspect yes. If so, do we want to backpatch? - Andres
On Fri, Nov 25, 2016 at 11:12 PM, Andres Freund <andres@anarazel.de> wrote: > while working on my faster expression evaluation stuff I noticed that a > lot of expression types that call functions don't call the necessary > functions to make track_functions work. > > ExecEvalFunc/ExecEvalOper (via ExecMakeFunctionResultNoSets) call > pgstat_init_function_usage/pgstat_end_function_usage, but others like > ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf, ExecEvalDistinct, > ExecEvalScalarArrayOp (and indirectly ExecEvalArrayCoerceExpr) don't. > > Similarly InvokeFunctionExecuteHook isn't used very thoroughly. > > Are these worth fixing? I suspect yes. If so, do we want to backpatch? If it doesn't torpedo performance, I assume we should fix and back-patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Nov 25, 2016 at 11:12 PM, Andres Freund <andres@anarazel.de> wrote:
>> while working on my faster expression evaluation stuff I noticed that a
>> lot of expression types that call functions don't call the necessary
>> functions to make track_functions work.
>>
>> ExecEvalFunc/ExecEvalOper (via ExecMakeFunctionResultNoSets) call
>> pgstat_init_function_usage/pgstat_end_function_usage, but others like
>> ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf, ExecEvalDistinct,
>> ExecEvalScalarArrayOp (and indirectly ExecEvalArrayCoerceExpr) don't.
>>
>> Similarly InvokeFunctionExecuteHook isn't used very thoroughly.
>>
>> Are these worth fixing? I suspect yes. If so, do we want to backpatch?
Those don't call functions, they call operators. Yes, I know that an
operator has a function underlying it, but the user-level expectation for
track_functions is that what it counts are things that look syntactically
like function calls. I'm not eager to add tracking overhead for cases
that there's been exactly zero field demand for.
> If it doesn't torpedo performance, I assume we should fix and back-patch.
It's certainly not going to help.
regards, tom lane
On November 26, 2016 8:06:26 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Nov 25, 2016 at 11:12 PM, Andres Freund <andres@anarazel.de> >wrote: >>> while working on my faster expression evaluation stuff I noticed >that a >>> lot of expression types that call functions don't call the necessary >>> functions to make track_functions work. >>> >>> ExecEvalFunc/ExecEvalOper (via ExecMakeFunctionResultNoSets) call >>> pgstat_init_function_usage/pgstat_end_function_usage, but others >like >>> ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf, >ExecEvalDistinct, >>> ExecEvalScalarArrayOp (and indirectly ExecEvalArrayCoerceExpr) >don't. >>> >>> Similarly InvokeFunctionExecuteHook isn't used very thoroughly. >>> >>> Are these worth fixing? I suspect yes. If so, do we want to >backpatch? > >Those don't call functions, they call operators. Yes, I know that an >operator has a function underlying it, but the user-level expectation >for >track_functions is that what it counts are things that look >syntactically >like function calls. I'm not eager to add tracking overhead for cases >that there's been exactly zero field demand for. But we do track for OpExprs? Otherwise I'd agree. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 2016-11-26 08:41:28 -0800, Andres Freund wrote: > On November 26, 2016 8:06:26 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >Robert Haas <robertmhaas@gmail.com> writes: > >> On Fri, Nov 25, 2016 at 11:12 PM, Andres Freund <andres@anarazel.de> > >wrote: > >>> while working on my faster expression evaluation stuff I noticed > >that a > >>> lot of expression types that call functions don't call the necessary > >>> functions to make track_functions work. > >>> > >>> ExecEvalFunc/ExecEvalOper (via ExecMakeFunctionResultNoSets) call > >>> pgstat_init_function_usage/pgstat_end_function_usage, but others > >like > >>> ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf, > >ExecEvalDistinct, > >>> ExecEvalScalarArrayOp (and indirectly ExecEvalArrayCoerceExpr) > >don't. > >>> > >>> Similarly InvokeFunctionExecuteHook isn't used very thoroughly. > >>> > >>> Are these worth fixing? I suspect yes. If so, do we want to > >backpatch? > > > >Those don't call functions, they call operators. Yes, I know that an > >operator has a function underlying it, but the user-level expectation > >for > >track_functions is that what it counts are things that look > >syntactically > >like function calls. I'm not eager to add tracking overhead for cases > >that there's been exactly zero field demand for. > > But we do track for OpExprs? Otherwise I'd agree. Bump? - Andres
Andres Freund <andres@anarazel.de> writes:
> On 2016-11-26 08:41:28 -0800, Andres Freund wrote:
>> On November 26, 2016 8:06:26 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Those don't call functions, they call operators. Yes, I know that an
>>> operator has a function underlying it, but the user-level expectation
>>> for track_functions is that what it counts are things that look
>>> syntactically like function calls. I'm not eager to add tracking
>>> overhead for cases that there's been exactly zero field demand for.
>> But we do track for OpExprs? Otherwise I'd agree.
> Bump?
If you're going to insist on foolish consistency, I'd rather take out
tracking in OpExpr than add it in dozens of other places.
regards, tom lane
On 2017-02-14 17:58:23 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2016-11-26 08:41:28 -0800, Andres Freund wrote: > >> On November 26, 2016 8:06:26 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> Those don't call functions, they call operators. Yes, I know that an > >>> operator has a function underlying it, but the user-level expectation > >>> for track_functions is that what it counts are things that look > >>> syntactically like function calls. I'm not eager to add tracking > >>> overhead for cases that there's been exactly zero field demand for. > > >> But we do track for OpExprs? Otherwise I'd agree. > > > Bump? > > If you're going to insist on foolish consistency, I'd rather take out > tracking in OpExpr than add it in dozens of other places. I'm ok with being inconsistent, but I'd like to make that a conscious choice rather it being the consequence of an oversight - and that's what it looks like to me. We're doing it for OpExpr, but not for a bunch of other function / operator invocations within execQual.c (namely ExecEvalDistinct, ExecEvalScalarArrayOp, ExecEvalRowCompare, ExecEvalMinMax, ExecEvalNullIf), neither do we do it for *function* invocations directly in the executor (prominently node[Window]Agg.c), but we do it for trigger invocations. That's, uh, a bit weird and hard to explain. - Andres