Skip to content

Commit bdd538c

Browse files
tglsfdctglsfdc
committed
Remove the single-argument form of string_agg(). It added nothing much in
functionality, while creating an ambiguity in usage with ORDER BY that at least two people have already gotten seriously confused by. Also, add an opr_sanity test to check that we don't in future violate the newly minted policy of not having built-in aggregates with the same name and different numbers of parameters. Per discussion of a complaint from Thom Brown.
1 parent 6a36611 commit bdd538c

File tree

11 files changed

+85
-77
lines changed

11 files changed

+85
-77
lines changed

‎doc/src/sgml/func.sgml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<!-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.521.2.1 2010/07/29 19:34:36 petere Exp $ -->
1+
<!-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.521.2.2 2010/08/05 18:21:29 tgl Exp $ -->
22

33
<chapter id="functions">
44
<title>Functions and Operators</title>
@@ -9731,7 +9731,7 @@ SELECT NULLIF(value, '(none)') ...
97319731
<thead>
97329732
<row>
97339733
<entry>Function</entry>
9734-
<entry>Argument Type</entry>
9734+
<entry>Argument Type(s)</entry>
97359735
<entry>Return Type</entry>
97369736
<entry>Description</entry>
97379737
</row>
@@ -9901,17 +9901,17 @@ SELECT NULLIF(value, '(none)') ...
99019901
<primary>string_agg</primary>
99029902
</indexterm>
99039903
<function>
9904-
string_agg(<replaceable class="parameter">expression</replaceable>
9905-
[, <replaceable class="parameter">delimiter</replaceable> ] )
9904+
string_agg(<replaceable class="parameter">expression</replaceable>,
9905+
<replaceable class="parameter">delimiter</replaceable>)
99069906
</function>
99079907
</entry>
99089908
<entry>
9909-
<type>text</type>
9909+
<type>text</type>, <type>text</type>
99109910
</entry>
99119911
<entry>
99129912
<type>text</type>
99139913
</entry>
9914-
<entry>input values concatenated into a string, optionally with delimiters</entry>
9914+
<entry>input values concatenated into a string, separated by delimiter</entry>
99159915
</row>
99169916

99179917
<row>

‎doc/src/sgml/syntax.sgml

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<!-- $PostgreSQL: pgsql/doc/src/sgml/syntax.sgml,v 1.147.2.1 2010/08/04 22:31:55 tgl Exp $ -->
1+
<!-- $PostgreSQL: pgsql/doc/src/sgml/syntax.sgml,v 1.147.2.2 2010/08/05 18:21:29 tgl Exp $ -->
22

