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.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 07ff132cbb5353e93d627044989e255c8e83cfc7..b981ca7f9209e39c989ea05c48dfcbb17624813b 100644 (file)
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.523 2010/08/05 04:21:53 petere Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/func.sgml,v 1.524 2010/08/05 18:21:17 tgl Exp $ -->
 
  <chapter id="functions">
   <title>Functions and Operators</title>
@@ -9782,7 +9782,7 @@ SELECT NULLIF(value, '(none)') ...
     <thead>
      <row>
       <entry>Function</entry>
-      <entry>Argument Type</entry>
+      <entry>Argument Type(s)</entry>
       <entry>Return Type</entry>
       <entry>Description</entry>
      </row>
@@ -9952,17 +9952,17 @@ SELECT NULLIF(value, '(none)') ...
         <primary>string_agg</primary>
        </indexterm>
        <function>
-         string_agg(<replaceable class="parameter">expression</replaceable> 
-                    [, <replaceable class="parameter">delimiter</replaceable> ] )
+         string_agg(<replaceable class="parameter">expression</replaceable>,
+                    <replaceable class="parameter">delimiter</replaceable>)
        </function>
       </entry>
       <entry>
-       <type>text</type>
+       <type>text</type>, <type>text</type>
       </entry>
       <entry>
        <type>text</type>
       </entry>
-      <entry>input values concatenated into a string, optionally with delimiters</entry>
+      <entry>input values concatenated into a string, separated by delimiter</entry>
      </row>
 
      <row>

diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 16aa89f497f3cc2c7066f4acf48a4ae08e4b4742..ee2c5214aa9b7356a9c579742ba5c6ddde3c0ab8 100644 (file)
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/syntax.sgml,v 1.149 2010/08/04 15:27:57 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/syntax.sgml,v 1.150 2010/08/05 18:21:17 tgl Exp $ -->
 
 <chapter id="sql-syntax">
  <title>SQL Syntax</title>
@@ -1583,16 +1583,17 @@ SELECT array_agg(a ORDER BY b DESC) FROM table;
    <para>
     When dealing with multiple-argument aggregate functions, note that the
     <literal>ORDER BY</> clause goes after all the aggregate arguments.
-    For example, this:
+    For example, write this:
 <programlisting>
 SELECT string_agg(a, ',' ORDER BY a) FROM table;
 </programlisting>
     not this:
 <programlisting>
-SELECT string_agg(a ORDER BY a, ',') FROM table;  -- not what you want
+SELECT string_agg(a ORDER BY a, ',') FROM table;  -- incorrect
 </programlisting>
