proposal: variadic argument support for least, greatest function

pavel.stehule@gmail.com

about 7 years ago

In reply to: Vik Fearing (#2)

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

so 10. 11. 2018 v 19:12 odesílatel Vik Fearing <vik.fearing@2ndquadrant.com>
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

Show quoted text

Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

tgl@sss.pgh.pa.us

about 7 years ago

In reply to: Pavel Stehule (#1)

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

Pavel Stehule <pavel.stehule@gmail.com> 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

pavel.stehule@gmail.com

about 7 years ago

In reply to: Tom Lane (#5)

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

st 9. 1. 2019 v 1:07 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> 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

Show quoted text

regards, tom lane

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#6)

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

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.

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#7)

1 attachment(s)

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

út 12. 2. 2019 v 2:00 odesílatel Chapman Flack <chap@anastigmatix.net>
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

Attachments:

minmax_variadic-20190212.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190212.patchDownload

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,
-									&scratch.d.minmax.values[off],
-									&scratch.d.minmax.nulls[off]);
-					off++;
+					/* evaluate expressions into minmax->values/nulls */
+					off = 0;
+					foreach(lc, minmaxexpr->args)
+					{
+						Expr	   *e = (Expr *) lfirst(lc);
+
+						ExecInitExprRec(e, state,
+										&scratch.d.minmax.values[off],
+										&scratch.d.minmax.nulls[off]);
+						off++;
+					}
+				}
+				else
+				{
+					scratch.d.minmax.variadic_arg = true;
+
+					ExecInitExprRec(minmaxexpr->variadic_arg, state,
+										&scratch.d.minmax.values[0],
+										&scratch.d.minmax.nulls[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, &value, &isnull))
+		{
+			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);
 	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
+++ 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));
@@ -2072,7 +2075,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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
@@ -2839,6 +2850,7 @@ expression_tree_mutator(Node *node,
 
 				FLATCOPY(newnode, minmaxexpr, MinMaxExpr);
 				MUTATE(newnode->args, minmaxexpr->args, List *);
+				MUTATE(newnode->variadic_arg, minmaxexpr->variadic_arg, Expr *);
 				return (Node *) newnode;
 			}
 			break;
@@ -3369,7 +3381,15 @@ raw_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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 10038a22cf..7472294a28 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1498,6 +1498,7 @@ _outMinMaxExpr(StringInfo str, const MinMaxExpr *node)
 	WRITE_OID_FIELD(inputcollid);
 	WRITE_ENUM_FIELD(op, MinMaxOp);
 	WRITE_NODE_FIELD(args);
+	WRITE_NODE_FIELD(variadic_arg);
 	WRITE_LOCATION_FIELD(location);
 }
 
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 3b002778ad..ff69c290ce 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1087,6 +1087,7 @@ _readMinMaxExpr(void)
 	READ_OID_FIELD(inputcollid);
 	READ_ENUM_FIELD(op, MinMaxOp);
 	READ_NODE_FIELD(args);
+	READ_NODE_FIELD(variadic_arg);
 	READ_LOCATION_FIELD(location);
 
 	READ_DONE();
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ef6bbe35d7..92da2c0ee0 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -13778,6 +13778,22 @@ func_expr_common_subexpr:
 					v->location = @1;
 					$$ = (Node *)v;
 				}
