From 6ff80e081ef2e853329eda19b2b24f6ef3da96ff Mon Sep 17 00:00:00 2001 From: Andrei Lepikhov Date: Wed, 19 Aug 2026 17:44:44 +0000 Subject: [PATCH 2/2] Allow a planner support function to be attached to an aggregate Commit 42473b3b31 added the SupportRequestSimplifyAggref request, but an extension had no way to reach it. AlterFunction() rejected every aggregate, and no other DDL command can set pg_proc.prosupport, so the only remaining option was a direct catalog update, which records no dependency. A support hook that an extension cannot use defeats the purpose of the support function mechanism. Teach compute_common_attribute() about prokind, rather than a plain is_procedure flag, so that it can reject each attribute per kind of routine. While here, improve the error message for a rejected attribute. The old wording was useless in a context that shows no accurate error pointer. pg_dump did not read pg_proc.prosupport for aggregates, so an aggregate would silently lose its support function during dump and restore, and during pg_upgrade. Emit a separate ALTER FUNCTION command after CREATE AGGREGATE, since CREATE AGGREGATE has no SUPPORT clause. Author: Tom Lane Reported-by: Andrei Lepikhov Discussion: https://postgr.es/m/8f58c96d-d3c7-4c0f-9898-116f00eeaff6@gmail.com --- doc/src/sgml/ref/alter_aggregate.sgml | 7 ++ doc/src/sgml/ref/alter_function.sgml | 11 ++- doc/src/sgml/ref/create_aggregate.sgml | 3 +- doc/src/sgml/xfunc.sgml | 16 +++++ src/backend/commands/functioncmds.c | 68 ++++++++++++++----- .../regress/expected/create_procedure.out | 6 +- src/test/regress/expected/misc_functions.out | 68 +++++++++++++++++++ src/test/regress/regress.c | 40 +++++++++++ src/test/regress/sql/misc_functions.sql | 40 +++++++++++ 9 files changed, 236 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/ref/alter_aggregate.sgml b/doc/src/sgml/ref/alter_aggregate.sgml index d0a39ba7b5e..8fedd0e0595 100644 --- a/doc/src/sgml/ref/alter_aggregate.sgml +++ b/doc/src/sgml/ref/alter_aggregate.sgml @@ -139,6 +139,13 @@ ALTER AGGREGATE name ( aggregate_signatu Notes + + An aggregate can have a planner support function, but this command does + not set it. Use + ALTER FUNCTION + with the SUPPORT clause for that. + + The recommended syntax for referencing an ordered-set aggregate is to write ORDER BY between the direct and aggregated diff --git a/doc/src/sgml/ref/alter_function.sgml b/doc/src/sgml/ref/alter_function.sgml index 8193b17f255..c42c105f0a3 100644 --- a/doc/src/sgml/ref/alter_function.sgml +++ b/doc/src/sgml/ref/alter_function.sgml @@ -57,6 +57,14 @@ ALTER FUNCTION name [ ( [ [ CREATE + AGGREGATE. + + You must own the function to use ALTER FUNCTION. To change a function's schema, you must also have CREATE @@ -269,7 +277,8 @@ ALTER FUNCTION name [ ( [ [ name diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 99be010f89f..e0e7b0a9b06 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -4184,6 +4184,22 @@ supportfn(internal) returns internal the SUPPORT clause when creating the target function. + + An aggregate can also be a target function. Because an aggregate is + created by CREATE + AGGREGATE, which has no SUPPORT + clause, attach the support function with + ALTER FUNCTION + instead. + + + + An aggregate's support function is not preserved by + pg_dump, and therefore not + by pg_upgrade either. Set it again after a + restore, or arrange for the owning extension to do so. + + The details of the API for planner support functions can be found in file src/include/nodes/supportnodes.h in the diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index ba24a708982..ee778502b8a 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -516,7 +516,7 @@ interpret_function_parameter_list(ParseState *pstate, */ static bool compute_common_attribute(ParseState *pstate, - bool is_procedure, + char prokind, DefElem *defel, DefElem **volatility_item, DefElem **strict_item, @@ -528,8 +528,13 @@ compute_common_attribute(ParseState *pstate, DefElem **support_item, DefElem **parallel_item) { + bool is_aggregate = (prokind == PROKIND_AGGREGATE); + bool is_procedure = (prokind == PROKIND_PROCEDURE); + if (strcmp(defel->defname, "volatility") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*volatility_item) @@ -539,6 +544,8 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "strict") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*strict_item) @@ -548,6 +555,8 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "security") == 0) { + if (is_aggregate) + goto aggregate_error; if (*security_item) errorConflictingDefElem(defel, pstate); @@ -555,6 +564,8 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "leakproof") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*leakproof_item) @@ -564,10 +575,14 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "set") == 0) { + if (is_aggregate) + goto aggregate_error; *set_items = lappend(*set_items, defel->arg); } else if (strcmp(defel->defname, "cost") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*cost_item) @@ -577,6 +592,8 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "rows") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*rows_item) @@ -595,6 +612,8 @@ compute_common_attribute(ParseState *pstate, } else if (strcmp(defel->defname, "parallel") == 0) { + if (is_aggregate) + goto aggregate_error; if (is_procedure) goto procedure_error; if (*parallel_item) @@ -608,10 +627,19 @@ compute_common_attribute(ParseState *pstate, /* Recognized an option */ return true; +aggregate_error: + ereport(ERROR, + (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), + errmsg("attribute \"%s\" is not allowed for aggregates", + defel->defname), + parser_errposition(pstate, defel->location))); + return false; + procedure_error: ereport(ERROR, (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("invalid attribute in procedure definition"), + errmsg("attribute \"%s\" is not allowed for procedures", + defel->defname), parser_errposition(pstate, defel->location))); return false; } @@ -730,7 +758,7 @@ interpret_func_support(DefElem *defel) */ static void compute_function_attributes(ParseState *pstate, - bool is_procedure, + char prokind, List *options, List **as, char **language, @@ -787,15 +815,16 @@ compute_function_attributes(ParseState *pstate, { if (windowfunc_item) errorConflictingDefElem(defel, pstate); - if (is_procedure) + if (prokind == PROKIND_PROCEDURE) ereport(ERROR, (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), - errmsg("invalid attribute in procedure definition"), + errmsg("attribute \"%s\" is not allowed for procedures", + defel->defname), parser_errposition(pstate, defel->location))); windowfunc_item = defel; } else if (compute_common_attribute(pstate, - is_procedure, + prokind, defel, &volatility_item, &strict_item, @@ -1052,6 +1081,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt) List *trfoids_list = NIL; ArrayType *trftypes; Oid requiredResultType; + char prokind; bool isWindowFunc, isStrict, security, @@ -1079,6 +1109,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt) /* Set default attributes */ as_clause = NIL; language = NULL; + prokind = (stmt->is_procedure ? PROKIND_PROCEDURE : PROKIND_FUNCTION); isWindowFunc = false; isStrict = false; security = false; @@ -1092,7 +1123,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt) /* Extract non-default attributes from stmt->options list */ compute_function_attributes(pstate, - stmt->is_procedure, + prokind, stmt->options, &as_clause, &language, &transformDefElem, &isWindowFunc, &volatility, @@ -1100,6 +1131,16 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt) &proconfig, &procost, &prorows, &prosupport, ¶llel); + /* + * If we found a WINDOW attribute, change prokind accordingly. Note that + * as things stand, we must allow all the same attributes for window + * functions as plain functions, since we don't know the difference while + * scanning the options list. If that ever needs to change, we'd have to + * scan the list twice, the first time just to identify WINDOW. + */ + if (isWindowFunc) + prokind = PROKIND_WINDOW; + if (!language) { if (stmt->sql_body) @@ -1285,7 +1326,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt) prosrc_str, /* converted to text later */ probin_str, /* converted to text later */ prosqlbody, - stmt->is_procedure ? PROKIND_PROCEDURE : (isWindowFunc ? PROKIND_WINDOW : PROKIND_FUNCTION), + prokind, security, isLeakProof, isStrict, @@ -1366,7 +1407,6 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt) HeapTuple tup; Oid funcOid; Form_pg_proc procForm; - bool is_procedure; Relation rel; ListCell *l; DefElem *volatility_item = NULL; @@ -1397,21 +1437,13 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt) aclcheck_error(ACLCHECK_NOT_OWNER, stmt->objtype, NameListToString(stmt->func->objname)); - if (procForm->prokind == PROKIND_AGGREGATE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is an aggregate function", - NameListToString(stmt->func->objname)))); - - is_procedure = (procForm->prokind == PROKIND_PROCEDURE); - /* Examine requested actions. */ foreach(l, stmt->actions) { DefElem *defel = (DefElem *) lfirst(l); if (compute_common_attribute(pstate, - is_procedure, + procForm->prokind, defel, &volatility_item, &strict_item, diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out index f89042cf798..48170c008db 100644 --- a/src/test/regress/expected/create_procedure.out +++ b/src/test/regress/expected/create_procedure.out @@ -392,11 +392,11 @@ LINE 1: CALL sum(1); ^ HINT: To call a function, use SELECT. CREATE PROCEDURE ptestx() LANGUAGE SQL WINDOW AS $$ INSERT INTO cp_test VALUES (1, 'a') $$; -ERROR: invalid attribute in procedure definition +ERROR: attribute "window" is not allowed for procedures LINE 1: CREATE PROCEDURE ptestx() LANGUAGE SQL WINDOW AS $$ INSERT I... ^ CREATE PROCEDURE ptestx() LANGUAGE SQL STRICT AS $$ INSERT INTO cp_test VALUES (1, 'a') $$; -ERROR: invalid attribute in procedure definition +ERROR: attribute "strict" is not allowed for procedures LINE 1: CREATE PROCEDURE ptestx() LANGUAGE SQL STRICT AS $$ INSERT I... ^ CREATE PROCEDURE ptestx(a VARIADIC int[], b OUT int) LANGUAGE SQL @@ -410,7 +410,7 @@ ERROR: procedure OUT parameters cannot appear after one with a default value LINE 1: CREATE PROCEDURE ptestx(a int DEFAULT 42, b OUT int) LANGUAG... ^ ALTER PROCEDURE ptest1(text) STRICT; -ERROR: invalid attribute in procedure definition +ERROR: attribute "strict" is not allowed for procedures LINE 1: ALTER PROCEDURE ptest1(text) STRICT; ^ ALTER FUNCTION ptest1(text) VOLATILE; -- error: not a function diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index c3261bff209..5465f206640 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -681,6 +681,74 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); (2 rows) DROP FUNCTION foo_from_bar; +-- +-- Test adding a support function to an aggregate +-- +CREATE FUNCTION test_aggref_support(internal) + RETURNS internal + AS :'regresslib', 'test_aggref_support' + LANGUAGE C STRICT; +-- my_count() counts its input rows in the same way as count(any) does +CREATE AGGREGATE my_count("any") ( + SFUNC = int8inc_any, STYPE = int8, INITCOND = '0', PARALLEL = SAFE +); +-- SUPPORT is the only attribute an aggregate accepts +ALTER FUNCTION my_count("any") COST 10; +ERROR: attribute "cost" is not allowed for aggregates +LINE 1: ALTER FUNCTION my_count("any") COST 10; + ^ +-- Setting the support function records a dependency +ALTER FUNCTION my_count("any") SUPPORT test_aggref_support; +DROP FUNCTION test_aggref_support(internal); +ERROR: cannot drop function test_aggref_support(internal) because other objects depend on it +DETAIL: function my_count("any") depends on function test_aggref_support(internal) +HINT: Use DROP ... CASCADE to drop the dependent objects too. +-- Attach a support function to a built-in aggregate and check that the +-- planner calls it. +BEGIN; +ALTER FUNCTION pg_catalog.count("any") SUPPORT test_aggref_support; +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(a) FROM generate_series(1,10) AS a; + QUERY PLAN +----------------------------------------------------- + Aggregate + Output: my_count(a) + -> Function Scan on pg_catalog.generate_series a + Output: a + Function Call: generate_series(1, 10) +(5 rows) + +SELECT count(a) FROM generate_series(1,10) AS a; + count +------- + 10 +(1 row) + +-- count(*) is a different aggregate, and keeps its own support function +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM generate_series(1,10) AS a; + QUERY PLAN +----------------------------------------------------- + Aggregate + Output: count(*) + -> Function Scan on pg_catalog.generate_series a + Output: a + Function Call: generate_series(1, 10) +(5 rows) + +ROLLBACK; +-- After the rollback the built-in support function is in charge again +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(a) FROM generate_series(1,10) AS a; + QUERY PLAN +----------------------------------------------------- + Aggregate + Output: count(a) + -> Function Scan on pg_catalog.generate_series a + Output: a + Function Call: generate_series(1, 10) +(5 rows) + +ALTER FUNCTION my_count("any") SUPPORT int8inc_support; +-- Now we can delete this support function +DROP FUNCTION test_aggref_support(internal); -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); ok diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c index c2975ffc1b0..faaee610a9d 100644 --- a/src/test/regress/regress.c +++ b/src/test/regress/regress.c @@ -35,10 +35,12 @@ #include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/makefuncs.h" #include "nodes/supportnodes.h" #include "optimizer/optimizer.h" #include "optimizer/plancat.h" #include "parser/parse_coerce.h" +#include "parser/parse_func.h" #include "port/atomics.h" #include "portability/instr_time.h" #include "postmaster/postmaster.h" /* for MAX_BACKENDS */ @@ -822,6 +824,44 @@ test_support_func(PG_FUNCTION_ARGS) PG_RETURN_POINTER(ret); } +PG_FUNCTION_INFO_V1(test_aggref_support); +Datum +test_aggref_support(PG_FUNCTION_ARGS) +{ + Node *rawreq = (Node *) PG_GETARG_POINTER(0); + + if (IsA(rawreq, SupportRequestSimplifyAggref)) + { + /* + * Replace the target with my_count(), a user aggregate that counts + * its input rows in the same way as count(any) does. That's safe as + * long as we don't attach this to any other aggregate. + */ + SupportRequestSimplifyAggref *req; + Aggref *agg; + Aggref *newagg; + Oid argtypes[1] = {ANYOID}; + Oid newfnoid; + + req = (SupportRequestSimplifyAggref *) rawreq; + agg = req->aggref; + + newfnoid = LookupFuncName(list_make1(makeString("my_count")), + 1, argtypes, true); + if (!OidIsValid(newfnoid) || newfnoid == agg->aggfnoid) + PG_RETURN_POINTER(NULL); + + /* Build a new Aggref that names my_count, and change nothing else */ + newagg = makeNode(Aggref); + memcpy(newagg, agg, sizeof(Aggref)); + newagg->aggfnoid = newfnoid; + + PG_RETURN_POINTER(newagg); + } + + PG_RETURN_POINTER(NULL); +} + PG_FUNCTION_INFO_V1(test_inline_in_from_support_func); Datum test_inline_in_from_support_func(PG_FUNCTION_ARGS) diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 946ee5726cd..9386a388a1b 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -270,6 +270,46 @@ EXPLAIN (COSTS OFF) SELECT * FROM foo_from_bar('f1', 'text_tbl', 'doh!'); DROP FUNCTION foo_from_bar; +-- +-- Test adding a support function to an aggregate +-- + +CREATE FUNCTION test_aggref_support(internal) + RETURNS internal + AS :'regresslib', 'test_aggref_support' + LANGUAGE C STRICT; + +-- my_count() counts its input rows in the same way as count(any) does +CREATE AGGREGATE my_count("any") ( + SFUNC = int8inc_any, STYPE = int8, INITCOND = '0', PARALLEL = SAFE +); + +-- SUPPORT is the only attribute an aggregate accepts +ALTER FUNCTION my_count("any") COST 10; + +-- Setting the support function records a dependency +ALTER FUNCTION my_count("any") SUPPORT test_aggref_support; +DROP FUNCTION test_aggref_support(internal); + +-- Attach a support function to a built-in aggregate and check that the +-- planner calls it. +BEGIN; +ALTER FUNCTION pg_catalog.count("any") SUPPORT test_aggref_support; + +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(a) FROM generate_series(1,10) AS a; +SELECT count(a) FROM generate_series(1,10) AS a; + +-- count(*) is a different aggregate, and keeps its own support function +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM generate_series(1,10) AS a; +ROLLBACK; + +-- After the rollback the built-in support function is in charge again +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(a) FROM generate_series(1,10) AS a; + +ALTER FUNCTION my_count("any") SUPPORT int8inc_support; +-- Now we can delete this support function +DROP FUNCTION test_aggref_support(internal); + -- Test functions for control data SELECT count(*) > 0 AS ok FROM pg_control_checkpoint(); SELECT count(*) > 0 AS ok FROM pg_control_init(); -- 2.52.0