-    The latter syntax will be accepted, but <literal>','</> will be
-    treated as a (useless) sort key.
+    The latter is syntactically valid, but it represents a call of a
+    single-argument aggregate function with two <literal>ORDER BY</> keys
+    (the second one being rather useless since it's a constant).
    </para>
 
    <para>

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index be41c977ffb865cc55a8295f7ce269c28a036ed6..e9f9f597ec79a583a30ebd685d638528064d1cb0 100644 (file)
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/adt/varlena.c,v 1.177 2010/02/26 02:01:10 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/adt/varlena.c,v 1.178 2010/08/05 18:21:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3320,7 +3320,7 @@ pg_column_size(PG_FUNCTION_ARGS)
 /*
  * string_agg - Concatenates values and returns string.
  *
- * Syntax: string_agg(value text, delimiter text = '') RETURNS text
+ * Syntax: string_agg(value text, delimiter text) RETURNS text
  *
  * Note: Any NULL values are ignored. The first-call delimiter isn't
  * actually used at all, and on subsequent calls the delimiter precedes
@@ -3359,28 +3359,6 @@ string_agg_transfn(PG_FUNCTION_ARGS)
 
        state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0);
 
-       /* Append the element unless null. */
-       if (!PG_ARGISNULL(1))
-       {
-               if (state == NULL)
-                       state = makeStringAggState(fcinfo);
-               appendStringInfoText(state, PG_GETARG_TEXT_PP(1));              /* value */
-       }
-
-       /*
-        * The transition type for string_agg() is declared to be "internal",
-        * which is a pass-by-value type the same size as a pointer.
-        */
-       PG_RETURN_POINTER(state);
-}
-
-Datum
-string_agg_delim_transfn(PG_FUNCTION_ARGS)
-{
-       StringInfo      state;
-
-       state = PG_ARGISNULL(0) ? NULL : (StringInfo) PG_GETARG_POINTER(0);
-
        /* Append the value unless null. */
        if (!PG_ARGISNULL(1))
        {

diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index a1a223b227d8fcb06f3ee487bfef32d2771eca54..69e87bb5850091dc73f90c01943a37a08c232e32 100644 (file)
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -37,7 +37,7 @@
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.589 2010/08/05 04:21:54 petere Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/catversion.h,v 1.590 2010/08/05 18:21:17 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -53,6 +53,6 @@
  */
 
 /*                                                     yyyymmddN */
-#define CATALOG_VERSION_NO     201008051
+#define CATALOG_VERSION_NO     201008052
 
 #endif

diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index c47adbe4755c1f807287249fad07b153e58cb473..0fc28df804df30a24dfaa68e395182b48ba49996 100644 (file)
--- a/src/include/catalog/pg_aggregate.h
+++ b/src/include/catalog/pg_aggregate.h
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/pg_aggregate.h,v 1.71 2010/02/01 03:14:43 itagaki Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/pg_aggregate.h,v 1.72 2010/08/05 18:21:17 tgl Exp $
  *
  * NOTES
  *       the genbki.pl script reads this file and generates .bki
@@ -224,8 +224,7 @@ DATA(insert ( 2901 xmlconcat2         -                                     0       142             _null_ ));
 DATA(insert ( 2335     array_agg_transfn       array_agg_finalfn               0       2281    _null_ ));
 
 /* text */
-DATA(insert (3537      string_agg_transfn                      string_agg_finalfn      0       2281    _null_ ));
-DATA(insert (3538      string_agg_delim_transfn        string_agg_finalfn      0       2281    _null_ ));
+DATA(insert ( 3538     string_agg_transfn      string_agg_finalfn              0       2281    _null_ ));
 
 /*
  * prototypes for functions in pg_aggregate.c

diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 8b9e1db5815d5a120c338fa93052c0a75e8111d3..ffea7c9b57ccaf3ba93b030641fe6ab40665ed07 100644 (file)
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.574 2010/08/05 04:21:54 petere Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/pg_proc.h,v 1.575 2010/08/05 18:21:17 tgl Exp $
  *
  * NOTES
  *       The script catalog/genbki.pl reads this file and generates .bki
@@ -2835,16 +2835,12 @@ DATA(insert OID = 2816 (  float8_covar_samp                     PGNSP PGUID 12 1 0 0 f f f t f i 1
 DESCR("COVAR_SAMP(double, double) aggregate final function");
 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_ ));
 DESCR("CORR(double, double) aggregate final function");
-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_ ));
-DESCR("string_agg(text) transition function");
-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_ ));
+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_ ));
 DESCR("string_agg(text, text) transition function");
 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_ ));
-DESCR("string_agg final function");
-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_ ));
-DESCR("concatenate aggregate input into an string");
+DESCR("string_agg(text, text) final function");
 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_ ));
-DESCR("concatenate aggregate input into an string with delimiter");
+DESCR("concatenate aggregate input into a string");
 
 /* To ASCII conversion */
 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_ ));

diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 92b3c64aaada7be35618c69fb4f1fcf721e6f50e..4ba866453c5073758ab7ddae0872389cef0bf549 100644 (file)
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/builtins.h,v 1.352 2010/07/22 01:22:35 rhaas Exp $
+ * $PostgreSQL: pgsql/src/include/utils/builtins.h,v 1.353 2010/08/05 18:21:19 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -729,7 +729,6 @@ extern Datum unknownsend(PG_FUNCTION_ARGS);
 extern Datum pg_column_size(PG_FUNCTION_ARGS);
 
 extern Datum string_agg_transfn(PG_FUNCTION_ARGS);