+			| GREATEST '(' VARIADIC a_expr ')'
+				{
+					MinMaxExpr *v = makeNode(MinMaxExpr);
+					v->variadic_arg = (Expr *) $4;
+					v->op = IS_GREATEST;
+					v->location = @1;
+					$$ = (Node *)v;
+				}
+			| LEAST '(' VARIADIC a_expr ')'
+				{
+					MinMaxExpr *v = makeNode(MinMaxExpr);
+					v->variadic_arg = (Expr *) $4;
+					v->op = IS_LEAST;
+					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..711de0b5b5 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,35 +2263,60 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 	MinMaxExpr *newm = makeNode(MinMaxExpr);
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
-	const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
-	ListCell   *args;
 
 	newm->op = m->op;
-	foreach(args, m->args)
+
+	if (m->args)
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		ListCell   *args;
+		const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
 
-		newe = transformExprRecurse(pstate, e);
-		newargs = lappend(newargs, newe);
-	}
+		foreach(args, m->args)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+			newe = transformExprRecurse(pstate, e);
+			newargs = lappend(newargs, newe);
+		}
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
+
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		Oid		input_type;
+		Oid		element_typeid;
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		Assert(m->variadic_arg);
+
+		newm->variadic_arg = (Expr *) transformExprRecurse(pstate, (Node *) m->variadic_arg);
+		input_type = exprType((Node *) newm->variadic_arg);
+
+		if (input_type == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typeid = get_base_element_type(input_type);
+		if (!OidIsValid(element_typeid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typeid;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 9fd1ebf3e5..b6e366595d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8462,7 +8462,17 @@ get_rule_expr(Node *node, deparse_context *context,
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (minmaxexpr->args)
+				{
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+				}
+				else
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr((Node *) minmaxexpr->variadic_arg, context, true);
+				}
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 7aacdc5d04..7b74f0d9f3 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -459,6 +459,7 @@ typedef struct ExprEvalStep
 			/* workspace for argument values */
 			Datum	   *values;
 			bool	   *nulls;
+			bool		variadic_arg;
 			int			nelems;
 			/* is it GREATEST or LEAST? */
 			MinMaxOp	op;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..4a5a2a4b5a 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1091,6 +1091,7 @@ typedef struct MinMaxExpr
 	Oid			inputcollid;	/* OID of collation that function should use */
 	MinMaxOp	op;				/* function to execute */
 	List	   *args;			/* the arguments */
+	Expr	   *variadic_arg;	/* the variadic argument */
 	int			location;		/* token location, or -1 if unknown */
 } MinMaxExpr;

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#8)

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

#10

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#9)

1 attachment(s)

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

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.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: 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

Attachments:

minmax_variadic-20190221.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190221.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions supports passing parameters as a array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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,
-									&scratch.d.minmax.values[off],
-									&scratch.d.minmax.nulls[off]);
-					off++;
+					/* evaluate expressions into minmax->values/nulls */
+					off = 0;
+					foreach(lc, minmaxexpr->args)
+					{
+						Expr	   *e = (Expr *) lfirst(lc);
+
+						ExecInitExprRec(e, state,
+										&scratch.d.minmax.values[off],
+										&scratch.d.minmax.nulls[off]);
+						off++;
+					}
+				}
+				else
+				{
+					scratch.d.minmax.variadic_arg = true;
+
+					ExecInitExprRec(minmaxexpr->variadic_arg, state,
+										&scratch.d.minmax.values[0],
+										&scratch.d.minmax.nulls[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, &value, &isnull))
+		{
+			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
+++ 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));
@@ -2072,7 +2075,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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
@@ -2839,6 +2850,7 @@ expression_tree_mutator(Node *node,
 
 				FLATCOPY(newnode, minmaxexpr, MinMaxExpr);
 				MUTATE(newnode->args, minmaxexpr->args, List *);
+				MUTATE(newnode->variadic_arg, minmaxexpr->variadic_arg, Expr *);
 				return (Node *) newnode;
 			}
 			break;
@@ -3369,7 +3381,15 @@ raw_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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 65302fe65b..531aa07b61 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1498,6 +1498,7 @@ _outMinMaxExpr(StringInfo str, const MinMaxExpr *node)
 	WRITE_OID_FIELD(inputcollid);
 	WRITE_ENUM_FIELD(op, MinMaxOp);
 	WRITE_NODE_FIELD(args);
+	WRITE_NODE_FIELD(variadic_arg);
 	WRITE_LOCATION_FIELD(location);
 }
 
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 5aa42242a9..14641ddf51 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1088,6 +1088,7 @@ _readMinMaxExpr(void)
 	READ_OID_FIELD(inputcollid);
 	READ_ENUM_FIELD(op, MinMaxOp);
 	READ_NODE_FIELD(args);
