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

proposal: variadic argument support for least, greatest function

From
Pavel Stehule
Date:
Hi

We can pass variadic arguments as a array to any variadic function. But some our old variadic functions doesn't supports this feature.

We cannot to write

SELECT least(VARIADIC ARRAY[1,2,3]);

Attached patch add this possibility to least, greatest functions.

Regards

Pavel
Attachment

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

From
Vik Fearing
Date:
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


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

From
Andrew Gierth
Date:
>>>>> "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)


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

From
Pavel Stehule
Date:


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


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

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

From
Tom Lane
Date:
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


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

From
Pavel Stehule
Date:


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
 

                        regards, tom lane

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

From
Chapman Flack
Date:
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.

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

From
Pavel Stehule
Date:
Hi

ú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
Attachment

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

From
Chapman Flack
Date:
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

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

From
Pavel Stehule
Date:
Hi

č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
Attachment

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

From
Tom Lane
Date:
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


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

From
David Fetter
Date:
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


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

From
Tom Lane
Date:
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


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

From
Chapman Flack
Date:
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

Attachment

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

From
Chapman Flack
Date:
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

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

From
Pavel Stehule
Date:
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?



                        regards, tom lane
Attachment

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

From
Pavel Stehule
Date:


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




                        regards, tom lane
Attachment

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

From
Chapman Flack
Date:
On 02/22/19 14:57, Pavel Stehule wrote:
> I am sending second variant (little bit longer, but the main code is not
> repeated)

minmax_variadic-20190222-3.patch same as -2 but for doc grammar fix
(same fix made in minmax_variadic-20190221b.patch).

Regards,
-Chap


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

From
Chapman Flack
Date:
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

Attachment

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

From
Chapman Flack
Date:
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

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

From
Pavel Stehule
Date:


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?



Regards,
-Chap

The new status of this patch is: Waiting on Author
Attachment

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

From
Chapman Flack
Date:
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


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

From
Pavel Stehule
Date:


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



-Chap
Attachment

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

From
Chapman Flack
Date:
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


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

From
Pavel Stehule
Date:


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


-Chap
Attachment

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

From
Chapman Flack
Date:
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

Re: Re: proposal: variadic argument support for least, greatestfunction

From
David Steele
Date:
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


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

From
Andrew Dunstan
Date:
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



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

From
Chapman Flack
Date:
On 3/6/19 10:12 AM, Andrew Dunstan wrote:

> Having reviewed the thread, I'm with Andres and Tom. Maybe though we
> should have a note somewhere to the effect that you can't use VARIADIC
> with these.

Perhaps such a note belongs hoisted into the functions-conditional
section of the manual, making a general observation that these things
are conditional *expressions* that may resemble functions, but in
particular, COALESCE, GREATEST, and LEAST cannot be called with
keyword VARIADIC and an array argument, as they could if they were
ordinary functions.

Regards,
-Chap


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

From
Pavel Stehule
Date:


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.

+1

Pavel


Regards,
-Chap

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

From
Andrew Dunstan
Date:
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


Attachment

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

From
David Steele
Date:
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


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

From
Pavel Stehule
Date:


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


ok

Pavel

--
-David
david@pgmasters.net

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

From
Tom Lane
Date:
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


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

From
Andrew Dunstan
Date:
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



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

From
"David G. Johnston"
Date:
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.