From b0c451e145be23211dce0718cc63e632959591e8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 5 Aug 2010 18:21:19 +0000 Subject: [PATCH] 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. --- doc/src/sgml/func.sgml | 12 +++++----- doc/src/sgml/syntax.sgml | 11 +++++---- src/backend/utils/adt/varlena.c | 26 ++------------------ src/include/catalog/catversion.h | 4 ++-- src/include/catalog/pg_aggregate.h | 5 ++-- src/include/catalog/pg_proc.h | 12 ++++------ src/include/utils/builtins.h | 6 +---- src/test/regress/expected/aggregates.out | 30 ++++++++++-------------- src/test/regress/expected/opr_sanity.out | 25 ++++++++++++++++++++ src/test/regress/sql/aggregates.sql | 11 ++++----- src/test/regress/sql/opr_sanity.sql | 20 ++++++++++++++++ 11 files changed, 85 insertions(+), 77 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 07ff132cbb..b981ca7f92 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -1,4 +1,4 @@ - + Functions and Operators @@ -9782,7 +9782,7 @@ SELECT NULLIF(value, '(none)') ... Function - Argument Type + Argument Type(s) Return Type Description @@ -9952,17 +9952,17 @@ SELECT NULLIF(value, '(none)') ... string_agg - string_agg(expression - [, delimiter ] ) + string_agg(expression, + delimiter) - text + text, text text - input values concatenated into a string, optionally with delimiters + input values concatenated into a string, separated by delimiter diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml index 16aa89f497..ee2c5214aa 100644 --- a/doc/src/sgml/syntax.sgml +++ b/doc/src/sgml/syntax.sgml @@ -1,4 +1,4 @@ - + SQL Syntax @@ -1583,16 +1583,17 @@ SELECT array_agg(a ORDER BY b DESC) FROM table; When dealing with multiple-argument aggregate functions, note that the ORDER BY clause goes after all the aggregate arguments. - For example, this: + For example, write this: SELECT string_agg(a, ',' ORDER BY a) FROM table; not this: -SELECT string_agg(a ORDER BY a, ',') FROM table; -- not what you want +SELECT string_agg(a ORDER BY a, ',') FROM table; -- incorrect - The latter syntax will be accepted, but ',' 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 ORDER BY keys + (the second one being rather useless since it's a constant). diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index be41c977ff..e9f9f597ec 100644 --- 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 a1a223b227..69e87bb585 100644 --- 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 c47adbe475..0fc28df804 100644 --- 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 8b9e1db581..ffea7c9b57 100644 --- 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 92b3c64aaa..4ba866453c 100644 --- 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 087b4679d4..b456d7e989 100644 --- 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 5e36d481df..f6fee25de5 100644 --- 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 b2199d1ce9..8f81ba763a 100644 --- 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 38866c9a54..46ec24cca6 100644 --- 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