+	READ_NODE_FIELD(variadic_arg);
 	READ_LOCATION_FIELD(location);
 
 	READ_DONE();
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a68f78e0e0..975eeb60ff 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->variadic_arg = (Expr *) $4;
+					v->op = IS_GREATEST;
+					v->location = @1;
+					$$ = (Node *)v;
+				}
+			| LEAST '(' VARIADIC a_expr ')'
+				{
+					MinMaxExpr *v = makeNode(MinMaxExpr);
+					v->variadic_arg = (Expr *) $4;
+					v->op = IS_LEAST;
+					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..711de0b5b5 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,35 +2263,60 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 	MinMaxExpr *newm = makeNode(MinMaxExpr);
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
-	const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
-	ListCell   *args;
 
 	newm->op = m->op;
-	foreach(args, m->args)
+
+	if (m->args)
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		ListCell   *args;
+		const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
 
-		newe = transformExprRecurse(pstate, e);
-		newargs = lappend(newargs, newe);
-	}
+		foreach(args, m->args)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+			newe = transformExprRecurse(pstate, e);
+			newargs = lappend(newargs, newe);
+		}
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
+
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		Oid		input_type;
+		Oid		element_typeid;
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		Assert(m->variadic_arg);
+
+		newm->variadic_arg = (Expr *) transformExprRecurse(pstate, (Node *) m->variadic_arg);
+		input_type = exprType((Node *) newm->variadic_arg);
+
+		if (input_type == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typeid = get_base_element_type(input_type);
+		if (!OidIsValid(element_typeid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typeid;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..3431b39d53 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8474,7 +8474,17 @@ get_rule_expr(Node *node, deparse_context *context,
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (minmaxexpr->args)
+				{
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+				}
+				else
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr((Node *) minmaxexpr->variadic_arg, context, true);
+				}
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 7aacdc5d04..7b74f0d9f3 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -459,6 +459,7 @@ typedef struct ExprEvalStep
 			/* workspace for argument values */
 			Datum	   *values;
 			bool	   *nulls;
+			bool		variadic_arg;
 			int			nelems;
 			/* is it GREATEST or LEAST? */
 			MinMaxOp	op;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..4a5a2a4b5a 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1091,6 +1091,7 @@ typedef struct MinMaxExpr
 	Oid			inputcollid;	/* OID of collation that function should use */
 	MinMaxOp	op;				/* function to execute */
 	List	   *args;			/* the arguments */
+	Expr	   *variadic_arg;	/* the variadic argument */
 	int			location;		/* token location, or -1 if unknown */
 } MinMaxExpr;
 
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..f79c4d4efc 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,18 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+
+

#11

tgl@sss.pgh.pa.us

almost 7 years ago

In reply to: Pavel Stehule (#10)

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

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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

#12

David Fetter

david@fetter.org

almost 7 years ago

In reply to: Tom Lane (#11)

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 <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#13

tgl@sss.pgh.pa.us

almost 7 years ago

In reply to: David Fetter (#12)

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

David Fetter <david@fetter.org> 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

#14

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#10)

1 attachment(s)

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

Attachments:

minmax_variadic-20190221b.patchtext/x-patch; name=minmax_variadic-20190221b.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions support passing parameters as an array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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,
-									&scratch.d.minmax.values[off],
-									&scratch.d.minmax.nulls[off]);
-					off++;
+					/* evaluate expressions into minmax->values/nulls */
+					off = 0;
+					foreach(lc, minmaxexpr->args)
+					{
+						Expr	   *e = (Expr *) lfirst(lc);
+
+						ExecInitExprRec(e, state,
+										&scratch.d.minmax.values[off],
+										&scratch.d.minmax.nulls[off]);
+						off++;
+					}
+				}
+				else
+				{
+					scratch.d.minmax.variadic_arg = true;
+
+					ExecInitExprRec(minmaxexpr->variadic_arg, state,
+										&scratch.d.minmax.values[0],
+										&scratch.d.minmax.nulls[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, &value, &isnull))
+		{
+			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
+++ 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));
@@ -2072,7 +2075,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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
@@ -2839,6 +2850,7 @@ expression_tree_mutator(Node *node,
 
 				FLATCOPY(newnode, minmaxexpr, MinMaxExpr);
 				MUTATE(newnode->args, minmaxexpr->args, List *);
+				MUTATE(newnode->variadic_arg, minmaxexpr->variadic_arg, Expr *);
 				return (Node *) newnode;
 			}
 			break;
@@ -3369,7 +3381,15 @@ raw_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 = (MinMaxExpr *) node;
+
+				if (walker(mexpr->args, context))
+					return true;
+				if (walker(mexpr->variadic_arg, context))
+					return true;
+			}
+			break;
 		case T_XmlExpr:
 			{
 				XmlExpr    *xexpr = (XmlExpr *) node;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 65302fe65b..531aa07b61 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1498,6 +1498,7 @@ _outMinMaxExpr(StringInfo str, const MinMaxExpr *node)
 	WRITE_OID_FIELD(inputcollid);
 	WRITE_ENUM_FIELD(op, MinMaxOp);
 	WRITE_NODE_FIELD(args);
+	WRITE_NODE_FIELD(variadic_arg);
 	WRITE_LOCATION_FIELD(location);
 }
 
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 5aa42242a9..14641ddf51 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1088,6 +1088,7 @@ _readMinMaxExpr(void)
 	READ_OID_FIELD(inputcollid);
 	READ_ENUM_FIELD(op, MinMaxOp);
 	READ_NODE_FIELD(args);
+	READ_NODE_FIELD(variadic_arg);
 	READ_LOCATION_FIELD(location);
 
 	READ_DONE();
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a68f78e0e0..975eeb60ff 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->variadic_arg = (Expr *) $4;
+					v->op = IS_GREATEST;
+					v->location = @1;
+					$$ = (Node *)v;
+				}
+			| LEAST '(' VARIADIC a_expr ')'
+				{
+					MinMaxExpr *v = makeNode(MinMaxExpr);
+					v->variadic_arg = (Expr *) $4;
+					v->op = IS_LEAST;
+					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..711de0b5b5 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,35 +2263,60 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 	MinMaxExpr *newm = makeNode(MinMaxExpr);
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
-	const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
-	ListCell   *args;
 
 	newm->op = m->op;
-	foreach(args, m->args)
+
+	if (m->args)
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		ListCell   *args;
+		const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
 
-		newe = transformExprRecurse(pstate, e);
-		newargs = lappend(newargs, newe);
-	}
+		foreach(args, m->args)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+			newe = transformExprRecurse(pstate, e);
+			newargs = lappend(newargs, newe);
+		}
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
+
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		Oid		input_type;
+		Oid		element_typeid;
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		Assert(m->variadic_arg);
+
+		newm->variadic_arg = (Expr *) transformExprRecurse(pstate, (Node *) m->variadic_arg);
+		input_type = exprType((Node *) newm->variadic_arg);
+
+		if (input_type == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typeid = get_base_element_type(input_type);
+		if (!OidIsValid(element_typeid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typeid;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..3431b39d53 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8474,7 +8474,17 @@ get_rule_expr(Node *node, deparse_context *context,
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (minmaxexpr->args)
+				{
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+				}
+				else
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr((Node *) minmaxexpr->variadic_arg, context, true);
+				}
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 7aacdc5d04..7b74f0d9f3 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -459,6 +459,7 @@ typedef struct ExprEvalStep
 			/* workspace for argument values */
 			Datum	   *values;
 			bool	   *nulls;
+			bool		variadic_arg;
 			int			nelems;
 			/* is it GREATEST or LEAST? */
 			MinMaxOp	op;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..4a5a2a4b5a 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1091,6 +1091,7 @@ typedef struct MinMaxExpr
 	Oid			inputcollid;	/* OID of collation that function should use */
 	MinMaxOp	op;				/* function to execute */
 	List	   *args;			/* the arguments */
+	Expr	   *variadic_arg;	/* the variadic argument */
 	int			location;		/* token location, or -1 if unknown */
 } MinMaxExpr;
 
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..f79c4d4efc 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,18 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+
+

#15

chap@anastigmatix.net

almost 7 years ago

In reply to: Chapman Flack (#14)

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

On 02/21/19 16:04, Tom Lane wrote:

Pavel Stehule <pavel.stehule@gmail.com> 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

#16

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Tom Lane (#11)

1 attachment(s)

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

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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?

Show quoted text

regards, tom lane

Attachments:

minmax_variadic-20190222.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190222.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions supports passing parameters as a array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull))
+		{
+			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 ')'
+				{
+					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 = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsVariadicMinMax(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
 			switch (((MinMaxExpr *) node)->op)
 			{
 				case IS_GREATEST:
+				case IS_GREATEST_VARIADIC:
 					*name = "greatest";
 					return 2;
 				case IS_LEAST:
+				case IS_LEAST_VARIADIC:
 					*name = "least";
 					return 2;
 			}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
 				switch (minmaxexpr->op)
 				{
 					case IS_GREATEST:
+					case IS_GREATEST_VARIADIC:
 						appendStringInfoString(buf, "GREATEST(");
 						break;
 					case IS_LEAST:
+					case IS_LEAST_VARIADIC:
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (IsVariadicMinMax(minmaxexpr->op))
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
+				}
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsVariadicMinMax(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#17

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Pavel Stehule (#16)

1 attachment(s)

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

pá 22. 2. 2019 v 13:42 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

Hi

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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

Show quoted text

regards, tom lane

Attachments:

minmax_variadic-20190222-2.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190222-2.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions supports passing parameters as a array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull);
+			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 = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsVariadicMinMax(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
 			switch (((MinMaxExpr *) node)->op)
 			{
 				case IS_GREATEST:
+				case IS_GREATEST_VARIADIC:
 					*name = "greatest";
 					return 2;
 				case IS_LEAST:
+				case IS_LEAST_VARIADIC:
 					*name = "least";
 					return 2;
 			}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
 				switch (minmaxexpr->op)
 				{
 					case IS_GREATEST:
+					case IS_GREATEST_VARIADIC:
 						appendStringInfoString(buf, "GREATEST(");
 						break;
 					case IS_LEAST:
+					case IS_LEAST_VARIADIC:
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (IsVariadicMinMax(minmaxexpr->op))
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
+				}
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsVariadicMinMax(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#18

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#17)

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

#19

chap@anastigmatix.net

almost 7 years ago

In reply to: Chapman Flack (#18)

1 attachment(s)

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

Attachments:

minmax_variadic-20190222-3.patchtext/x-patch; name=minmax_variadic-20190222-3.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions support passing parameters as an array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull);
+			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 = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsVariadicMinMax(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot to determinate result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			elog(ERROR, "expected a array parameter");
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
 			switch (((MinMaxExpr *) node)->op)
 			{
 				case IS_GREATEST:
+				case IS_GREATEST_VARIADIC:
 					*name = "greatest";
 					return 2;
 				case IS_LEAST:
+				case IS_LEAST_VARIADIC:
 					*name = "least";
 					return 2;
 			}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
 				switch (minmaxexpr->op)
 				{
 					case IS_GREATEST:
+					case IS_GREATEST_VARIADIC:
 						appendStringInfoString(buf, "GREATEST(");
 						break;
 					case IS_LEAST:
+					case IS_LEAST_VARIADIC:
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (IsVariadicMinMax(minmaxexpr->op))
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
+				}
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsVariadicMinMax(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#20

chap@anastigmatix.net

almost 7 years ago

In reply to: Chapman Flack (#19)

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

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

#21

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#20)

1 attachment(s)

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

so 23. 2. 2019 v 4:50 odesílatel Chapman Flack <chap@anastigmatix.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?

Show quoted text

Regards,
-Chap

The new status of this patch is: Waiting on Author

Attachments:

minmax_variadic-20190223.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190223.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions supports passing parameters as a array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull);
+			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 = 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 && IsMinMaxLeast(operator))
+				*op->resvalue = value;
+			else if (cmpresult < 0 && IsMinMaxGreatest(operator))
+				*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..f68cebfee8 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 (!IsMinMaxVariadic(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..45aae3f89b 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2276,22 +2276,47 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 		newargs = lappend(newargs, newe);
 	}
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsMinMaxVariadic(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot to determine result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATATYPE_MISMATCH),
+					 errmsg("VARIADIC argument must be an array"),
+					 parser_errposition(pstate,
+								exprLocation((Node *) linitial(newargs)))));
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..de1794201a 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1839,15 +1839,20 @@ FigureColnameInternal(Node *node, char **name)
 			*name = "coalesce";
 			return 2;
 		case T_MinMaxExpr:
-			/* make greatest/least act like a regular function */
-			switch (((MinMaxExpr *) node)->op)
 			{
-				case IS_GREATEST:
+				/* make greatest/least act like a regular function */
+				MinMaxOp op = ((MinMaxExpr *) node)->op;
+
+				if (IsMinMaxGreatest(op))
+				{
 					*name = "greatest";
 					return 2;
-				case IS_LEAST:
+				}
+				else if (IsMinMaxLeast(op))
+				{
 					*name = "least";
 					return 2;
+				}
 			}
 			break;
 		case T_SQLValueFunction:
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..b5a58403ee 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8465,16 +8465,19 @@ get_rule_expr(Node *node, deparse_context *context,
 			{
 				MinMaxExpr *minmaxexpr = (MinMaxExpr *) node;
 
-				switch (minmaxexpr->op)
+				if (IsMinMaxGreatest(minmaxexpr->op))
+					appendStringInfoString(buf, "GREATEST(");
+				else if (IsMinMaxLeast(minmaxexpr->op))
+					appendStringInfoString(buf, "LEAST(");
+
+				if (IsMinMaxVariadic(minmaxexpr->op))
 				{
-					case IS_GREATEST:
-						appendStringInfoString(buf, "GREATEST(");
-						break;
-					case IS_LEAST:
-						appendStringInfoString(buf, "LEAST(");
-						break;
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..d3c10f5dbd 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,18 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsMinMaxVariadic(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+#define IsMinMaxLeast(op)			(op == IS_LEAST || \
+									 op == IS_LEAST_VARIADIC)
+#define IsMinMaxGreatest(op)		(op == IS_GREATEST || \
+									 op == IS_GREATEST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#22

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#21)

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 <chap@anastigmatix.net>
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 <literal>VARIADIC</literal> 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

#23

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#22)

1 attachment(s)

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

so 23. 2. 2019 v 18:28 odesílatel Chapman Flack <chap@anastigmatix.net>
napsal:

On 02/23/19 01:22, Pavel Stehule wrote:

so 23. 2. 2019 v 4:50 odesílatel Chapman Flack <chap@anastigmatix.net>
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 <literal>VARIADIC</literal> 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 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.

Thank you for precious review :)

please, see, attached patch

Show quoted text

-Chap

Attachments:

minmax_variadic-20190223-2.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190223-2.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions supports passing parameters as a array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull);
+			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 *) 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..f274b3ba5c 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,9 +2263,12 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 	MinMaxExpr *newm = makeNode(MinMaxExpr);
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
-	const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
+	const char *funcname;
 	ListCell   *args;
 
+	funcname = (m->op == IS_GREATEST || m->op == IS_GREATEST_VARIADIC)
+					 ? "GREATEST" : "LEAST";
+
 	newm->op = m->op;
 	foreach(args, m->args)
 	{
@@ -2276,22 +2279,47 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 		newargs = lappend(newargs, newe);
 	}
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsVariadicMinMax(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot determine result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATATYPE_MISMATCH),
+					 errmsg("VARIADIC argument must be an array"),
+					 parser_errposition(pstate,
+								exprLocation((Node *) linitial(newargs)))));
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
 			switch (((MinMaxExpr *) node)->op)
 			{
 				case IS_GREATEST:
+				case IS_GREATEST_VARIADIC:
 					*name = "greatest";
 					return 2;
 				case IS_LEAST:
+				case IS_LEAST_VARIADIC:
 					*name = "least";
 					return 2;
 			}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
 				switch (minmaxexpr->op)
 				{
 					case IS_GREATEST:
+					case IS_GREATEST_VARIADIC:
 						appendStringInfoString(buf, "GREATEST(");
 						break;
 					case IS_LEAST:
+					case IS_LEAST_VARIADIC:
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (IsVariadicMinMax(minmaxexpr->op))
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
+				}
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsVariadicMinMax(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#24

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#23)

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 <literal>VARIADIC</literal>
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

#25

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#24)

1 attachment(s)

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

so 23. 2. 2019 v 20:23 odesílatel Chapman Flack <chap@anastigmatix.net>
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 <literal>VARIADIC</literal>
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

Show quoted text

-Chap

Attachments:

minmax_variadic-20190224.patchtext/x-patch; charset=US-ASCII; name=minmax_variadic-20190224.patchDownload

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)') ...
 </synopsis>
 <synopsis>
 <function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
 </synopsis>
 
    <para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
     make them return NULL if any argument is NULL, rather than only when
     all are NULL.
    </para>
+
+   <para>
+    These functions support passing parameters as an array after
+    <literal>VARIADIC</literal> keyword.
+   </para>
   </sect2>
  </sect1>
 
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, &value, &isnull);
+			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 *) 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..7025b5c3f5 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,9 +2263,12 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 	MinMaxExpr *newm = makeNode(MinMaxExpr);
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
-	const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
+	const char *funcname;
 	ListCell   *args;
 
+	funcname = (m->op == IS_GREATEST || m->op == IS_GREATEST_VARIADIC)
+					 ? "GREATEST" : "LEAST";
+
 	newm->op = m->op;
 	foreach(args, m->args)
 	{
@@ -2276,22 +2279,47 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
 		newargs = lappend(newargs, newe);
 	}
 
-	newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
-	/* minmaxcollid and inputcollid will be set by parse_collate.c */
+	if (IsVariadicMinMax(newm->op))
+	{
+		Oid		array_typid;
+		Oid		element_typid;
 
-	/* Convert arguments if necessary */
-	foreach(args, newargs)
+		array_typid = exprType(linitial(newargs));
+
+		if (array_typid == InvalidOid)
+			elog(ERROR, "cannot determine result type");
+
+		element_typid = get_base_element_type(array_typid);
+		if (!OidIsValid(element_typid))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATATYPE_MISMATCH),
+					 errmsg("VARIADIC argument must be an array"),
+					 parser_errposition(pstate,
+								exprLocation((Node *) linitial(newargs)))));
+
+		newm->minmaxtype = element_typid;
+		newm->args = newargs;
+	}
+	else
 	{
-		Node	   *e = (Node *) lfirst(args);
-		Node	   *newe;
+		newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+		/* minmaxcollid and inputcollid will be set by parse_collate.c */
 
-		newe = coerce_to_common_type(pstate, e,
-									 newm->minmaxtype,
-									 funcname);
-		newcoercedargs = lappend(newcoercedargs, newe);
+		/* Convert arguments if necessary */
+		foreach(args, newargs)
+		{
+			Node	   *e = (Node *) lfirst(args);
+			Node	   *newe;
+
+			newe = coerce_to_common_type(pstate, e,
+										 newm->minmaxtype,
+										 funcname);
+			newcoercedargs = lappend(newcoercedargs, newe);
+		}
+
+		newm->args = newcoercedargs;
 	}
 
-	newm->args = newcoercedargs;
 	newm->location = m->location;
 	return (Node *) newm;
 }
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
 			switch (((MinMaxExpr *) node)->op)
 			{
 				case IS_GREATEST:
+				case IS_GREATEST_VARIADIC:
 					*name = "greatest";
 					return 2;
 				case IS_LEAST:
+				case IS_LEAST_VARIADIC:
 					*name = "least";
 					return 2;
 			}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
 				switch (minmaxexpr->op)
 				{
 					case IS_GREATEST:
+					case IS_GREATEST_VARIADIC:
 						appendStringInfoString(buf, "GREATEST(");
 						break;
 					case IS_LEAST:
+					case IS_LEAST_VARIADIC:
 						appendStringInfoString(buf, "LEAST(");
 						break;
 				}