-extern Datum string_agg_delim_transfn(PG_FUNCTION_ARGS);
 extern Datum string_agg_finalfn(PG_FUNCTION_ARGS);
 
 /* version.c */
@@ -780,9 +779,6 @@ extern Datum translate(PG_FUNCTION_ARGS);
 extern Datum chr (PG_FUNCTION_ARGS);
 extern Datum repeat(PG_FUNCTION_ARGS);
 extern Datum ascii(PG_FUNCTION_ARGS);
-extern Datum string_agg_transfn(PG_FUNCTION_ARGS);
-extern Datum string_agg_delim_transfn(PG_FUNCTION_ARGS);
-extern Datum string_agg_finalfn(PG_FUNCTION_ARGS);
 
 /* inet_net_ntop.c */
 extern char *inet_net_ntop(int af, const void *src, int bits,

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 087b4679d4d826e362a09b3bcf28a9ca4fcc08f9..b456d7e989ab1c5006d85730286f82069e6c3585 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -800,12 +800,6 @@ ERROR:  in an aggregate with DISTINCT, ORDER BY expressions must appear in argum
 LINE 1: select aggfns(distinct a,a,c order by a,b)
                                                 ^
 -- string_agg tests
-select string_agg(a) from (values('aaaa'),('bbbb'),('cccc')) g(a);
-  string_agg  
---------------
- aaaabbbbcccc
-(1 row)
-
 select string_agg(a,',') from (values('aaaa'),('bbbb'),('cccc')) g(a);
    string_agg   
 ----------------
@@ -818,10 +812,10 @@ select string_agg(a,',') from (values('aaaa'),(null),('bbbb'),('cccc')) g(a);
  aaaa,bbbb,cccc
 (1 row)
 
-select string_agg(a,',') from (values(null),(null),('bbbb'),('cccc')) g(a);
+select string_agg(a,'AB') from (values(null),(null),('bbbb'),('cccc')) g(a);
  string_agg 
 ------------
- bbbb,cccc
+ bbbbABcccc
 (1 row)
 
 select string_agg(a,',') from (values(null),(null)) g(a);
@@ -831,23 +825,23 @@ select string_agg(a,',') from (values(null),(null)) g(a);
 (1 row)
 
 -- check some implicit casting cases, as per bug #5564
-select string_agg(distinct f1 order by f1) from varchar_tbl;  -- ok
+select string_agg(distinct f1, ',' order by f1) from varchar_tbl;  -- ok
  string_agg 
 ------------
- aababcd
+ a,ab,abcd
 (1 row)
 
-select string_agg(distinct f1::text order by f1) from varchar_tbl;  -- not ok
+select string_agg(distinct f1::text, ',' order by f1) from varchar_tbl;  -- not ok
 ERROR:  in an aggregate with DISTINCT, ORDER BY expressions must appear in argument list
-LINE 1: select string_agg(distinct f1::text order by f1) from varcha...
-                                                     ^
-select string_agg(distinct f1 order by f1::text) from varchar_tbl;  -- not ok
+LINE 1: select string_agg(distinct f1::text, ',' order by f1) from v...
+                                                          ^
+select string_agg(distinct f1, ',' order by f1::text) from varchar_tbl;  -- not ok
 ERROR:  in an aggregate with DISTINCT, ORDER BY expressions must appear in argument list
-LINE 1: select string_agg(distinct f1 order by f1::text) from varcha...
-                                               ^
-select string_agg(distinct f1::text order by f1::text) from varchar_tbl;  -- ok
+LINE 1: select string_agg(distinct f1, ',' order by f1::text) from v...
+                                                    ^
+select string_agg(distinct f1::text, ',' order by f1::text) from varchar_tbl;  -- ok
  string_agg 
 ------------
- aababcd
+ a,ab,abcd
 (1 row)
 

diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 5e36d481dfbd0bf68948a49278b1ccd5119b3e58..f6fee25de5574cef89640bbf0b3aa5f2dc7b5398 100644 (file)
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -773,6 +773,31 @@ ORDER BY 1, 2;
  min     | <       |            1
 (2 rows)
 
+-- Check that there are not aggregates with the same name and different
+-- numbers of arguments.  While not technically wrong, we have a project policy
+-- to avoid this because it opens the door for confusion in connection with
+-- ORDER BY: novices frequently put the ORDER BY in the wrong place.
+-- See the fate of the single-argument form of string_agg() for history.
+-- The only aggregates that should show up here are count(x) and count(*).
+SELECT p1.oid::regprocedure, p2.oid::regprocedure
+FROM pg_proc AS p1, pg_proc AS p2
+WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
+    p1.proisagg AND p2.proisagg AND
+    array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
+ORDER BY 1;
+     oid      |   oid   
+--------------+---------
+ count("any") | count()
+(1 row)
+
+-- For the same reason, aggregates with default arguments are no good.
+SELECT oid, proname
+FROM pg_proc AS p
+WHERE proisagg AND proargdefaults IS NOT NULL;
+ oid | proname 
+-----+---------
+(0 rows)
+
 -- **************** pg_opfamily ****************
 -- Look for illegal values in pg_opfamily fields
 SELECT p1.oid

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index b2199d1ce94353df9669ba351931cc62cdc2e3ce..8f81ba763a02e1d84e6748ee10f9bcdefc3c2d57 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -357,14 +357,13 @@ select aggfns(distinct a,a,c order by a,b)
   from (values (1,1,'foo')) v(a,b,c), generate_series(1,2) i;
 
 -- string_agg tests
-select string_agg(a) from (values('aaaa'),('bbbb'),('cccc')) g(a);
 select string_agg(a,',') from (values('aaaa'),('bbbb'),('cccc')) g(a);
 select string_agg(a,',') from (values('aaaa'),(null),('bbbb'),('cccc')) g(a);
-select string_agg(a,',') from (values(null),(null),('bbbb'),('cccc')) g(a);
+select string_agg(a,'AB') from (values(null),(null),('bbbb'),('cccc')) g(a);
 select string_agg(a,',') from (values(null),(null)) g(a);
 
 -- check some implicit casting cases, as per bug #5564
-select string_agg(distinct f1 order by f1) from varchar_tbl;  -- ok
-select string_agg(distinct f1::text order by f1) from varchar_tbl;  -- not ok
-select string_agg(distinct f1 order by f1::text) from varchar_tbl;  -- not ok
-select string_agg(distinct f1::text order by f1::text) from varchar_tbl;  -- ok
+select string_agg(distinct f1, ',' order by f1) from varchar_tbl;  -- ok
+select string_agg(distinct f1::text, ',' order by f1) from varchar_tbl;  -- not ok
+select string_agg(distinct f1, ',' order by f1::text) from varchar_tbl;  -- not ok
+select string_agg(distinct f1::text, ',' order by f1::text) from varchar_tbl;  -- ok

diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 38866c9a5499093936c402acf7708b969236dcf9..46ec24cca6f759f691391a7df2bbe85fd2d00eca 100644 (file)
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -621,6 +621,26 @@ WHERE a.aggfnoid = p.oid AND a.aggsortop = o.oid AND
     amopmethod = (SELECT oid FROM pg_am WHERE amname = 'btree')
 ORDER BY 1, 2;
 
+-- Check that there are not aggregates with the same name and different
+-- numbers of arguments.  While not technically wrong, we have a project policy
+-- to avoid this because it opens the door for confusion in connection with
+-- ORDER BY: novices frequently put the ORDER BY in the wrong place.
+-- See the fate of the single-argument form of string_agg() for history.
+-- The only aggregates that should show up here are count(x) and count(*).
+
+SELECT p1.oid::regprocedure, p2.oid::regprocedure
+FROM pg_proc AS p1, pg_proc AS p2
+WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
+    p1.proisagg AND p2.proisagg AND
+    array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
+ORDER BY 1;
+
+-- For the same reason, aggregates with default arguments are no good.
+
+SELECT oid, proname
+FROM pg_proc AS p
+WHERE proisagg AND proargdefaults IS NOT NULL;
+
 -- **************** pg_opfamily ****************
 
 -- Look for illegal values in pg_opfamily fields