From bf3feaa70765f78a988cf14bd09eace362253fd3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 31 Mar 2025 18:10:29 -0400 Subject: [PATCH v11 7/7] Delay parse analysis and rewrite until we're ready to execute the query. This change fixes a longstanding bugaboo with SQL functions: you could not write DDL that would affect later statements in the same function. That's mostly still true with new-style SQL functions, since the results of parse analysis are baked into the stored query trees (and protected by dependency records). But for old-style SQL functions, it will now work much as it does with plpgsql functions. The key changes required are to (1) stash the parsetrees read from pg_proc somewhere safe until we're ready to process them, and (2) adjust the error context reporting. sql_compile_error_callback is now only useful for giving context for errors detected by raw parsing. Errors detected in either parse analysis or planning are handled by sql_exec_error_callback, as they were before this patch series. Author: Tom Lane Discussion: https://postgr.es/m/8216639.NyiUUSuA9g@aivenlaptop --- doc/src/sgml/xfunc.sgml | 15 - src/backend/executor/functions.c | 315 ++++++++++-------- .../regress/expected/create_function_sql.out | 42 ++- src/test/regress/expected/rangefuncs.out | 2 +- src/test/regress/sql/create_function_sql.sql | 25 ++ 5 files changed, 241 insertions(+), 158 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 35d34f224ef..8074f66417d 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -234,21 +234,6 @@ CALL clean_emp(); whereas returning void is a PostgreSQL extension. - - - The entire body of an SQL function is parsed before any of it is - executed. While an SQL function can contain commands that alter - the system catalogs (e.g., CREATE TABLE), the effects - of such commands will not be visible during parse analysis of - later commands in the function. Thus, for example, - CREATE TABLE foo (...); INSERT INTO foo VALUES(...); - will not work as desired if packaged up into a single SQL function, - since foo won't exist yet when the INSERT - command is parsed. It's recommended to use PL/pgSQL - instead of an SQL function in this type of situation. - - - The syntax of the CREATE FUNCTION command requires the function body to be written as a string constant. It is usually diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c index b5a9ecea637..83dbcad78ad 100644 --- a/src/backend/executor/functions.c +++ b/src/backend/executor/functions.c @@ -90,7 +90,13 @@ typedef struct execution_state * function is polymorphic or used as a trigger, there is a separate * SQLFunctionHashEntry for each usage, so that we need consider only one * set of relevant data types.) The struct itself is in memory managed by - * funccache.c, and its subsidiary data is kept in hcontext ("hash context"). + * funccache.c, and its subsidiary data is kept in one of two contexts: + * * pcontext ("parse context") holds the raw parse trees or Query trees + * that we read from the pg_proc row. These will be converted to + * CachedPlanSources as they are needed. Once the last one is converted, + * pcontext can be freed. + * * hcontext ("hash context") holds everything else belonging to the + * SQLFunctionHashEntry. * * 2. SQLFunctionCache lasts for the duration of a single execution of * the SQL function. (In "lazyEval" mode, this might span multiple calls of @@ -127,12 +133,14 @@ typedef struct SQLFunctionHashEntry TupleDesc rettupdesc; /* result tuple descriptor */ - List *plansource_list; /* CachedPlanSources for fn's queries */ + List *source_list; /* RawStmts or Queries read from pg_proc */ + int num_queries; /* original length of source_list */ + bool raw_source; /* true if source_list contains RawStmts */ - /* if positive, this is the index of the query we're parsing */ - int error_query_index; + List *plansource_list; /* CachedPlanSources for fn's queries */ - MemoryContext hcontext; /* memory context holding all above */ + MemoryContext pcontext; /* memory context holding source_list */ + MemoryContext hcontext; /* memory context holding all else */ } SQLFunctionHashEntry; typedef struct SQLFunctionCache @@ -149,7 +157,7 @@ typedef struct SQLFunctionCache JunkFilter *junkFilter; /* will be NULL if function returns VOID */ - /* if positive, this is the index of the query we're executing */ + /* if positive, this is the index of the query we're processing */ int error_query_index; /* @@ -193,6 +201,13 @@ static Node *sql_fn_make_param(SQLFunctionParseInfoPtr pinfo, static Node *sql_fn_resolve_param_name(SQLFunctionParseInfoPtr pinfo, const char *paramname, int location); static bool init_execution_state(SQLFunctionCachePtr fcache); +static void sql_compile_callback(FunctionCallInfo fcinfo, + HeapTuple procedureTuple, + const CachedFunctionHashKey *hashkey, + CachedFunction *cfunc, + bool forValidator); +static void prepare_next_query(SQLFunctionHashEntry *func); +static void sql_delete_callback(CachedFunction *cfunc); static void sql_postrewrite_callback(List *querytree_list, void *arg); static SQLFunctionCache *init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK); @@ -542,17 +557,25 @@ init_execution_state(SQLFunctionCachePtr fcache) fcache->eslist = NULL; /* - * Get the next CachedPlanSource, or stop if there are no more. + * Get the next CachedPlanSource, or stop if there are no more. We might + * need to create the next CachedPlanSource; if so, advance + * error_query_index first, so that errors detected in prepare_next_query + * are blamed on the right statement. */ if (fcache->next_query_index >= list_length(fcache->func->plansource_list)) - return false; + { + if (fcache->next_query_index >= fcache->func->num_queries) + return false; + fcache->error_query_index++; + prepare_next_query(fcache->func); + } + else + fcache->error_query_index++; + plansource = (CachedPlanSource *) list_nth(fcache->func->plansource_list, fcache->next_query_index); fcache->next_query_index++; - /* Count source queries for sql_exec_error_callback */ - fcache->error_query_index++; - /* * Generate plans for the query or queries within this CachedPlanSource. * Register the CachedPlan with the current resource owner. (Saving @@ -624,7 +647,7 @@ init_execution_state(SQLFunctionCachePtr fcache) * If this isn't the last CachedPlanSource, we're done here. Otherwise, * we need to prepare information about how to return the results. */ - if (fcache->next_query_index < list_length(fcache->func->plansource_list)) + if (fcache->next_query_index < fcache->func->num_queries) return true; /* @@ -718,7 +741,7 @@ init_execution_state(SQLFunctionCachePtr fcache) * * We expect to be called in a short-lived memory context (typically a * query's per-tuple context). Data that is to be part of the hash entry - * must be copied into the hcontext, or put into a CachedPlanSource. + * must be copied into the hcontext or pcontext as appropriate. */ static void sql_compile_callback(FunctionCallInfo fcinfo, @@ -731,18 +754,17 @@ sql_compile_callback(FunctionCallInfo fcinfo, Form_pg_proc procedureStruct = (Form_pg_proc) GETSTRUCT(procedureTuple); ErrorContextCallback comperrcontext; MemoryContext hcontext; + MemoryContext pcontext; MemoryContext oldcontext = CurrentMemoryContext; Oid rettype; TupleDesc rettupdesc; Datum tmp; bool isNull; - List *queryTree_list; - List *plansource_list; - ListCell *qlc; - ListCell *plc; + List *source_list; /* - * Setup error traceback support for ereport() during compile + * Setup error traceback support for ereport() during compile. (This is + * mainly useful for reporting parse errors from pg_parse_query.) */ comperrcontext.callback = sql_compile_error_callback; comperrcontext.arg = func; @@ -757,6 +779,15 @@ sql_compile_callback(FunctionCallInfo fcinfo, "SQL function", ALLOCSET_SMALL_SIZES); + /* + * Create the not-as-long-lived pcontext. We make this a child of + * hcontext so that it doesn't require separate deletion. + */ + pcontext = AllocSetContextCreate(hcontext, + "SQL function parse trees", + ALLOCSET_SMALL_SIZES); + func->pcontext = pcontext; + /* * copy function name immediately for use by error reporting callback, and * for use as memory context identifier @@ -815,96 +846,111 @@ sql_compile_callback(FunctionCallInfo fcinfo, procedureTuple, Anum_pg_proc_prosqlbody, &isNull); - - /* - * Now we must parse and rewrite the function's queries, and create - * CachedPlanSources. Note that we apply CreateCachedPlan[ForQuery] - * immediately so that it captures the original state of the parsetrees, - * but we don't do CompleteCachedPlan until after fixing up the final - * query's targetlist. - */ - queryTree_list = NIL; - plansource_list = NIL; if (!isNull) { /* Source queries are already parse-analyzed */ Node *n; - List *stored_query_list; - ListCell *lc; n = stringToNode(TextDatumGetCString(tmp)); if (IsA(n, List)) - stored_query_list = linitial_node(List, castNode(List, n)); + source_list = linitial_node(List, castNode(List, n)); else - stored_query_list = list_make1(n); + source_list = list_make1(n); + func->raw_source = false; + } + else + { + /* Source queries are raw parsetrees */ + source_list = pg_parse_query(func->src); + func->raw_source = true; + } - foreach(lc, stored_query_list) - { - Query *parsetree = lfirst_node(Query, lc); - CachedPlanSource *plansource; - List *queryTree_sublist; + /* + * Note: we must save the number of queries so that we'll still remember + * how many there are after we discard source_list. + */ + func->num_queries = list_length(source_list); + + /* Save the source trees in pcontext for now. */ + MemoryContextSwitchTo(pcontext); + func->source_list = copyObject(source_list); + MemoryContextSwitchTo(oldcontext); - /* Count source queries for sql_compile_error_callback */ - func->error_query_index++; + /* + * We now have a fully valid hash entry, so reparent hcontext under + * CacheMemoryContext to make all the subsidiary data long-lived, and only + * then install the hcontext link so that sql_delete_callback will know to + * delete it. + */ + MemoryContextSetParent(hcontext, CacheMemoryContext); + func->hcontext = hcontext; - plansource = CreateCachedPlanForQuery(parsetree, - func->src, - CreateCommandTag((Node *) parsetree)); - plansource_list = lappend(plansource_list, plansource); + error_context_stack = comperrcontext.previous; +} - AcquireRewriteLocks(parsetree, true, false); - queryTree_sublist = pg_rewrite_query(parsetree); - queryTree_list = lappend(queryTree_list, queryTree_sublist); - } +/* + * Convert the SQL function's next query from source form (RawStmt or Query) + * into a CachedPlanSource. If it's the last query, also determine whether + * the function returnsTuple. + */ +static void +prepare_next_query(SQLFunctionHashEntry *func) +{ + int qindex; + bool islast; + CachedPlanSource *plansource; + List *queryTree_list; + MemoryContext oldcontext; + + /* Which query should we process? */ + qindex = list_length(func->plansource_list); + Assert(qindex < func->num_queries); /* else caller error */ + islast = (qindex + 1 >= func->num_queries); + + /* + * Parse and/or rewrite the query, creating a CachedPlanSource that holds + * a copy of the original parsetree. + */ + if (!func->raw_source) + { + /* Source queries are already parse-analyzed */ + Query *parsetree = list_nth_node(Query, func->source_list, qindex); + + plansource = CreateCachedPlanForQuery(parsetree, + func->src, + CreateCommandTag((Node *) parsetree)); + AcquireRewriteLocks(parsetree, true, false); + queryTree_list = pg_rewrite_query(parsetree); } else { /* Source queries are raw parsetrees */ - List *raw_parsetree_list; - ListCell *lc; - - raw_parsetree_list = pg_parse_query(func->src); - - foreach(lc, raw_parsetree_list) - { - RawStmt *parsetree = lfirst_node(RawStmt, lc); - CachedPlanSource *plansource; - List *queryTree_sublist; - - /* Count source queries for sql_compile_error_callback */ - func->error_query_index++; - - plansource = CreateCachedPlan(parsetree, - func->src, - CreateCommandTag(parsetree->stmt)); - plansource_list = lappend(plansource_list, plansource); - - queryTree_sublist = pg_analyze_and_rewrite_withcb(parsetree, - func->src, - (ParserSetupHook) sql_fn_parser_setup, - func->pinfo, - NULL); - queryTree_list = lappend(queryTree_list, queryTree_sublist); - } + RawStmt *parsetree = list_nth_node(RawStmt, func->source_list, qindex); + + plansource = CreateCachedPlan(parsetree, + func->src, + CreateCommandTag(parsetree->stmt)); + queryTree_list = pg_analyze_and_rewrite_withcb(parsetree, + func->src, + (ParserSetupHook) sql_fn_parser_setup, + func->pinfo, + NULL); } - /* Failures below here are reported as "during startup" */ - func->error_query_index = 0; - /* * Check that there are no statements we don't want to allow. */ - check_sql_fn_statements(queryTree_list); + check_sql_fn_statement(queryTree_list); /* - * Check that the function returns the type it claims to. Although in - * simple cases this was already done when the function was defined, we - * have to recheck because database objects used in the function's queries - * might have changed type. We'd have to recheck anyway if the function - * had any polymorphic arguments. Moreover, check_sql_fn_retval takes - * care of injecting any required column type coercions. (But we don't - * ask it to insert nulls for dropped columns; the junkfilter handles - * that.) + * If this is the last query, check that the function returns the type it + * claims to. Although in simple cases this was already done when the + * function was defined, we have to recheck because database objects used + * in the function's queries might have changed type. We'd have to + * recheck anyway if the function had any polymorphic arguments. Moreover, + * check_sql_stmt_retval takes care of injecting any required column type + * coercions. (But we don't ask it to insert nulls for dropped columns; + * the junkfilter handles that.) * * Note: we set func->returnsTuple according to whether we are returning * the whole tuple result or just a single column. In the latter case we @@ -914,69 +960,60 @@ sql_compile_callback(FunctionCallInfo fcinfo, * the rowtype column into multiple columns, since we have no way to * notify the caller that it should do that.) */ - func->returnsTuple = check_sql_fn_retval(queryTree_list, - rettype, - rettupdesc, - procedureStruct->prokind, - false); + if (islast) + func->returnsTuple = check_sql_stmt_retval(queryTree_list, + func->rettype, + func->rettupdesc, + func->prokind, + false); /* - * Now that check_sql_fn_retval has done its thing, we can complete plan + * Now that check_sql_stmt_retval has done its thing, we can complete plan * cache entry creation. */ - forboth(qlc, queryTree_list, plc, plansource_list) - { - List *queryTree_sublist = lfirst(qlc); - CachedPlanSource *plansource = lfirst(plc); - bool islast; - - /* Finish filling in the CachedPlanSource */ - CompleteCachedPlan(plansource, - queryTree_sublist, - NULL, - NULL, - 0, - (ParserSetupHook) sql_fn_parser_setup, - func->pinfo, - CURSOR_OPT_PARALLEL_OK | CURSOR_OPT_NO_SCROLL, - false); + CompleteCachedPlan(plansource, + queryTree_list, + NULL, + NULL, + 0, + (ParserSetupHook) sql_fn_parser_setup, + func->pinfo, + CURSOR_OPT_PARALLEL_OK | CURSOR_OPT_NO_SCROLL, + false); - /* - * Install post-rewrite hook. Its arg is the hash entry if this is - * the last statement, else NULL. - */ - islast = (lnext(queryTree_list, qlc) == NULL); - SetPostRewriteHook(plansource, - sql_postrewrite_callback, - islast ? func : NULL); - } + /* + * Install post-rewrite hook. Its arg is the hash entry if this is the + * last statement, else NULL. + */ + SetPostRewriteHook(plansource, + sql_postrewrite_callback, + islast ? func : NULL); /* * While the CachedPlanSources can take care of themselves, our List * pointing to them had better be in the hcontext. */ - MemoryContextSwitchTo(hcontext); - plansource_list = list_copy(plansource_list); + oldcontext = MemoryContextSwitchTo(func->hcontext); + func->plansource_list = lappend(func->plansource_list, plansource); MemoryContextSwitchTo(oldcontext); /* - * We have now completed building the hash entry, so reparent stuff under - * CacheMemoryContext to make all the subsidiary data long-lived. - * Importantly, this part can't fail partway through. + * As soon as we've linked the CachedPlanSource into the list, mark it as + * "saved". */ - foreach(plc, plansource_list) - { - CachedPlanSource *plansource = lfirst(plc); + SaveCachedPlan(plansource); - SaveCachedPlan(plansource); + /* + * Finally, if this was the last statement, we can flush the pcontext with + * the original query trees; they're all safely copied into + * CachedPlanSources now. + */ + if (islast) + { + func->source_list = NIL; /* avoid dangling pointer */ + MemoryContextDelete(func->pcontext); + func->pcontext = NULL; } - MemoryContextSetParent(hcontext, CacheMemoryContext); - - /* And finally, arm sql_delete_callback to delete the stuff again */ - func->plansource_list = plansource_list; - func->hcontext = hcontext; - - error_context_stack = comperrcontext.previous; } /* Deletion callback used by funccache.c */ @@ -997,7 +1034,7 @@ sql_delete_callback(CachedFunction *cfunc) /* * If we have an hcontext, free it, thereby getting rid of all subsidiary - * data. + * data. (If we still have a pcontext, this gets rid of that too.) */ if (func->hcontext) MemoryContextDelete(func->hcontext); @@ -1016,7 +1053,7 @@ sql_postrewrite_callback(List *querytree_list, void *arg) check_sql_fn_statement(querytree_list); /* - * If this is the last query, we must re-do what check_sql_fn_retval did + * If this is the last query, we must re-do what check_sql_stmt_retval did * to its targetlist. Also check that returnsTuple didn't change (it * probably cannot, but be cautious). */ @@ -1749,14 +1786,10 @@ sql_compile_error_callback(void *arg) } /* - * If we failed while parsing an identifiable query within the function, - * report that. Otherwise say it was "during startup". + * sql_compile_callback() doesn't do any per-query processing, so just + * report the context as "during startup". */ - if (func->error_query_index > 0) - errcontext("SQL function \"%s\" statement %d", - func->fname, func->error_query_index); - else - errcontext("SQL function \"%s\" during startup", func->fname); + errcontext("SQL function \"%s\" during startup", func->fname); } /* diff --git a/src/test/regress/expected/create_function_sql.out b/src/test/regress/expected/create_function_sql.out index 70ed5742b65..2ee7631044e 100644 --- a/src/test/regress/expected/create_function_sql.out +++ b/src/test/regress/expected/create_function_sql.out @@ -680,6 +680,43 @@ SELECT * FROM voidtest5(3); ----------- (0 rows) +-- DDL within a SQL function can now affect later statements in the function; +-- though that doesn't work if check_function_bodies is on. +SET check_function_bodies TO off; +CREATE FUNCTION create_and_insert() RETURNS VOID LANGUAGE sql AS $$ + create table ddl_test (f1 int); + insert into ddl_test values (1.2); +$$; +SELECT create_and_insert(); + create_and_insert +------------------- + +(1 row) + +TABLE ddl_test; + f1 +---- + 1 +(1 row) + +CREATE FUNCTION alter_and_insert() RETURNS VOID LANGUAGE sql AS $$ + alter table ddl_test alter column f1 type numeric; + insert into ddl_test values (1.2); +$$; +SELECT alter_and_insert(); + alter_and_insert +------------------ + +(1 row) + +TABLE ddl_test; + f1 +----- + 1 + 1.2 +(2 rows) + +RESET check_function_bodies; -- Regression tests for bugs: -- Check that arguments that are R/W expanded datums aren't corrupted by -- multiple uses. This test knows that array_append() returns a R/W datum @@ -722,7 +759,7 @@ CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL ERROR: only one AS item needed for language "sql" -- Cleanup DROP SCHEMA temp_func_test CASCADE; -NOTICE: drop cascades to 31 other objects +NOTICE: drop cascades to 34 other objects DETAIL: drop cascades to function functest_a_1(text,date) drop cascades to function functest_a_2(text[]) drop cascades to function functest_a_3() @@ -753,6 +790,9 @@ drop cascades to function voidtest2(integer,integer) drop cascades to function voidtest3(integer) drop cascades to function voidtest4(integer) drop cascades to function voidtest5(integer) +drop cascades to function create_and_insert() +drop cascades to table ddl_test +drop cascades to function alter_and_insert() drop cascades to function double_append(anyarray,anyelement) DROP USER regress_unpriv_user; RESET search_path; diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out index 397a8b35d6d..c21be83aa4a 100644 --- a/src/test/regress/expected/rangefuncs.out +++ b/src/test/regress/expected/rangefuncs.out @@ -1885,7 +1885,7 @@ select * from array_to_set(array['one', 'two']) as t(f1 numeric(4,2),f2 text); select * from array_to_set(array['one', 'two']) as t(f1 point,f2 text); ERROR: return type mismatch in function declared to return record DETAIL: Final statement returns integer instead of point at column 1. -CONTEXT: SQL function "array_to_set" during startup +CONTEXT: SQL function "array_to_set" statement 1 -- with "strict", this function can't be inlined in FROM explain (verbose, costs off) select * from array_to_set(array['one', 'two']) as t(f1 numeric(4,2),f2 text); diff --git a/src/test/regress/sql/create_function_sql.sql b/src/test/regress/sql/create_function_sql.sql index 1dd3c4a4e5f..68776be4c8d 100644 --- a/src/test/regress/sql/create_function_sql.sql +++ b/src/test/regress/sql/create_function_sql.sql @@ -394,6 +394,31 @@ CREATE FUNCTION voidtest5(a int) RETURNS SETOF VOID LANGUAGE SQL AS $$ SELECT generate_series(1, a) $$ STABLE; SELECT * FROM voidtest5(3); +-- DDL within a SQL function can now affect later statements in the function; +-- though that doesn't work if check_function_bodies is on. + +SET check_function_bodies TO off; + +CREATE FUNCTION create_and_insert() RETURNS VOID LANGUAGE sql AS $$ + create table ddl_test (f1 int); + insert into ddl_test values (1.2); +$$; + +SELECT create_and_insert(); + +TABLE ddl_test; + +CREATE FUNCTION alter_and_insert() RETURNS VOID LANGUAGE sql AS $$ + alter table ddl_test alter column f1 type numeric; + insert into ddl_test values (1.2); +$$; + +SELECT alter_and_insert(); + +TABLE ddl_test; + +RESET check_function_bodies; + -- Regression tests for bugs: -- Check that arguments that are R/W expanded datums aren't corrupted by -- 2.43.5