Re: proposal: variadic argument support for least, greatest function

so 23. 2. 2019 v 4:50 odesílatel Chapman Flack <chap(at)anastigmatix(dot)net>
napsal:

> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: not tested
> Documentation: tested, passed
>
> The latest patch provides the same functionality without growing the size
> of struct ExprEvalStep, and without using the presence/absence of
> args/variadic_args to distinguish the cases. It now uses the args field
> consistently, and distinguishes the cases with new op constants,
> IS_GREATEST_VARIADIC and IS_LEAST_VARIADIC, assigned at parse time. I
> concede Tom's points about the comparative wartiness of the former patch.
>
> I'll change to WoA, though, for a few loose ends:
>
> In transformMinMaxExpr:
> The assignment of funcname doesn't look right.
> Two new errors are elogs. If they can be caused by user input (I'm sure
> the second one can), should they not be ereports?
> In fact, I think the second one should copy the equivalent one from
> parse_func.c:
>
> > ereport(ERROR,
> > (errcode(ERRCODE_DATATYPE_MISMATCH),
> > errmsg("VARIADIC argument must be an array"),
> > parser_errposition(pstate,
> > exprLocation((Node *) llast(fargs)))));
>
> ... both for consistency of the message, and so (I assume) it can use the
> existing translations for that message string.
>

good idea, done

> I am not sure if there is a way for user input to trigger the first one.
> Perhaps it can stay an elog if not. In any case, s/to
> determinate/determine/.
>

It is +/- internal error and usually should not be touched - so there I
prefer elog. I fix message

>
> In EvalExecMinMax:
>
> + if (cmpresult > 0 &&
> + (operator == IS_LEAST || operator ==
> IS_LEAST_VARIADIC))
> + *op->resvalue = value;
> + else if (cmpresult < 0 &&
> + (operator == IS_GREATEST ||
> operator == IS_GREATEST_VARIADIC))
>
> would it make sense to just compute a boolean isleast before entering the
> loop, to get simply (cmpresult > 0 && isleast) or (cmpresult < 0 &&
> !isleast) inside the loop? I'm unsure whether to assume the compiler will
> see that opportunity.
>

I am have not strong opinion about it. Personally I dislike a two variables
- but any discussion is partially about premature optimization. What about
using macros?

> Regards,
> -Chap
>
> The new status of this patch is: Waiting on Author
>