From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Chapman Flack <chap(at)anastigmatix(dot)net> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: proposal: variadic argument support for least, greatest function |
Date: | 2019-02-23 06:22:15 |
Message-ID: | CAFj8pRDV0HG2YKJ2Y3aE4ZbAAtZOnFCccK1Ny9u0z87m6oLb9w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
>
Attachment | Content-Type | Size |
---|---|---|
minmax_variadic-20190223.patch | text/x-patch | 10.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Surafel Temesgen | 2019-02-23 07:53:36 | Re: FETCH FIRST clause PERCENT option |
Previous Message | Chapman Flack | 2019-02-23 03:49:17 | Re: proposal: variadic argument support for least, greatest function |