-				get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+				if (IsVariadicMinMax(minmaxexpr->op))
+				{
+					appendStringInfoString(buf, "VARIADIC ");
+					get_rule_expr(linitial(minmaxexpr->args), context, true);
+				}
+				else
+					get_rule_expr((Node *) minmaxexpr->args, context, true);
+
 				appendStringInfoChar(buf, ')');
 			}
 			break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
 typedef enum MinMaxOp
 {
 	IS_GREATEST,
-	IS_LEAST
+	IS_LEAST,
+	IS_GREATEST_VARIADIC,
+	IS_LEAST_VARIADIC
 } MinMaxOp;
 
+#define IsVariadicMinMax(op)		(op == IS_GREATEST_VARIADIC || \
+									 op == IS_LEAST_VARIADIC)
+
 typedef struct MinMaxExpr
 {
 	Expr		xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
 --
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest 
+----------
+       40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least 
+-------
+    10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest 
+----------
+       40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
 
 DROP TABLE CASE_TBL;
 DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);

#26

[1]: /messages/by-id/20190214203752.t4hl574k6jlu4t25@alap3.anarazel.de
/messages/by-id/20190214203752.t4hl574k6jlu4t25@alap3.anarazel.de

chap@anastigmatix.net

almost 7 years ago

In reply to: Pavel Stehule (#25)

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

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

#27

David Steele

david@pgmasters.net

almost 7 years ago

In reply to: Chapman Flack (#26)

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]/messages/by-id/20190214203752.t4hl574k6jlu4t25@alap3.anarazel.de) recommend that it be rejected.