33
<chapter id="sql-syntax">
44
<title>SQL Syntax</title>
@@ -1584,16 +1584,17 @@ SELECT array_agg(a ORDER BY b DESC) FROM table;
15841584
<para>
15851585
When dealing with multiple-argument aggregate functions, note that the
15861586
<literal>ORDER BY</> clause goes after all the aggregate arguments.
1587-
For example, this:
1587+
For example, write this:
15881588
<programlisting>
15891589
SELECT string_agg(a, ',' ORDER BY a) FROM table;
15901590
</programlisting>
15911591
not this:
15921592
<programlisting>
1593-
SELECT string_agg(a ORDER BY a, ',') FROM table; -- not what you want
1593+
SELECT string_agg(a ORDER BY a, ',') FROM table; -- incorrect
15941594
</programlisting>
1595-
The latter syntax will be accepted, but <literal>','</> will be
1596-
treated as a (useless) sort key.
1595+
The latter is syntactically valid, but it represents a call of a
1596+
single-argument aggregate function with two <literal>ORDER BY</> keys
1597+
(the second one being rather useless since it's a constant).
15971598
</para>
15981599

15991600
<para>

‎src/backend/utils/adt/varlena.c

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/utils/adt/varlena.c,v 1.177 2010/02/26 02:01:10 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/utils/adt/varlena.c,v 1.177.4.1 2010/08/05 18:21:29 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -3320,7 +3320,7 @@ pg_column_size(PG_FUNCTION_ARGS)
33203320
/*
33213321
* string_agg - Concatenates values and returns string.
33223322
*
3323-
* Syntax: string_agg(value text, delimiter text = '') RETURNS text
3323+
* Syntax: string_agg(value text, delimiter text) RETURNS text
33243324
*
33253325
* Note: Any NULL values are ignored. The first-call delimiter isn't
33263326
* actually used at all, and on subsequent calls the delimiter precedes
@@ -3359,28 +3359,6 @@ string_agg_transfn(PG_FUNCTION_ARGS)
33593359

33603360
state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0);
33613361

3362-
/* Append the element unless null. */
3363-
if (!PG_ARGISNULL(1))
3364-
{
3365-
if (state == NULL)
3366-
state = makeStringAggState(fcinfo);
3367-
appendStringInfoText(state, PG_GETARG_TEXT_PP(1)); /* value */
3368-
}
3369-
3370-
/*
3371-
* The transition type for string_agg() is declared to be "internal",
3372-
* which is a pass-by-value type the same size as a pointer.
3373-
*/
3374-
PG_RETURN_POINTER(state);
3375-
}
3376-
3377-
Datum
3378-
string_agg_delim_transfn(PG_FUNCTION_ARGS)
3379-
{
3380-
StringInfo state;
3381-
3382-
state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0);
3383-
33843362
/* Append the value unless null. */
33853363
if (!PG_ARGISNULL(1))
33863364
{

‎src/include/catalog/catversion.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
3838
* Portions Copyright (c) 1994, Regents of the University of California
3939
*
40-
* $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.587 2010/04/26 14:22:37 momjian Exp $
40+
* $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.587.2.1 2010/08/05 18:21:29 tgl Exp $
4141
*
4242
*-------------------------------------------------------------------------
4343
*/
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/* yyyymmddN */
56-
#define CATALOG_VERSION_NO 201004261
56+
#define CATALOG_VERSION_NO 201008051
5757

5858
#endif

‎src/include/catalog/pg_aggregate.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
99
* Portions Copyright (c) 1994, Regents of the University of California
1010
*
11-
* $PostgreSQL: pgsql/src/include/catalog/pg_aggregate.h,v 1.71 2010/02/01 03:14:43 itagaki Exp $
11+
* $PostgreSQL: pgsql/src/include/catalog/pg_aggregate.h,v 1.71.6.1 2010/08/05 18:21:29 tgl Exp $
1212
*
1313
* NOTES
1414
* the genbki.pl script reads this file and generates .bki
@@ -224,8 +224,7 @@ DATA(insert ( 2901 xmlconcat2 - 0 142 _null_ ));
224224
DATA(insert ( 2335 array_agg_transfn array_agg_finalfn 0 2281 _null_ ));
225225

226226
/* text */
227-
DATA(insert (3537 string_agg_transfn string_agg_finalfn 0 2281 _null_ ));
228-
DATA(insert (3538 string_agg_delim_transfn string_agg_finalfn 0 2281 _null_ ));
227+
DATA(insert ( 3538 string_agg_transfn string_agg_finalfn 0 2281 _null_ ));
229228

230229
/*
231230
* prototypes for functions in pg_aggregate.c

‎src/include/catalog/pg_proc.h

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.571.2.1 2010/07/29 20:09:34 tgl Exp $
10+
* $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.571.2.2 2010/08/05 18:21:29 tgl Exp $
1111
*
1212
* NOTES
1313
* The script catalog/genbki.pl reads this file and generates .bki
@@ -2829,16 +2829,12 @@ DATA(insert OID = 2816 ( float8_covar_samp PGNSP PGUID 12 1 0 0 f f f t f i 1
28292829
DESCR("COVAR_SAMP(double, double) aggregate final function");
28302830
DATA(insert OID = 2817 ( float8_corr PGNSP PGUID 12 1 0 0 f f f t f i 1 0 701 "1022" _null_ _null_ _null_ _null_ float8_corr _null_ _null_ _null_ ));
28312831
DESCR("CORR(double, double) aggregate final function");
2832-
DATA(insert OID = 3534 ( string_agg_transfn PGNSP PGUID 12 1 0 0 f f f f f i 2 0 2281 "2281 25" _null_ _null_ _null_ _null_ string_agg_transfn _null_ _null_ _null_ ));
2833-
DESCR("string_agg(text) transition function");
2834-
DATA(insert OID = 3535 ( string_agg_delim_transfn PGNSP PGUID 12 1 0 0 f f f f f i 3 0 2281 "2281 25 25" _null_ _null_ _null_ _null_ string_agg_delim_transfn _null_ _null_ _null_ ));
2832+
DATA(insert OID = 3535 ( string_agg_transfn PGNSP PGUID 12 1 0 0 f f f f f i 3 0 2281 "2281 25 25" _null_ _null_ _null_ _null_ string_agg_transfn _null_ _null_ _null_ ));
28352833
DESCR("string_agg(text, text) transition function");
28362834
DATA(insert OID = 3536 ( string_agg_finalfn PGNSP PGUID 12 1 0 0 f f f f f i 1 0 25 "2281" _null_ _null_ _null_ _null_ string_agg_finalfn _null_ _null_ _null_ ));
2837-
DESCR("string_agg final function");
2838-
DATA(insert OID = 3537 ( string_agg PGNSP PGUID 12 1 0 0 t f f f f i 1 0 25 "25" _null_ _null_ _null_ _null_ aggregate_dummy _null_ _null_ _null_ ));
2839-
DESCR("concatenate aggregate input into an string");
2835+
DESCR("string_agg(text, text) final function");
28402836
DATA(insert OID = 3538 ( string_agg PGNSP PGUID 12 1 0 0 t f f f f i 2 0 25 "25 25" _null_ _null_ _null_ _null_ aggregate_dummy _null_ _null_ _null_ ));
2841-
DESCR("concatenate aggregate input into an string with delimiter");
2837+
DESCR("concatenate aggregate input into a string");
28422838

28432839
/* To ASCII conversion */
28442840
DATA(insert OID = 1845 ( to_ascii PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "25" _null_ _null_ _null_ _null_ to_ascii_default _null_ _null_ _null_ ));

‎src/include/utils/builtins.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/utils/builtins.h,v 1.350 2010/07/06 19:19:00 momjian Exp $
10+
* $PostgreSQL: pgsql/src/include/utils/builtins.h,v 1.350.2.1 2010/08/05 18:21:31 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -728,7 +728,6 @@ extern Datum unknownsend(PG_FUNCTION_ARGS);
728728
extern Datum pg_column_size(PG_FUNCTION_ARGS);
729729

730730
extern Datum string_agg_transfn(PG_FUNCTION_ARGS);
731-
extern Datum string_agg_delim_transfn(PG_FUNCTION_ARGS);
732731
extern Datum string_agg_finalfn(PG_FUNCTION_ARGS);
733732

734733
/* version.c */
@@ -779,9 +778,6 @@ extern Datum translate(PG_FUNCTION_ARGS);
779778
extern Datum chr (PG_FUNCTION_ARGS);
780779
extern Datum repeat(PG_FUNCTION_ARGS);
781780
extern Datum ascii(PG_FUNCTION_ARGS);
782-
extern Datum string_agg_transfn(PG_FUNCTION_ARGS);
783-
extern Datum string_agg_delim_transfn(PG_FUNCTION_ARGS);
784-
extern Datum string_agg_finalfn(PG_FUNCTION_ARGS);
785781

786782
/* inet_net_ntop.c */
787783
extern char *inet_net_ntop(int af, const void *src, int bits,

‎src/test/regress/expected/aggregates.out

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -800,12 +800,6 @@ ERROR: in an aggregate with DISTINCT, ORDER BY expressions must appear in argum
800800
LINE 1: select aggfns(distinct a,a,c order by a,b)
801801
^
802802
-- string_agg tests
803-
select string_agg(a) from (values('aaaa'),('bbbb'),('cccc')) g(a);
804-
string_agg
805-
--------------
806-
aaaabbbbcccc
807-
(1 row)
808-
809803
select string_agg(a,',') from (values('aaaa'),('bbbb'),('cccc')) g(a);
810804
string_agg
811805
----------------
@@ -818,10 +812,10 @@ select string_agg(a,',') from (values('aaaa'),(null),('bbbb'),('cccc')) g(a);
818812
aaaa,bbbb,cccc
819813
(1 row)
820814

821-
select string_agg(a,',') from (values(null),(null),('bbbb'),('cccc')) g(a);
815+
select string_agg(a,'AB') from (values(null),(null),('bbbb'),('cccc')) g(a);
822816
string_agg
823817
------------
824-
bbbb,cccc
818+
bbbbABcccc
825819
(1 row)
826820

827821
select string_agg(a,',') from (values(null),(null)) g(a);
@@ -831,23 +825,23 @@ select string_agg(a,',') from (values(null),(null)) g(a);
831825
(1 row)
832826

833827
-- check some implicit casting cases, as per bug #5564
834-
select string_agg(distinct f1 order by f1) from varchar_tbl; -- ok
828+
select string_agg(distinct f1, ',' order by f1) from varchar_tbl; -- ok
835829
string_agg
836830
------------
837-
aababcd
831+
a,ab,abcd
838832
(1 row)
839833

840-
select string_agg(distinct f1::text order by f1) from varchar_tbl; -- not ok
834+
select string_agg(distinct f1::text, ',' order by f1) from varchar_tbl; -- not ok
841835
ERROR: in an aggregate with DISTINCT, ORDER BY expressions must appear in argument list
842-
LINE 1: select string_agg(distinct f1::text order by f1) from varcha...
843-
^
844-
select string_agg(distinct f1 order by f1::text) from varchar_tbl; -- not ok
836+
LINE 1: select string_agg(distinct f1::text, ',' order by f1) from v...
837+
^
838+
select string_agg(distinct f1, ',' order by f1::text) from varchar_tbl; -- not ok
845839
ERROR: in an aggregate with DISTINCT, ORDER BY expressions must appear in argument list
846-
LINE 1: select string_agg(distinct f1 order by f1::text) from varcha...
847-
^
848-
select string_agg(distinct f1::text order by f1::text) from varchar_tbl; -- ok
840+
LINE 1: select string_agg(distinct f1, ',' order by f1::text) from v...
841+
^
842+
select string_agg(distinct f1::text, ',' order by f1::text) from varchar_tbl; -- ok
849843
string_agg
850844
------------
851-
aababcd
845+
a,ab,abcd
852846
(1 row)
853847

‎src/test/regress/expected/opr_sanity.out

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,6 +773,31 @@ ORDER BY 1, 2;
773773
min | < | 1
774774
(2 rows)
775775

776+
-- Check that there are not aggregates with the same name and different
777+
-- numbers of arguments. While not technically wrong, we have a project policy
778+
-- to avoid this because it opens the door for confusion in connection with
779+
-- ORDER BY: novices frequently put the ORDER BY in the wrong place.
780+
-- See the fate of the single-argument form of string_agg() for history.
781+
-- The only aggregates that should show up here are count(x) and count(*).
782+
SELECT p1.oid::regprocedure, p2.oid::regprocedure
783+
FROM pg_proc AS p1, pg_proc AS p2
784+
WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
785+
p1.proisagg AND p2.proisagg AND
786+
array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
787+
ORDER BY 1;
788+
oid | oid
789+
--------------+---------
790+
count("any") | count()
791+
(1 row)
792+
793+
-- For the same reason, aggregates with default arguments are no good.
794+
SELECT oid, proname
795+
FROM pg_proc AS p
796+
WHERE proisagg AND proargdefaults IS NOT NULL;
797+
oid | proname
798+
-----+---------
799+
(0 rows)
800+
776801
-- **************** pg_opfamily ****************
777802
-- Look for illegal values in pg_opfamily fields
778803
SELECT p1.oid

‎src/test/regress/sql/aggregates.sql

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -357,14 +357,13 @@ select aggfns(distinct a,a,c order by a,b)
357357
from (values (1,1,'foo')) v(a,b,c), generate_series(1,2) i;
358358

359359
-- string_agg tests
360-
select string_agg(a) from (values('aaaa'),('bbbb'),('cccc')) g(a);
361360
select string_agg(a,',') from (values('aaaa'),('bbbb'),('cccc')) g(a);
362361
select string_agg(a,',') from (values('aaaa'),(null),('bbbb'),('cccc')) g(a);
363-
select string_agg(a,',') from (values(null),(null),('bbbb'),('cccc')) g(a);
362+
select string_agg(a,'AB') from (values(null),(null),('bbbb'),('cccc')) g(a);
364363
select string_agg(a,',') from (values(null),(null)) g(a);
365364

366365
-- check some implicit casting cases, as per bug #5564
367-
select string_agg(distinct f1 order by f1) from varchar_tbl; -- ok
368-
select string_agg(distinct f1::text order by f1) from varchar_tbl; -- not ok
369-
select string_agg(distinct f1 order by f1::text) from varchar_tbl; -- not ok
370-
select string_agg(distinct f1::text order by f1::text) from varchar_tbl; -- ok
366+
select string_agg(distinct f1, ',' order by f1) from varchar_tbl; -- ok
367+
select string_agg(distinct f1::text, ',' order by f1) from varchar_tbl; -- not ok
368+
select string_agg(distinct f1, ',' order by f1::text) from varchar_tbl; -- not ok
369+
select string_agg(distinct f1::text, ',' order by f1::text) from varchar_tbl; -- ok

‎src/test/regress/sql/opr_sanity.sql

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,26 @@ WHERE a.aggfnoid = p.oid AND a.aggsortop = o.oid AND
621621
amopmethod = (SELECT oid FROM pg_am WHERE amname = 'btree')
622622
ORDER BY 1, 2;
623623

624+
-- Check that there are not aggregates with the same name and different
625+
-- numbers of arguments. While not technically wrong, we have a project policy
626+
-- to avoid this because it opens the door for confusion in connection with
627+
-- ORDER BY: novices frequently put the ORDER BY in the wrong place.
628+
-- See the fate of the single-argument form of string_agg() for history.
629+
-- The only aggregates that should show up here are count(x) and count(*).
630+
631+
SELECT p1.oid::regprocedure, p2.oid::regprocedure
632+
FROM pg_proc AS p1, pg_proc AS p2
633+
WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
634+
p1.proisagg AND p2.proisagg AND
635+
array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
636+
ORDER BY 1;
637+
638+
-- For the same reason, aggregates with default arguments are no good.
639+
640+
SELECT oid, proname
641+
FROM pg_proc AS p
642+
WHERE proisagg AND proargdefaults IS NOT NULL;
643+
624644
-- **************** pg_opfamily ****************
625645

626646
-- Look for illegal values in pg_opfamily fields

0 commit comments

Comments
 (0)