Обсуждение: proposal: variadic argument support for least, greatest function

On 08/11/2018 15:59, Pavel Stehule wrote:
> Hi
> 
> We can pass variadic arguments as a array to any variadic function. But
> some our old variadic functions doesn't supports this feature.
> 
> We cannot to write
> 
> SELECT least(VARIADIC ARRAY[1,2,3]);
> 
> Attached patch add this possibility to least, greatest functions.

Is there any particular reason you didn't just make least and greatest
actual functions?
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

>>>>> "Vik" == Vik Fearing <vik.fearing@2ndquadrant.com> writes:

 >> Attached patch add this possibility to least, greatest functions.

 Vik> Is there any particular reason you didn't just make least and
 Vik> greatest actual functions?

least() and greatest() have some type unification logic that I don't
think works for actual functions.

create function s(variadic anyarray) returns anyelement
  language sql immutable
  as $$ select min(v) from unnest($1) u(v); $$;

select s(1,2,3); -- works
select s(1,2,3.0);  -- ERROR:  function s(integer, integer, numeric) does not exist
select least(1,2,3.0);  -- works

-- 
Andrew (irc:RhodiumToad)

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

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
ordinaryvariadic functions in the type unification they do, their description in the manual simply calls them functions
withoutcalling attention to any way that they are different beasts. So, a reader might reasonably be surprised that
VARIADICdoesn't work in the usual way.
 

This patch applies (with some offsets) but the build produces several incompatible pointer type assignment warnings,
andfails on errors where fcinfo->arg is no longer a thing (so should be rebased over the variable-length function call
argspatch).
 

It does not yet add regression tests, or update the documentation in func.sgml.

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
andLEAST are described in the docco as functions, they are used much like variadic functions, and this patch allows
themto 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
nullif 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

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

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

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

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

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

On 02/21/19 16:04, Tom Lane wrote:
> Pavel Stehule <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

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

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

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/absenceof args/variadic_args to distinguish the cases. It now uses the args field consistently, and
distinguishesthe cases with new op constants, IS_GREATEST_VARIADIC and IS_LEAST_VARIADIC, assigned at parse time. I
concedeTom'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

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

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

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
suggestionsfrom me, and is improved on the code-structure matters that Tom pointed out. I don't know if it will meet
Tom'sthreshold 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

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
suggestionsfrom me, and is improved on the code-structure matters that Tom pointed out. I don't know if it will meet
Tom'sthreshold for desirability overall, but that sounds like a committer call at this point, so I'll change it to
RfC.

Both committers who have looked at this patch (Tom, and Andres in his 
patch roundup [1]) recommend that it be rejected.

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

Regards,
-- 
-David
david@pgmasters.net

[1] 
https://www.postgresql.org/message-id/20190214203752.t4hl574k6jlu4t25%40alap3.anarazel.de

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

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

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

On 3/11/19 6:43 PM, Andrew Dunstan wrote:
> 
> On 3/6/19 10:24 AM, Chapman Flack wrote:
>> On 3/6/19 10:12 AM, Andrew Dunstan wrote:
>>
>>> Having reviewed the thread, I'm with Andres and Tom. Maybe though we
>>> should have a note somewhere to the effect that you can't use VARIADIC
>>> with these.
>> Perhaps such a note belongs hoisted into the functions-conditional
>> section of the manual, making a general observation that these things
>> are conditional *expressions* that may resemble functions, but in
>> particular, COALESCE, GREATEST, and LEAST cannot be called with
>> keyword VARIADIC and an array argument, as they could if they were
>> ordinary functions.
>>
> 
> 
> I'm going to mark this as rejected. Here's a possible doc patch

+1

-- 
-David
david@pgmasters.net

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> I'm going to mark this as rejected. Here's a possible doc patch

Maybe s/strictly/ordinary/, or some other word?  "strictly"
doesn't convey much to me.  Otherwise seems fine.

            regards, tom lane

On 3/11/19 6:07 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> I'm going to mark this as rejected. Here's a possible doc patch
> Maybe s/strictly/ordinary/, or some other word?  "strictly"
> doesn't convey much to me.  Otherwise seems fine.


OK, done.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 11, 2019 at 3:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > I'm going to mark this as rejected. Here's a possible doc patch
>
> Maybe s/strictly/ordinary/, or some other word?  "strictly"
> doesn't convey much to me.  Otherwise seems fine.
>

How about:

While the COALESCE, GREATEST, and LEAST functions below accept a
variable number of arguments they do not support the passing of a
VARIADIC array.

?

David J.