If no committer steps up in the next week I think we should mark it as
rejected.

Regards,
--
-David
david@pgmasters.net

#28

Andrew Dunstan

andrew.dunstan@2ndquadrant.com

almost 7 years ago

In reply to: David Steele (#27)

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 Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#29

chap@anastigmatix.net

almost 7 years ago

In reply to: Andrew Dunstan (#28)

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

#30

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Chapman Flack (#29)

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

st 6. 3. 2019 v 16:24 odesílatel Chapman Flack <chap@anastigmatix.net>
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.

Pavel

Show quoted text

Regards,
-Chap

#31

Andrew Dunstan

andrew.dunstan@2ndquadrant.com

almost 7 years ago

In reply to: Chapman Flack (#29)

1 attachment(s)

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 Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

conditional-variadic-doc.patchtext/x-patch; name=conditional-variadic-doc.patchDownload

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);    <lineannotation>Next <function>nextval</func
    </para>
   </tip>
 
+   <note>
+    <para>
+     Although <token>COALESCE</token>, <token>GREATEST</token>, and
+     <token>LEAST</token> are syntactically similar to functions, they are
+     not strictly functions, and thus cannot be used with explicit
+     <token>VARIADIC</token> array arguments.
+    </para>
+   </note>
+
   <sect2 id="functions-case">
    <title><literal>CASE</literal></title>

#32

David Steele

david@pgmasters.net

almost 7 years ago

In reply to: Andrew Dunstan (#31)

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

--
-David
david@pgmasters.net

#33

pavel.stehule@gmail.com

almost 7 years ago

In reply to: David Steele (#32)

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

po 11. 3. 2019 v 18:04 odesílatel David Steele <david@pgmasters.net> 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

Pavel

Show quoted text

-David
david@pgmasters.net

#34