Re: proposal: variadic argument support for least, greatest function
On 3/11/19 6:07 PM, Tom Lane wrote: > Andrew Dunstan writes: >> I'm going to mark this as rejected. Here's a possible doc patch > Maybe s/strictly/ordinary/, or some other word? "strictly" > doesn't convey much to me. Otherwise seems fine. OK, done. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: proposal: variadic argument support for least, greatest function
On Mon, Mar 11, 2019 at 3:07 PM Tom Lane wrote: > > Andrew Dunstan writes: > > I'm going to mark this as rejected. Here's a possible doc patch > > Maybe s/strictly/ordinary/, or some other word? "strictly" > doesn't convey much to me. Otherwise seems fine. > How about: While the COALESCE, GREATEST, and LEAST functions below accept a variable number of arguments they do not support the passing of a VARIADIC array. ? David J.
Re: proposal: variadic argument support for least, greatest function
Andrew Dunstan writes: > I'm going to mark this as rejected. Here's a possible doc patch Maybe s/strictly/ordinary/, or some other word? "strictly" doesn't convey much to me. Otherwise seems fine. regards, tom lane
Re: proposal: variadic argument support for least, greatest function
po 11. 3. 2019 v 18:04 odesílatel David Steele napsal: > On 3/11/19 6:43 PM, Andrew Dunstan wrote: > > > > On 3/6/19 10:24 AM, Chapman Flack wrote: > >> On 3/6/19 10:12 AM, Andrew Dunstan wrote: > >> > >>> Having reviewed the thread, I'm with Andres and Tom. Maybe though we > >>> should have a note somewhere to the effect that you can't use VARIADIC > >>> with these. > >> Perhaps such a note belongs hoisted into the functions-conditional > >> section of the manual, making a general observation that these things > >> are conditional *expressions* that may resemble functions, but in > >> particular, COALESCE, GREATEST, and LEAST cannot be called with > >> keyword VARIADIC and an array argument, as they could if they were > >> ordinary functions. > >> > > > > > > I'm going to mark this as rejected. Here's a possible doc patch > > +1 > > ok Pavel -- > -David > da...@pgmasters.net >
Re: proposal: variadic argument support for least, greatest function
On 3/11/19 6:43 PM, Andrew Dunstan wrote: On 3/6/19 10:24 AM, Chapman Flack wrote: On 3/6/19 10:12 AM, Andrew Dunstan wrote: Having reviewed the thread, I'm with Andres and Tom. Maybe though we should have a note somewhere to the effect that you can't use VARIADIC with these. Perhaps such a note belongs hoisted into the functions-conditional section of the manual, making a general observation that these things are conditional *expressions* that may resemble functions, but in particular, COALESCE, GREATEST, and LEAST cannot be called with keyword VARIADIC and an array argument, as they could if they were ordinary functions. I'm going to mark this as rejected. Here's a possible doc patch +1 -- -David da...@pgmasters.net
Re: proposal: variadic argument support for least, greatest function
On 3/6/19 10:24 AM, Chapman Flack wrote:
> On 3/6/19 10:12 AM, Andrew Dunstan wrote:
>
>> Having reviewed the thread, I'm with Andres and Tom. Maybe though we
>> should have a note somewhere to the effect that you can't use VARIADIC
>> with these.
> Perhaps such a note belongs hoisted into the functions-conditional
> section of the manual, making a general observation that these things
> are conditional *expressions* that may resemble functions, but in
> particular, COALESCE, GREATEST, and LEAST cannot be called with
> keyword VARIADIC and an array argument, as they could if they were
> ordinary functions.
>
I'm going to mark this as rejected. Here's a possible doc patch
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 03859a78ea..7fbcdfeae5 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12481,6 +12481,15 @@ SELECT setval('foo', 42, false);Next nextval
+
+
+ Although COALESCE, GREATEST, and
+ LEAST are syntactically similar to functions, they are
+ not strictly functions, and thus cannot be used with explicit
+ VARIADIC array arguments.
+
+
+
CASE
Re: proposal: variadic argument support for least, greatest function
st 6. 3. 2019 v 16:24 odesílatel Chapman Flack napsal: > On 3/6/19 10:12 AM, Andrew Dunstan wrote: > > > Having reviewed the thread, I'm with Andres and Tom. Maybe though we > > should have a note somewhere to the effect that you can't use VARIADIC > > with these. > > Perhaps such a note belongs hoisted into the functions-conditional > section of the manual, making a general observation that these things > are conditional *expressions* that may resemble functions, but in > particular, COALESCE, GREATEST, and LEAST cannot be called with > keyword VARIADIC and an array argument, as they could if they were > ordinary functions. > +1 Pavel > Regards, > -Chap >
Re: proposal: variadic argument support for least, greatest function
On 3/6/19 10:12 AM, Andrew Dunstan wrote: > Having reviewed the thread, I'm with Andres and Tom. Maybe though we > should have a note somewhere to the effect that you can't use VARIADIC > with these. Perhaps such a note belongs hoisted into the functions-conditional section of the manual, making a general observation that these things are conditional *expressions* that may resemble functions, but in particular, COALESCE, GREATEST, and LEAST cannot be called with keyword VARIADIC and an array argument, as they could if they were ordinary functions. Regards, -Chap
Re: proposal: variadic argument support for least, greatest function
On 3/4/19 9:39 AM, David Steele wrote: > On 3/1/19 3:59 AM, Chapman Flack wrote: >> 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 >> >> For completeness, I'll mark this reviewed again. It passes >> installcheck-world, the latest patch addresses the suggestions from >> me, and is improved on the code-structure matters that Tom pointed >> out. I don't know if it will meet Tom's threshold for desirability >> overall, but that sounds like a committer call at this point, so I'll >> change it to RfC. > > Both committers who have looked at this patch (Tom, and Andres in his > patch roundup [1]) recommend that it be rejected. > > If no committer steps up in the next week I think we should mark it as > rejected. > > Having reviewed the thread, I'm with Andres and Tom. Maybe though we should have a note somewhere to the effect that you can't use VARIADIC with these. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: proposal: variadic argument support for least, greatest function
On 3/1/19 3:59 AM, Chapman Flack wrote: 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 For completeness, I'll mark this reviewed again. It passes installcheck-world, the latest patch addresses the suggestions from me, and is improved on the code-structure matters that Tom pointed out. I don't know if it will meet Tom's threshold for desirability overall, but that sounds like a committer call at this point, so I'll change it to RfC. Both committers who have looked at this patch (Tom, and Andres in his patch roundup [1]) recommend that it be rejected. If no committer steps up in the next week I think we should mark it as rejected. Regards, -- -David da...@pgmasters.net [1] https://www.postgresql.org/message-id/20190214203752.t4hl574k6jlu4t25%40alap3.anarazel.de
Re: proposal: variadic argument support for least, greatest function
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 For completeness, I'll mark this reviewed again. It passes installcheck-world, the latest patch addresses the suggestions from me, and is improved on the code-structure matters that Tom pointed out. I don't know if it will meet Tom's threshold for desirability overall, but that sounds like a committer call at this point, so I'll change it to RfC. -Chap The new status of this patch is: Ready for Committer
Re: proposal: variadic argument support for least, greatest function
so 23. 2. 2019 v 20:23 odesílatel Chapman Flack
napsal:
> On 02/23/19 13:35, Pavel Stehule wrote:
> > please, see, attached patch
>
> It is getting close, for my purposes. There is still this:
>
> >> Can the sentence added to the doc be changed to "These functions support
> >> passing parameters as an array after VARIADIC
> >> keyword."? That is, s/supports/support/ and s/a/an/. I've done that
> >> after a couple of recent patches, but it seems to be on springs.
>
fixed
>
>
> > I know so is important to understand to things, but nobody can understand
> > to all. And then it is nice, so the things just works
> >>
> >> The approach with more parsimony indoors would be to just create a few
> >> new ordinary functions, and add to the doc explaining why they are
> >> different, and that would be a patch only needing to touch a couple
> files.
> >>
> >> I have a feeling that the final decision on whether the indoor or
> outdoor
> >> parsimony matters more will be made by Tom, or another committer; I find
> >> myself seeing both sides, and not feeling insider-y enough myself to
> >> pick one.
> >
> > sure, every time it is commiter's decision.
>
>
> A part of me waits to see if another voice chimes in on the high-level
> want/don't-want question ... I think enough of the patch is ready for
> that question to be ripe, and if the answer is going to be "don't want"
> it would be ideal to know that before additional iterations of work on it.
>
sure
Regards
Pavel
> -Chap
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..5e10124de9 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions support passing parameters as an array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..b1f3bc86de 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2811,6 +2811,10 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
FunctionCallInfo fcinfo = op->d.minmax.fcinfo_data;
MinMaxOp operator = op->d.minmax.op;
int off;
+ ArrayIterator array_iterator = NULL;
+ int nelems;
+ bool is_greatest;
+ bool is_least;
/* set at initialization */
Assert(fcinfo->args[0].isnull == false);
@@ -2819,16 +2823,52 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
- for (off = 0; off < op->d.minmax.nelems; off++)
+ is_least = operator == IS_LEAST || operator == IS_LEAST_VARIADIC;
+ is_greatest = operator == IS_GREATEST || operator == IS_GREATEST_VARIADIC;
+
+ if (IsVariadicMinMax(operator))
+ {
+ ArrayType *arr;
+
+ /* result is null, when variadic argument is NULL */
+ if (nulls[0])
+ return;
+
+ /* prepare iterarator */
+ arr = DatumGetArrayTypeP(values[0]);
+ array_iterator = array_create_iterator(arr, 0, NULL);
+
+ nelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ }
+ else
+ nelems = op->d.minmax.nelems;
+
+ for (off = 0; off < nelems; off++)
{
+ Datum value;
+ bool isnull;
+
+ if (array_iterator)
+ {
+ bool has_data PG_USED_FOR_ASSERTS_ONLY;
+
+ has_data = array_iterate(array_iterator, , );
+ Assert(has_data);
+ }
+ else
+ {
+ value = values[off];
+ isnull = nulls[off];
+ }
+
/* ignore NULL inputs */
- if (nulls[off])
+ if (isnull)
continue;
if (*op->resnull)
{
/* first nonnull input, adopt value */
- *op->resvalue = values[off];
+ *op->resvalue = value;
*op->resnull = false;
}
else
@@ -2837,19 +2877,23 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* apply comparison function */
fcinfo->args[0].value = *op->resvalue;
- fcinfo->args[1].value = values[off];
+ fcinfo->args[1].value = value;
fcinfo->isnull = false;
cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
if (fcinfo->isnull) /* probably should not happen */
continue;
- if (cmpresult > 0 && operator == IS_LEAST)
-*op->resvalue = values[off];
- else if (cmpresult < 0 && operator == IS_GREATEST)
-*op->resvalue = values[off];
+ if (cmpresult > 0 && is_least)
+*op->resvalue = value;
+ else if (cmpresult < 0 && is_greatest)
+*op->resvalue = value;
}
}
+
+ /* release iterator */
+ if (array_iterator)
+ array_free_iterator(array_iterator);
}
/*
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 8ed30c011a..efabaa0046 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -465,7 +465,8 @@ exprTypmod(const Node *expr)
int32 typmod;
ListCell *arg;
-if (exprType((Node *)
Re: proposal: variadic argument support for least, greatest function
On 02/23/19 13:35, Pavel Stehule wrote: > please, see, attached patch It is getting close, for my purposes. There is still this: >> Can the sentence added to the doc be changed to "These functions support >> passing parameters as an array after VARIADIC >> keyword."? That is, s/supports/support/ and s/a/an/. I've done that >> after a couple of recent patches, but it seems to be on springs. > I know so is important to understand to things, but nobody can understand > to all. And then it is nice, so the things just works >> >> The approach with more parsimony indoors would be to just create a few >> new ordinary functions, and add to the doc explaining why they are >> different, and that would be a patch only needing to touch a couple files. >> >> I have a feeling that the final decision on whether the indoor or outdoor >> parsimony matters more will be made by Tom, or another committer; I find >> myself seeing both sides, and not feeling insider-y enough myself to >> pick one. > > sure, every time it is commiter's decision. A part of me waits to see if another voice chimes in on the high-level want/don't-want question ... I think enough of the patch is ready for that question to be ripe, and if the answer is going to be "don't want" it would be ideal to know that before additional iterations of work on it. -Chap
Re: proposal: variadic argument support for least, greatest function
so 23. 2. 2019 v 18:28 odesílatel Chapman Flack napsal: > On 02/23/19 01:22, Pavel Stehule wrote: > > so 23. 2. 2019 v 4:50 odesílatel Chapman Flack > > napsal: > > >> In transformMinMaxExpr: > >> The assignment of funcname doesn't look right. > > ... it still doesn't. > fixed > >> Two new errors are elogs. ... > >> 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 > > ... still needs s/to //. > fixed > Can the sentence added to the doc be changed to "These functions support > passing parameters as an array after VARIADIC keyword."? > That is, s/supports/support/ and s/a/an/. I've done that after a couple of > recent patches, but it seems to be on springs. > > > What about using macros? > > Meh ... the macros look nicer, but still rely just as much on the compiler > to hoist the tests out of the loop. I suppose it probably can. > reverted, I use a variables > > I wouldn't have thought it necessary to change the switch statements in > FigureColnameInternal or get_rule_expr ... cases with multiple labels are > seen often enough, and it's probably hard to do better. > > > Taking a step back ... > > > All of this still begs Tom's question about whether array_greatest/ > array_least would be preferable. > > I understand Pavel's point: > > >> I am not against these functions, but these functions doesn't solve a > >> confusing of some users, so LEAST, GREATEST looks like variadic > >> functions, but it doesn't allow VARIADIC parameter. > > but, to advocate the other side for a moment, perhaps that could be viewed > as more of a documentation problem. > > At bottom, the confusion potential that concerns Pavel exists because > these things look like variadic functions, and the manual calls them > "the GREATEST and LEAST functions", but they won't accept a VARIADIC array > parameter as a genuine variadic function would. > > But that's hardly the only way these differ from normal functions. > You can't find them in pg_proc or call them through fmgr. In fact, they > share these non-function properties with all of their siblings in the > functions-conditional section of the manual. (Arguably, if GREATEST and > LEAST end up taking a VARIADIC array parameter, COALESCE will be wanting > one next. And indeed, there doesn't seem to be an existing > array_firstnonnull function for that job, either.) And these "functions" > have special, type-unifying argument rules (already documented in > typeconv-union-case), late argument evaluation in the case of COALESCE, > and so on. > > In other words, any effort to make these animals act more like functions > will be necessarily incomplete, and differences will remain. > > Maybe the confusion-potential could be fixed by having one more sentence > at the top of the whole functions-conditional section, saying "Some of > these resemble functions, but are better viewed as function-like > expressions, with special rules for their argument lists." Then the section > for GREATEST/LEAST could have a "see also" for array_greatest/array_least, > the COALESCE section a "see also" for array_firstnonnull, and those simple > functions could be written. > > The special rules for these constructs don't really buy anything for the > array-argument flavors: you can't pass in an array with heterogeneous > types or late-evaluated values anyway, so ordinary functions would suffice. > > From the outside, it would look tidy and parsimonious to just have > GREATEST(VARIADIC...)/LEAST(VARIADIC...)/COALESCE(VARIADIC...) and have > just one set of "function" names to remember, rather than separate > array_* variants. But the cost of that tidiness from the outside is an > implementation that has to frob half a dozen source files on the inside. > This is goal of this small patch - do life little bit more easier and little bit more consistent. It is very small patch without risks or slowdowns. So I expect the cost of this patch is small. Just it is small step forward to users. I wrote "greatest", "least" support before I wrote variadic functions support. If I wrote it in different order, probably, greatest, least functions was pg_proc based. On second hand, the limitation of pg_proc functions was motivation for variadic function support. With my experience, I am not sure if better documentation does things better. For some kind of users some smart magic is important. They don't want to see implementation details. In my car, i lost hope to understand completely to engine. I am not sure if users should to know so we have three kind of functions - a) pg_proc based functions, b) parser based functions (and looks like functions), c) parser based functions (and looks like constant). I know so is important to understand to things, but nobody can understand to all. And then it is
Re: proposal: variadic argument support for least, greatest function
On 02/23/19 01:22, Pavel Stehule wrote: > so 23. 2. 2019 v 4:50 odesílatel Chapman Flack > napsal: >> In transformMinMaxExpr: >> The assignment of funcname doesn't look right. ... it still doesn't. >> Two new errors are elogs. ... >> 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 ... still needs s/to //. Can the sentence added to the doc be changed to "These functions support passing parameters as an array after VARIADIC keyword."? That is, s/supports/support/ and s/a/an/. I've done that after a couple of recent patches, but it seems to be on springs. > What about using macros? Meh ... the macros look nicer, but still rely just as much on the compiler to hoist the tests out of the loop. I suppose it probably can. I wouldn't have thought it necessary to change the switch statements in FigureColnameInternal or get_rule_expr ... cases with multiple labels are seen often enough, and it's probably hard to do better. Taking a step back ... All of this still begs Tom's question about whether array_greatest/ array_least would be preferable. I understand Pavel's point: >> I am not against these functions, but these functions doesn't solve a >> confusing of some users, so LEAST, GREATEST looks like variadic >> functions, but it doesn't allow VARIADIC parameter. but, to advocate the other side for a moment, perhaps that could be viewed as more of a documentation problem. At bottom, the confusion potential that concerns Pavel exists because these things look like variadic functions, and the manual calls them "the GREATEST and LEAST functions", but they won't accept a VARIADIC array parameter as a genuine variadic function would. But that's hardly the only way these differ from normal functions. You can't find them in pg_proc or call them through fmgr. In fact, they share these non-function properties with all of their siblings in the functions-conditional section of the manual. (Arguably, if GREATEST and LEAST end up taking a VARIADIC array parameter, COALESCE will be wanting one next. And indeed, there doesn't seem to be an existing array_firstnonnull function for that job, either.) And these "functions" have special, type-unifying argument rules (already documented in typeconv-union-case), late argument evaluation in the case of COALESCE, and so on. In other words, any effort to make these animals act more like functions will be necessarily incomplete, and differences will remain. Maybe the confusion-potential could be fixed by having one more sentence at the top of the whole functions-conditional section, saying "Some of these resemble functions, but are better viewed as function-like expressions, with special rules for their argument lists." Then the section for GREATEST/LEAST could have a "see also" for array_greatest/array_least, the COALESCE section a "see also" for array_firstnonnull, and those simple functions could be written. The special rules for these constructs don't really buy anything for the array-argument flavors: you can't pass in an array with heterogeneous types or late-evaluated values anyway, so ordinary functions would suffice. >From the outside, it would look tidy and parsimonious to just have GREATEST(VARIADIC...)/LEAST(VARIADIC...)/COALESCE(VARIADIC...) and have just one set of "function" names to remember, rather than separate array_* variants. But the cost of that tidiness from the outside is an implementation that has to frob half a dozen source files on the inside. The approach with more parsimony indoors would be to just create a few new ordinary functions, and add to the doc explaining why they are different, and that would be a patch only needing to touch a couple files. I have a feeling that the final decision on whether the indoor or outdoor parsimony matters more will be made by Tom, or another committer; I find myself seeing both sides, and not feeling insider-y enough myself to pick one. -Chap
Re: proposal: variadic argument support for least, greatest function
so 23. 2. 2019 v 4:50 odesílatel Chapman Flack
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
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions supports passing parameters as a array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..8ac4628ec1 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2811,6 +2811,8 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
FunctionCallInfo fcinfo = op->d.minmax.fcinfo_data;
MinMaxOp operator = op->d.minmax.op;
int off;
+ ArrayIterator array_iterator = NULL;
+ int nelems;
/* set at initialization */
Assert(fcinfo->args[0].isnull == false);
@@ -2819,16 +2821,49 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
- for (off = 0; off < op->d.minmax.nelems; off++)
+ if (IsMinMaxVariadic(operator))
{
+ ArrayType *arr;
+
+ /* result is null, when variadic argument is NULL */
+ if (nulls[0])
+ return;
+
+ /* prepare iterarator */
+ arr = DatumGetArrayTypeP(values[0]);
+ array_iterator = array_create_iterator(arr, 0, NULL);
+
+ nelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ }
+ else
+ nelems = op->d.minmax.nelems;
+
+ for (off = 0; off < nelems; off++)
+ {
+ Datum value;
+ bool isnull;
+
+ if (array_iterator)
+ {
+ bool has_data PG_USED_FOR_ASSERTS_ONLY;
+
+ has_data = array_iterate(array_iterator, , );
+ Assert(has_data);
+ }
+ else
+ {
+ value = values[off];
+ isnull = nulls[off];
+ }
+
/* ignore NULL inputs */
- if (nulls[off])
+ if (isnull)
continue;
if (*op->resnull)
{
/* first nonnull input, adopt value */
- *op->resvalue = values[off];
+ *op->resvalue = value;
*op->resnull = false;
}
else
@@ -2837,19 +2872,23 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* apply comparison function */
fcinfo->args[0].value = *op->resvalue;
- fcinfo->args[1].value = values[off];
+ fcinfo->args[1].value
Re: proposal: variadic argument support for least, greatest function
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.
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/.
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.
Regards,
-Chap
The new status of this patch is: Waiting on Author
Re: proposal: variadic argument support for least, greatest function
On 02/22/19 19:31, Chapman Flack wrote:
> minmax_variadic-20190222-3.patch same as -2 but for doc grammar fix
> (same fix made in minmax_variadic-20190221b.patch).
and naturally I didn't attach it.
-Chap
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions support passing parameters as an array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..2b98f7e820 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2811,6 +2811,8 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
FunctionCallInfo fcinfo = op->d.minmax.fcinfo_data;
MinMaxOp operator = op->d.minmax.op;
int off;
+ ArrayIterator array_iterator = NULL;
+ int nelems;
/* set at initialization */
Assert(fcinfo->args[0].isnull == false);
@@ -2819,16 +2821,49 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
- for (off = 0; off < op->d.minmax.nelems; off++)
+ if (IsVariadicMinMax(operator))
{
+ ArrayType *arr;
+
+ /* result is null, when variadic argument is NULL */
+ if (nulls[0])
+ return;
+
+ /* prepare iterarator */
+ arr = DatumGetArrayTypeP(values[0]);
+ array_iterator = array_create_iterator(arr, 0, NULL);
+
+ nelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ }
+ else
+ nelems = op->d.minmax.nelems;
+
+ for (off = 0; off < nelems; off++)
+ {
+ Datum value;
+ bool isnull;
+
+ if (array_iterator)
+ {
+ bool has_data PG_USED_FOR_ASSERTS_ONLY;
+
+ has_data = array_iterate(array_iterator, , );
+ Assert(has_data);
+ }
+ else
+ {
+ value = values[off];
+ isnull = nulls[off];
+ }
+
/* ignore NULL inputs */
- if (nulls[off])
+ if (isnull)
continue;
if (*op->resnull)
{
/* first nonnull input, adopt value */
- *op->resvalue = values[off];
+ *op->resvalue = value;
*op->resnull = false;
}
else
@@ -2837,19 +2872,25 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* apply comparison function */
fcinfo->args[0].value = *op->resvalue;
- fcinfo->args[1].value = values[off];
+ fcinfo->args[1].value = value;
fcinfo->isnull = false;
cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
if (fcinfo->isnull) /* probably should not happen */
continue;
- if (cmpresult > 0 && operator == IS_LEAST)
-*op->resvalue = values[off];
- else if (cmpresult < 0 && operator == IS_GREATEST)
-*op->resvalue = values[off];
+ if (cmpresult > 0 &&
+(operator == IS_LEAST || operator == IS_LEAST_VARIADIC))
+*op->resvalue = value;
+ else if (cmpresult < 0 &&
+ (operator == IS_GREATEST || operator == IS_GREATEST_VARIADIC))
+*op->resvalue = value;
}
}
+
+ /* release iterator */
+ if (array_iterator)
+ array_free_iterator(array_iterator);
}
/*
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 8ed30c011a..efabaa0046 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -465,7 +465,8 @@ exprTypmod(const Node *expr)
int32 typmod;
ListCell *arg;
-if (exprType((Node *) linitial(mexpr->args)) != minmaxtype)
+if (!IsVariadicMinMax(mexpr->op) &&
+ exprType((Node *) linitial(mexpr->args)) != minmaxtype)
return -1;
typmod = exprTypmod((Node *) linitial(mexpr->args));
if (typmod < 0)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a68f78e0e0..65f44a8df6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -13803,6 +13803,22 @@ func_expr_common_subexpr:
v->location = @1;
$$ = (Node *)v;
}
+ | GREATEST '(' VARIADIC a_expr ')'
+{
+ MinMaxExpr *v = makeNode(MinMaxExpr);
+ v->args = list_make1($4);
+ v->op = IS_GREATEST_VARIADIC;
+ v->location = @1;
+ $$ = (Node *)v;
+}
+ | LEAST '(' VARIADIC a_expr ')'
+{
+ MinMaxExpr *v = makeNode(MinMaxExpr);
+ v->args = list_make1($4);
+ v->op = IS_LEAST_VARIADIC;
+ v->location = @1;
+ $$ = (Node *)v;
+}
| XMLCONCAT '(' expr_list ')'
{
$$ = makeXmlExpr(IS_XMLCONCAT, NULL, NIL, $3, @1);
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index e559353529..d739582ba0 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2276,22 +2276,43 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
newargs = lappend(newargs, newe);
}
- newm->minmaxtype =
Re: proposal: variadic argument support for least, greatest function
On 02/22/19 14:57, Pavel Stehule wrote: > I am sending second variant (little bit longer, but the main code is not > repeated) minmax_variadic-20190222-3.patch same as -2 but for doc grammar fix (same fix made in minmax_variadic-20190221b.patch). Regards, -Chap
Re: proposal: variadic argument support for least, greatest function
pá 22. 2. 2019 v 13:42 odesílatel Pavel Stehule
napsal:
> Hi
>
> čt 21. 2. 2019 v 22:05 odesílatel Tom Lane napsal:
>
>> Pavel Stehule writes:
>> > čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack
>> > napsal:
>> >> I am not sure I have an answer to the objections being raised on
>> grounds
>> >> of taste. To me, it's persuasive that GREATEST and LEAST are described
>> in
>> >> the docco as functions, they are used much like variadic functions, and
>> >> this patch allows them to be used in the ways you would expect variadic
>> >> functions to be usable.
>>
>> > I wrote doc (just one sentence) and minimal test. Both can be enhanced.
>>
>> I remain of the opinion that this patch is a mess.
>>
>> I don't share Pavel's opinion that this is a clean user API, though
>> I'll grant that others might have different opinions on that.
>> I could hold my nose and overlook that if it led to a clean internal
>> implementation. But it doesn't: this patch just bolts a huge,
>> undocumented wart onto the side of MinMaxExpr. (The arguments are
>> in the args field, except when they aren't? And everyplace that
>> deals with MinMaxExpr needs to know that, as well as the fact that
>> the semantics are totally different? Ick.)
>>
>
> fixed
>
>
>> An example of the lack of care here is that the change in struct
>> ExprEvalStep breaks that struct's size constraint:
>>
>> * Inline data for the operation. Inline data is faster to access,
>> but
>> * also bloats the size of all instructions. The union should be
>> kept to
>> * no more than 40 bytes on 64-bit systems (so that the entire struct
>> is
>> * no more than 64 bytes, a single cacheline on common systems).
>>
>>
> fixed
>
>
>> Andres is going to be quite displeased if that gets committed ;-).
>>
>
> I hope so I solved all your objections. Now, the patch is really reduced.
>
>
>
>> I still say we should reject this and invent array_greatest/array_least
>> functions instead.
>>
>
> I am not against these functions, but these functions doesn't solve a
> confusing of some users, so LEAST, GREATEST looks like variadic functions,
> but it doesn't allow VARIADIC parameter.
>
> Comments, notes?
>
I am sending second variant (little bit longer, but the main code is not
repeated)
regards
Pavel
>
>
>> regards, tom lane
>>
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions supports passing parameters as a array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..2b98f7e820 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2811,6 +2811,8 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
FunctionCallInfo fcinfo = op->d.minmax.fcinfo_data;
MinMaxOp operator = op->d.minmax.op;
int off;
+ ArrayIterator array_iterator = NULL;
+ int nelems;
/* set at initialization */
Assert(fcinfo->args[0].isnull == false);
@@ -2819,16 +2821,49 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
- for (off = 0; off < op->d.minmax.nelems; off++)
+ if (IsVariadicMinMax(operator))
{
+ ArrayType *arr;
+
+ /* result is null, when variadic argument is NULL */
+ if (nulls[0])
+ return;
+
+ /* prepare iterarator */
+ arr = DatumGetArrayTypeP(values[0]);
+ array_iterator = array_create_iterator(arr, 0, NULL);
+
+ nelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ }
+ else
+ nelems = op->d.minmax.nelems;
+
+ for (off = 0; off < nelems; off++)
+ {
+ Datum value;
+ bool isnull;
+
+ if (array_iterator)
+ {
+ bool has_data PG_USED_FOR_ASSERTS_ONLY;
+
+ has_data = array_iterate(array_iterator, , );
+ Assert(has_data);
+ }
+ else
+ {
+ value = values[off];
+ isnull = nulls[off];
+ }
+
/* ignore NULL inputs */
- if (nulls[off])
+ if (isnull)
continue;
if (*op->resnull)
{
/* first nonnull input, adopt value */
- *op->resvalue = values[off];
+ *op->resvalue = value;
*op->resnull = false;
}
else
@@ -2837,19 +2872,25 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* apply comparison function */
fcinfo->args[0].value = *op->resvalue;
- fcinfo->args[1].value = values[off];
+ fcinfo->args[1].value = value;
fcinfo->isnull = false;
cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
if (fcinfo->isnull) /* probably should not happen */
continue;
- if (cmpresult > 0 && operator == IS_LEAST)
-
Re: proposal: variadic argument support for least, greatest function
Hi
čt 21. 2. 2019 v 22:05 odesílatel Tom Lane napsal:
> Pavel Stehule writes:
> > čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack
> > napsal:
> >> I am not sure I have an answer to the objections being raised on grounds
> >> of taste. To me, it's persuasive that GREATEST and LEAST are described
> in
> >> the docco as functions, they are used much like variadic functions, and
> >> this patch allows them to be used in the ways you would expect variadic
> >> functions to be usable.
>
> > I wrote doc (just one sentence) and minimal test. Both can be enhanced.
>
> I remain of the opinion that this patch is a mess.
>
> I don't share Pavel's opinion that this is a clean user API, though
> I'll grant that others might have different opinions on that.
> I could hold my nose and overlook that if it led to a clean internal
> implementation. But it doesn't: this patch just bolts a huge,
> undocumented wart onto the side of MinMaxExpr. (The arguments are
> in the args field, except when they aren't? And everyplace that
> deals with MinMaxExpr needs to know that, as well as the fact that
> the semantics are totally different? Ick.)
>
fixed
> An example of the lack of care here is that the change in struct
> ExprEvalStep breaks that struct's size constraint:
>
> * Inline data for the operation. Inline data is faster to access, but
> * also bloats the size of all instructions. The union should be kept
> to
> * no more than 40 bytes on 64-bit systems (so that the entire struct
> is
> * no more than 64 bytes, a single cacheline on common systems).
>
>
fixed
> Andres is going to be quite displeased if that gets committed ;-).
>
I hope so I solved all your objections. Now, the patch is really reduced.
> I still say we should reject this and invent array_greatest/array_least
> functions instead.
>
I am not against these functions, but these functions doesn't solve a
confusing of some users, so LEAST, GREATEST looks like variadic functions,
but it doesn't allow VARIADIC parameter.
Comments, notes?
> regards, tom lane
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions supports passing parameters as a array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..418aca9c84 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2819,6 +2819,55 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
+ if (IsVariadicMinMax(operator))
+ {
+ ArrayIterator array_iterator;
+ ArrayType *arr;
+ Datum value;
+ bool isnull;
+
+ if (nulls[0])
+ return;
+
+ arr = DatumGetArrayTypeP(values[0]);
+
+ array_iterator = array_create_iterator(arr, 0, NULL);
+ while (array_iterate(array_iterator, , ))
+ {
+ if (isnull)
+continue;
+
+ if (*op->resnull)
+ {
+/* first nonnull input */
+*op->resvalue = value;
+*op->resnull = false;
+ }
+ else
+ {
+int cmpresult;
+
+Assert(fcinfo->nargs == 2);
+
+/* apply comparison function */
+fcinfo->args[0].value = *op->resvalue;
+fcinfo->args[1].value = value;
+
+fcinfo->isnull = false;
+cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
+if (fcinfo->isnull) /* probably should not happen */
+ continue;
+
+if (cmpresult > 0 && operator == IS_LEAST_VARIADIC)
+ *op->resvalue = value;
+else if (cmpresult < 0 && operator == IS_GREATEST_VARIADIC)
+ *op->resvalue = value;
+ }
+ }
+
+ return;
+ }
+
for (off = 0; off < op->d.minmax.nelems; off++)
{
/* ignore NULL inputs */
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 8ed30c011a..efabaa0046 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -465,7 +465,8 @@ exprTypmod(const Node *expr)
int32 typmod;
ListCell *arg;
-if (exprType((Node *) linitial(mexpr->args)) != minmaxtype)
+if (!IsVariadicMinMax(mexpr->op) &&
+ exprType((Node *) linitial(mexpr->args)) != minmaxtype)
return -1;
typmod = exprTypmod((Node *) linitial(mexpr->args));
if (typmod < 0)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a68f78e0e0..65f44a8df6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -13803,6 +13803,22 @@ func_expr_common_subexpr:
v->location = @1;
$$ = (Node *)v;
}
+ | GREATEST '(' VARIADIC a_expr ')'
+{
+
Re: proposal: variadic argument support for least, greatest function
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 On 02/21/19 16:04, Tom Lane wrote: > Pavel Stehule writes: >> I wrote doc (just one sentence) and minimal test. Both can be enhanced. * dutifully submits review saying latest patch passes installcheck-world and has tests and docs, could be RfC > I still say we should reject this and invent array_greatest/array_least > functions instead. * reflects on own pay grade, wanders off in search of other patch to review The new status of this patch is: Ready for Committer
Re: proposal: variadic argument support for least, greatest function
On 02/21/19 11:31, Pavel Stehule wrote:
> I wrote doc (just one sentence) and minimal test. Both can be enhanced.
Attaching minmax_variadic-20190221b.patch, identical but for
s/supports/support/ and s/a/an/ in the doc.
Regards,
-Chap
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions support passing parameters as an array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index db3777d15e..01d7f0e02c 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1854,13 +1854,18 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_MinMaxExpr:
{
MinMaxExpr *minmaxexpr = (MinMaxExpr *) node;
-int nelems = list_length(minmaxexpr->args);
+int nelems;
TypeCacheEntry *typentry;
FmgrInfo *finfo;
FunctionCallInfo fcinfo;
ListCell *lc;
int off;
+if (minmaxexpr->args)
+ nelems = list_length(minmaxexpr->args);
+else
+ nelems = 1;
+
/* Look up the btree comparison function for the datatype */
typentry = lookup_type_cache(minmaxexpr->minmaxtype,
TYPECACHE_CMP_PROC);
@@ -1897,16 +1902,29 @@ ExecInitExprRec(Expr *node, ExprState *state,
scratch.d.minmax.finfo = finfo;
scratch.d.minmax.fcinfo_data = fcinfo;
-/* evaluate expressions into minmax->values/nulls */
-off = 0;
-foreach(lc, minmaxexpr->args)
+if (minmaxexpr->args)
{
- Expr *e = (Expr *) lfirst(lc);
+ scratch.d.minmax.variadic_arg = false;
- ExecInitExprRec(e, state,
- [off],
- [off]);
- off++;
+ /* evaluate expressions into minmax->values/nulls */
+ off = 0;
+ foreach(lc, minmaxexpr->args)
+ {
+ Expr *e = (Expr *) lfirst(lc);
+
+ ExecInitExprRec(e, state,
+ [off],
+ [off]);
+ off++;
+ }
+}
+else
+{
+ scratch.d.minmax.variadic_arg = true;
+
+ ExecInitExprRec(minmaxexpr->variadic_arg, state,
+ [0],
+ [0]);
}
/* and push the final comparison */
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..748c950885 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2819,6 +2819,55 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
+ if (op->d.minmax.variadic_arg)
+ {
+ ArrayIterator array_iterator;
+ ArrayType *arr;
+ Datum value;
+ bool isnull;
+
+ if (nulls[0])
+ return;
+
+ arr = DatumGetArrayTypeP(values[0]);
+
+ array_iterator = array_create_iterator(arr, 0, NULL);
+ while (array_iterate(array_iterator, , ))
+ {
+ if (isnull)
+continue;
+
+ if (*op->resnull)
+ {
+/* first nonnull input */
+*op->resvalue = value;
+*op->resnull = false;
+ }
+ else
+ {
+int cmpresult;
+
+Assert(fcinfo->nargs == 2);
+
+/* apply comparison function */
+fcinfo->args[0].value = *op->resvalue;
+fcinfo->args[1].value = value;
+
+fcinfo->isnull = false;
+cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
+if (fcinfo->isnull) /* probably should not happen */
+ continue;
+
+if (cmpresult > 0 && operator == IS_LEAST)
+ *op->resvalue = value;
+else if (cmpresult < 0 && operator == IS_GREATEST)
+ *op->resvalue = value;
+ }
+ }
+
+ return;
+ }
+
for (off = 0; off < op->d.minmax.nelems; off++)
{
/* ignore NULL inputs */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index e15724bb0e..e8ad89799f 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1942,6 +1942,7 @@ _copyMinMaxExpr(const MinMaxExpr *from)
COPY_SCALAR_FIELD(inputcollid);
COPY_SCALAR_FIELD(op);
COPY_NODE_FIELD(args);
+ COPY_NODE_FIELD(variadic_arg);
COPY_LOCATION_FIELD(location);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 31499eb798..6623c7800d 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -635,6 +635,7 @@ _equalMinMaxExpr(const MinMaxExpr *a, const MinMaxExpr *b)
COMPARE_SCALAR_FIELD(inputcollid);
COMPARE_SCALAR_FIELD(op);
COMPARE_NODE_FIELD(args);
+ COMPARE_NODE_FIELD(variadic_arg);
COMPARE_LOCATION_FIELD(location);
return true;
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 8ed30c011a..52fcf8d9b0 100644
--- a/src/backend/nodes/nodeFuncs.c
+++
Re: proposal: variadic argument support for least, greatest function
David Fetter writes: > On Thu, Feb 21, 2019 at 04:04:41PM -0500, Tom Lane wrote: >> I still say we should reject this and invent array_greatest/array_least >> functions instead. > Might other array_* functions of this type be in scope for this patch? Uh ... no, I wouldn't expect that. Why would we insist on more functionality than is there now? (I'm only arguing about how we present the functionality, not what it does.) regards, tom lane
Re: proposal: variadic argument support for least, greatest function
On Thu, Feb 21, 2019 at 04:04:41PM -0500, Tom Lane wrote: > I still say we should reject this and invent array_greatest/array_least > functions instead. Might other array_* functions of this type be in scope for this patch? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: proposal: variadic argument support for least, greatest function
Pavel Stehule writes: > čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack > napsal: >> I am not sure I have an answer to the objections being raised on grounds >> of taste. To me, it's persuasive that GREATEST and LEAST are described in >> the docco as functions, they are used much like variadic functions, and >> this patch allows them to be used in the ways you would expect variadic >> functions to be usable. > I wrote doc (just one sentence) and minimal test. Both can be enhanced. I remain of the opinion that this patch is a mess. I don't share Pavel's opinion that this is a clean user API, though I'll grant that others might have different opinions on that. I could hold my nose and overlook that if it led to a clean internal implementation. But it doesn't: this patch just bolts a huge, undocumented wart onto the side of MinMaxExpr. (The arguments are in the args field, except when they aren't? And everyplace that deals with MinMaxExpr needs to know that, as well as the fact that the semantics are totally different? Ick.) An example of the lack of care here is that the change in struct ExprEvalStep breaks that struct's size constraint: * Inline data for the operation. Inline data is faster to access, but * also bloats the size of all instructions. The union should be kept to * no more than 40 bytes on 64-bit systems (so that the entire struct is * no more than 64 bytes, a single cacheline on common systems). Andres is going to be quite displeased if that gets committed ;-). I still say we should reject this and invent array_greatest/array_least functions instead. regards, tom lane
Re: proposal: variadic argument support for least, greatest function
Hi
čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack
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:not tested
>
> Latest patch passes installcheck-world as of d26a810 and does what it sets
> out to do.
>
> I am not sure I have an answer to the objections being raised on grounds
> of taste. To me, it's persuasive that GREATEST and LEAST are described in
> the docco as functions, they are used much like variadic functions, and
> this patch allows them to be used in the ways you would expect variadic
> functions to be usable.
>
> But as to technical readiness, this builds and passes installcheck-world.
> The functions behave as expected (and return null if passed an empty array,
> or an array containing only nulls, or variadic null::foo[]).
>
> Still no corresponding regression tests or doc.
>
> The new status of this patch is: Waiting on Author
>
I wrote doc (just one sentence) and minimal test. Both can be enhanced.
Regards
Pavel
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
LEAST(value , ...)
+
+
+GREATEST(VARIADIC anyarray)
+
+
+LEAST(VARIADIC anyarray)
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
+
+
+These functions supports passing parameters as a array after
+VARIADIC keyword.
+
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index db3777d15e..01d7f0e02c 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1854,13 +1854,18 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_MinMaxExpr:
{
MinMaxExpr *minmaxexpr = (MinMaxExpr *) node;
-int nelems = list_length(minmaxexpr->args);
+int nelems;
TypeCacheEntry *typentry;
FmgrInfo *finfo;
FunctionCallInfo fcinfo;
ListCell *lc;
int off;
+if (minmaxexpr->args)
+ nelems = list_length(minmaxexpr->args);
+else
+ nelems = 1;
+
/* Look up the btree comparison function for the datatype */
typentry = lookup_type_cache(minmaxexpr->minmaxtype,
TYPECACHE_CMP_PROC);
@@ -1897,16 +1902,29 @@ ExecInitExprRec(Expr *node, ExprState *state,
scratch.d.minmax.finfo = finfo;
scratch.d.minmax.fcinfo_data = fcinfo;
-/* evaluate expressions into minmax->values/nulls */
-off = 0;
-foreach(lc, minmaxexpr->args)
+if (minmaxexpr->args)
{
- Expr *e = (Expr *) lfirst(lc);
+ scratch.d.minmax.variadic_arg = false;
- ExecInitExprRec(e, state,
- [off],
- [off]);
- off++;
+ /* evaluate expressions into minmax->values/nulls */
+ off = 0;
+ foreach(lc, minmaxexpr->args)
+ {
+ Expr *e = (Expr *) lfirst(lc);
+
+ ExecInitExprRec(e, state,
+ [off],
+ [off]);
+ off++;
+ }
+}
+else
+{
+ scratch.d.minmax.variadic_arg = true;
+
+ ExecInitExprRec(minmaxexpr->variadic_arg, state,
+ [0],
+ [0]);
}
/* and push the final comparison */
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..748c950885 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2819,6 +2819,55 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
+ if (op->d.minmax.variadic_arg)
+ {
+ ArrayIterator array_iterator;
+ ArrayType *arr;
+ Datum value;
+ bool isnull;
+
+ if (nulls[0])
+ return;
+
+ arr = DatumGetArrayTypeP(values[0]);
+
+ array_iterator = array_create_iterator(arr, 0, NULL);
+ while (array_iterate(array_iterator, , ))
+ {
+ if (isnull)
+continue;
+
+ if (*op->resnull)
+ {
+/* first nonnull input */
+*op->resvalue = value;
+*op->resnull = false;
+ }
+ else
+ {
+int cmpresult;
+
+Assert(fcinfo->nargs == 2);
+
+/* apply comparison function */
+fcinfo->args[0].value = *op->resvalue;
+fcinfo->args[1].value = value;
+
+fcinfo->isnull = false;
+cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
+if (fcinfo->isnull) /* probably should not happen */
+ continue;
+
+if (cmpresult > 0 && operator == IS_LEAST)
+ *op->resvalue = value;
+else if (cmpresult < 0 && operator == IS_GREATEST)
+ *op->resvalue = value;
+ }
+ }
+
+ return;
+ }
+
for (off = 0; off < op->d.minmax.nelems; off++)
{
/* ignore NULL inputs */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index e15724bb0e..e8ad89799f 100644
---
Re: proposal: variadic argument support for least, greatest function
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Latest patch passes installcheck-world as of d26a810 and does what it sets out to do. I am not sure I have an answer to the objections being raised on grounds of taste. To me, it's persuasive that GREATEST and LEAST are described in the docco as functions, they are used much like variadic functions, and this patch allows them to be used in the ways you would expect variadic functions to be usable. But as to technical readiness, this builds and passes installcheck-world. The functions behave as expected (and return null if passed an empty array, or an array containing only nulls, or variadic null::foo[]). Still no corresponding regression tests or doc. The new status of this patch is: Waiting on Author
Re: proposal: variadic argument support for least, greatest function
Hi
út 12. 2. 2019 v 2:00 odesílatel Chapman Flack
napsal:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, failed
> Implements feature: not tested
> Spec compliant: not tested
> Documentation:not tested
>
> The argument for consistency with other functions that are variadic makes
> sense to me: although they are different from ordinary variadic functions
> in the type unification they do, their description in the manual simply
> calls them functions without calling attention to any way that they are
> different beasts. So, a reader might reasonably be surprised that VARIADIC
> doesn't work in the usual way.
>
> This patch applies (with some offsets) but the build produces several
> incompatible pointer type assignment warnings, and fails on errors where
> fcinfo->arg is no longer a thing (so should be rebased over the
> variable-length function call args patch).
>
> It does not yet add regression tests, or update the documentation in
> func.sgml.
here is fixed patch
Regards
Pavel
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index db3777d15e..01d7f0e02c 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1854,13 +1854,18 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_MinMaxExpr:
{
MinMaxExpr *minmaxexpr = (MinMaxExpr *) node;
-int nelems = list_length(minmaxexpr->args);
+int nelems;
TypeCacheEntry *typentry;
FmgrInfo *finfo;
FunctionCallInfo fcinfo;
ListCell *lc;
int off;
+if (minmaxexpr->args)
+ nelems = list_length(minmaxexpr->args);
+else
+ nelems = 1;
+
/* Look up the btree comparison function for the datatype */
typentry = lookup_type_cache(minmaxexpr->minmaxtype,
TYPECACHE_CMP_PROC);
@@ -1897,16 +1902,29 @@ ExecInitExprRec(Expr *node, ExprState *state,
scratch.d.minmax.finfo = finfo;
scratch.d.minmax.fcinfo_data = fcinfo;
-/* evaluate expressions into minmax->values/nulls */
-off = 0;
-foreach(lc, minmaxexpr->args)
+if (minmaxexpr->args)
{
- Expr *e = (Expr *) lfirst(lc);
+ scratch.d.minmax.variadic_arg = false;
- ExecInitExprRec(e, state,
- [off],
- [off]);
- off++;
+ /* evaluate expressions into minmax->values/nulls */
+ off = 0;
+ foreach(lc, minmaxexpr->args)
+ {
+ Expr *e = (Expr *) lfirst(lc);
+
+ ExecInitExprRec(e, state,
+ [off],
+ [off]);
+ off++;
+ }
+}
+else
+{
+ scratch.d.minmax.variadic_arg = true;
+
+ ExecInitExprRec(minmaxexpr->variadic_arg, state,
+ [0],
+ [0]);
}
/* and push the final comparison */
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..748c950885 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2819,6 +2819,55 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
+ if (op->d.minmax.variadic_arg)
+ {
+ ArrayIterator array_iterator;
+ ArrayType *arr;
+ Datum value;
+ bool isnull;
+
+ if (nulls[0])
+ return;
+
+ arr = DatumGetArrayTypeP(values[0]);
+
+ array_iterator = array_create_iterator(arr, 0, NULL);
+ while (array_iterate(array_iterator, , ))
+ {
+ if (isnull)
+continue;
+
+ if (*op->resnull)
+ {
+/* first nonnull input */
+*op->resvalue = value;
+*op->resnull = false;
+ }
+ else
+ {
+int cmpresult;
+
+Assert(fcinfo->nargs == 2);
+
+/* apply comparison function */
+fcinfo->args[0].value = *op->resvalue;
+fcinfo->args[1].value = value;
+
+fcinfo->isnull = false;
+cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
+if (fcinfo->isnull) /* probably should not happen */
+ continue;
+
+if (cmpresult > 0 && operator == IS_LEAST)
+ *op->resvalue = value;
+else if (cmpresult < 0 && operator == IS_GREATEST)
+ *op->resvalue = value;
+ }
+ }
+
+ return;
+ }
+
for (off = 0; off < op->d.minmax.nelems; off++)
{
/* ignore NULL inputs */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index b44ead269f..4d1c209c6a 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1942,6 +1942,7 @@ _copyMinMaxExpr(const MinMaxExpr *from)
COPY_SCALAR_FIELD(inputcollid);
COPY_SCALAR_FIELD(op);
COPY_NODE_FIELD(args);
+ COPY_NODE_FIELD(variadic_arg);
COPY_LOCATION_FIELD(location);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 1e169e0b9c..aebc0115ea 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -635,6 +635,7 @@ _equalMinMaxExpr(const MinMaxExpr *a, const MinMaxExpr *b)
COMPARE_SCALAR_FIELD(inputcollid);
COMPARE_SCALAR_FIELD(op);
Re: proposal: variadic argument support for least, greatest function
st 9. 1. 2019 v 1:07 odesílatel Tom Lane napsal:
> Pavel Stehule writes:
> > We cannot to write
> > SELECT least(VARIADIC ARRAY[1,2,3]);
> > Attached patch add this possibility to least, greatest functions.
>
> TBH, I don't find that natural at all. If I were looking for the
> functionality "smallest element of an array", I think I'd expect to find
> that exposed as "array_smallest(anyarray) returns anyelement", not as
> some weird syntax option for LEAST.
>
The target of this patch is a consistency LEAST, GREATEST variadic
functions (implementet) with generic variadic functions.
Sure it is possible to implement array_smallest(anyarray), but it
different. This patch try to eliminate unpleasing surprising about
different behave LEAST, GREATEST from other variadic functions.
> It also seems rather inconsistent that this behaves so differently
> from, eg,
>
> =# select least(array[1,2], array[3,4]);
> least
> ---
> {1,2}
> (1 row)
>
> Normally, if you have a variadic function, it doesn't also take arrays,
> so that there's less possibility for confusion.
>
This is different case - the keyword VARIADIC was not used here.
> The implementation seems mighty ugly too, in that it has to treat this
> as entirely disjoint from MinMaxExpr's normal argument interpretation.
> But that seems like a symptom of the fact that the definition is
> disjointed itself.
>
I don't think so there is any other possibility - I have not a possibility
to unpack a array to elements inside analyze stage.
> In short, I'd rather see this done with a couple of array functions,
> independently of MinMaxExpr.
>
It doesn't help to user, when they try to use VARIADIC keyword on LEAST,
GREATEST functions.
Regards
Pavel
>
> regards, tom lane
>
Re: proposal: variadic argument support for least, greatest function
Pavel Stehule writes:
> We cannot to write
> SELECT least(VARIADIC ARRAY[1,2,3]);
> Attached patch add this possibility to least, greatest functions.
TBH, I don't find that natural at all. If I were looking for the
functionality "smallest element of an array", I think I'd expect to find
that exposed as "array_smallest(anyarray) returns anyelement", not as
some weird syntax option for LEAST.
It also seems rather inconsistent that this behaves so differently
from, eg,
=# select least(array[1,2], array[3,4]);
least
---
{1,2}
(1 row)
Normally, if you have a variadic function, it doesn't also take arrays,
so that there's less possibility for confusion.
The implementation seems mighty ugly too, in that it has to treat this
as entirely disjoint from MinMaxExpr's normal argument interpretation.
But that seems like a symptom of the fact that the definition is
disjointed itself.
In short, I'd rather see this done with a couple of array functions,
independently of MinMaxExpr.
regards, tom lane
Re: proposal: variadic argument support for least, greatest function
so 10. 11. 2018 v 19:12 odesílatel Vik Fearing napsal: > On 08/11/2018 15:59, Pavel Stehule wrote: > > Hi > > > > We can pass variadic arguments as a array to any variadic function. But > > some our old variadic functions doesn't supports this feature. > > > > We cannot to write > > > > SELECT least(VARIADIC ARRAY[1,2,3]); > > > > Attached patch add this possibility to least, greatest functions. > > Is there any particular reason you didn't just make least and greatest > actual functions? > These functions has are able to use most common type and enforce casting. Generic variadic function cannot do it. Regards Pavel -- > Vik Fearing +33 6 46 75 15 36 > http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support >
Re: proposal: variadic argument support for least, greatest function
> "Vik" == Vik Fearing writes: >> Attached patch add this possibility to least, greatest functions. Vik> Is there any particular reason you didn't just make least and Vik> greatest actual functions? least() and greatest() have some type unification logic that I don't think works for actual functions. create function s(variadic anyarray) returns anyelement language sql immutable as $$ select min(v) from unnest($1) u(v); $$; select s(1,2,3); -- works select s(1,2,3.0); -- ERROR: function s(integer, integer, numeric) does not exist select least(1,2,3.0); -- works -- Andrew (irc:RhodiumToad)
Re: proposal: variadic argument support for least, greatest function
On 08/11/2018 15:59, Pavel Stehule wrote: > Hi > > We can pass variadic arguments as a array to any variadic function. But > some our old variadic functions doesn't supports this feature. > > We cannot to write > > SELECT least(VARIADIC ARRAY[1,2,3]); > > Attached patch add this possibility to least, greatest functions. Is there any particular reason you didn't just make least and greatest actual functions? -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
proposal: variadic argument support for least, greatest function
Hi
We can pass variadic arguments as a array to any variadic function. But
some our old variadic functions doesn't supports this feature.
We cannot to write
SELECT least(VARIADIC ARRAY[1,2,3]);
Attached patch add this possibility to least, greatest functions.
Regards
Pavel
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 885da18306..53021b1ae2 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1852,13 +1852,18 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_MinMaxExpr:
{
MinMaxExpr *minmaxexpr = (MinMaxExpr *) node;
-int nelems = list_length(minmaxexpr->args);
+int nelems;
TypeCacheEntry *typentry;
FmgrInfo *finfo;
FunctionCallInfo fcinfo;
ListCell *lc;
int off;
+if (minmaxexpr->args)
+ nelems = list_length(minmaxexpr->args);
+else
+ nelems = 1;
+
/* Look up the btree comparison function for the datatype */
typentry = lookup_type_cache(minmaxexpr->minmaxtype,
TYPECACHE_CMP_PROC);
@@ -1895,16 +1900,29 @@ ExecInitExprRec(Expr *node, ExprState *state,
scratch.d.minmax.finfo = finfo;
scratch.d.minmax.fcinfo_data = fcinfo;
-/* evaluate expressions into minmax->values/nulls */
-off = 0;
-foreach(lc, minmaxexpr->args)
+if (minmaxexpr->args)
{
- Expr *e = (Expr *) lfirst(lc);
+ scratch.d.minmax.variadic_arg = false;
- ExecInitExprRec(e, state,
- [off],
- [off]);
- off++;
+ /* evaluate expressions into minmax->values/nulls */
+ off = 0;
+ foreach(lc, minmaxexpr->args)
+ {
+ Expr *e = (Expr *) lfirst(lc);
+
+ ExecInitExprRec(e, state,
+ [off],
+ [off]);
+ off++;
+ }
+}
+else
+{
+ scratch.d.minmax.variadic_arg = true;
+
+ ExecInitExprRec(minmaxexpr->variadic_arg, state,
+ [0],
+ [0]);
}
/* and push the final comparison */
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index c08df6f162..e26487d315 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2791,6 +2791,53 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
+ if (op->d.minmax.variadic_arg)
+ {
+ ArrayIterator array_iterator;
+ ArrayType *arr;
+ Datum value;
+ bool isnull;
+
+ if (nulls[0])
+ return;
+
+ arr = DatumGetArrayTypeP(values[0]);
+
+ array_iterator = array_create_iterator(arr, 0, NULL);
+ while (array_iterate(array_iterator, , ))
+ {
+ if (isnull)
+continue;
+
+ if (*op->resnull)
+ {
+/* first nonnull input */
+*op->resvalue = value;
+*op->resnull = false;
+ }
+ else
+ {
+int cmpresult;
+
+/* apply comparison function */
+fcinfo->arg[0] = *op->resvalue;
+fcinfo->arg[1] = value;
+
+fcinfo->isnull = false;
+cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
+if (fcinfo->isnull) /* probably should not happen */
+ continue;
+
+if (cmpresult > 0 && operator == IS_LEAST)
+ *op->resvalue = value;
+else if (cmpresult < 0 && operator == IS_GREATEST)
+ *op->resvalue = value;
+ }
+ }
+
+ return;
+ }
+
for (off = 0; off < op->d.minmax.nelems; off++)
{
/* ignore NULL inputs */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index e8ea59e34a..3df2d42ad6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1942,6 +1942,7 @@ _copyMinMaxExpr(const MinMaxExpr *from)
COPY_SCALAR_FIELD(inputcollid);
COPY_SCALAR_FIELD(op);
COPY_NODE_FIELD(args);
+ COPY_NODE_FIELD(variadic_arg);
COPY_LOCATION_FIELD(location);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 3bb91c9595..1c6fddac2a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -634,6 +634,7 @@ _equalMinMaxExpr(const MinMaxExpr *a, const MinMaxExpr *b)
COMPARE_SCALAR_FIELD(inputcollid);
COMPARE_SCALAR_FIELD(op);
COMPARE_NODE_FIELD(args);
+ COMPARE_NODE_FIELD(variadic_arg);
COMPARE_LOCATION_FIELD(location);
return true;
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index a10014f755..2fbdcd78f8 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -465,6 +465,9 @@ exprTypmod(const Node *expr)
int32 typmod;
ListCell *arg;
+if (mexpr->variadic_arg)
+ return exprTypmod((Node *) mexpr->variadic_arg);
+
if (exprType((Node *) linitial(mexpr->args)) != minmaxtype)
return -1;
typmod = exprTypmod((Node *) linitial(mexpr->args));
@@ -2065,7 +2068,15 @@ expression_tree_walker(Node *node,
case T_CoalesceExpr:
return walker(((CoalesceExpr *) node)->args, context);
case T_MinMaxExpr:
- return walker(((MinMaxExpr *) node)->args, context);
+ {
+MinMaxExpr *mexpr =
