From 1111b2668d89bfcb6f502789158b1233ab4217a6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 15 Apr 2021 17:17:20 -0400 Subject: [PATCH] Undo decision to allow pg_proc.prosrc to be NULL. Commit e717a9a18 changed the longstanding rule that prosrc is NOT NULL because when a SQL-language function is written in SQL-standard style, we don't currently have anything useful to put there. This seems a poor decision though, as it could easily have negative impacts on external PLs (opening them to crashes they didn't use to have, for instance). SQL-function-related code can just as easily test "is prosqlbody not null" as "is prosrc null", so there's no real gain there either. Hence, revert the NOT NULL marking removal and adjust related logic. For now, we just put an empty string into prosrc for SQL-standard functions. Maybe we'll have a better idea later, although the history of things like pg_attrdef.adsrc suggests that it's not easy to maintain a string equivalent of a node tree. This also adds an assertion that queryDesc->sourceText != NULL to standard_ExecutorStart. We'd been silently relying on that for awhile, so let's make it less silent. Also fix some overlooked documentation and test cases. Discussion: https://postgr.es/m/2197698.1617984583@sss.pgh.pa.us --- doc/src/sgml/catalogs.sgml | 16 +++-- src/backend/catalog/pg_proc.c | 31 ++++----- src/backend/commands/functioncmds.c | 11 +++- src/backend/executor/execMain.c | 2 + src/backend/executor/functions.c | 20 +++--- src/backend/optimizer/util/clauses.c | 66 +++++++++---------- src/bin/pg_dump/pg_dump.c | 2 +- src/bin/psql/describe.c | 2 +- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.h | 2 +- src/include/executor/functions.h | 5 +- .../regress/expected/create_function_3.out | 6 ++ src/test/regress/expected/opr_sanity.out | 11 +++- src/test/regress/sql/create_function_3.sql | 5 ++ src/test/regress/sql/opr_sanity.sql | 7 +- 15 files changed, 109 insertions(+), 79 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 2656786d1e..1345791e96 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -6007,8 +6007,9 @@ SCRAM-SHA-256$<iteration count>:&l prosqlbody pg_node_tree - Pre-parsed SQL function body. This will be used for language SQL - functions if the body is not specified as a string constant. + Pre-parsed SQL function body. This is used for SQL-language + functions when the body is given in SQL-standard notation + rather than as a string literal. It's null in other cases. @@ -6036,9 +6037,16 @@ SCRAM-SHA-256$<iteration count>:&l For compiled functions, both built-in and dynamically loaded, prosrc contains the function's C-language - name (link symbol). For all other currently-known language types, + name (link symbol). + For SQL-language functions, prosrc contains + the function's source text if that is specified as a string literal; + but if the function body is specified in SQL-standard style, + prosrc is unused (typically it's an empty + string) and prosqlbody contains the + pre-parsed definition. + For all other currently-known language types, prosrc contains the function's source - text. probin is unused except for + text. probin is null except for dynamically-loaded C functions, for which it gives the name of the shared library file containing the function. diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c index cd13e63852..478dbde3fe 100644 --- a/src/backend/catalog/pg_proc.c +++ b/src/backend/catalog/pg_proc.c @@ -121,7 +121,7 @@ ProcedureCreate(const char *procedureName, /* * sanity checks */ - Assert(PointerIsValid(prosrc) || PointerIsValid(prosqlbody)); + Assert(PointerIsValid(prosrc)); parameterCount = parameterTypes->dim1; if (parameterCount < 0 || parameterCount > FUNC_MAX_ARGS) @@ -336,10 +336,7 @@ ProcedureCreate(const char *procedureName, values[Anum_pg_proc_protrftypes - 1] = trftypes; else nulls[Anum_pg_proc_protrftypes - 1] = true; - if (prosrc) - values[Anum_pg_proc_prosrc - 1] = CStringGetTextDatum(prosrc); - else - nulls[Anum_pg_proc_prosrc - 1] = true; + values[Anum_pg_proc_prosrc - 1] = CStringGetTextDatum(prosrc); if (probin) values[Anum_pg_proc_probin - 1] = CStringGetTextDatum(probin); else @@ -874,26 +871,29 @@ fmgr_sql_validator(PG_FUNCTION_ARGS) /* Postpone body checks if !check_function_bodies */ if (check_function_bodies) { + tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull); + if (isnull) + elog(ERROR, "null prosrc"); + + prosrc = TextDatumGetCString(tmp); + /* * Setup error traceback support for ereport(). */ callback_arg.proname = NameStr(proc->proname); - callback_arg.prosrc = NULL; + callback_arg.prosrc = prosrc; sqlerrcontext.callback = sql_function_parse_error_callback; sqlerrcontext.arg = (void *) &callback_arg; sqlerrcontext.previous = error_context_stack; error_context_stack = &sqlerrcontext; - tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull); - if (isnull) + /* If we have prosqlbody, pay attention to that not prosrc */ + tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosqlbody, &isnull); + if (!isnull) { Node *n; - tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosqlbody, &isnull); - if (isnull) - elog(ERROR, "null prosrc and prosqlbody"); - n = stringToNode(TextDatumGetCString(tmp)); if (IsA(n, List)) querytree_list = castNode(List, n); @@ -902,10 +902,6 @@ fmgr_sql_validator(PG_FUNCTION_ARGS) } else { - prosrc = TextDatumGetCString(tmp); - - callback_arg.prosrc = prosrc; - /* * We can't do full prechecking of the function definition if there * are any polymorphic input types, because actual datatypes of @@ -1001,9 +997,6 @@ function_parse_error_transpose(const char *prosrc) int newerrposition; const char *queryText; - if (!prosrc) - return false; - /* * Nothing to do unless we are dealing with a syntax error that has a * cursor position. diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 199029b7a8..dc317c83af 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -958,8 +958,17 @@ interpret_AS_clause(Oid languageOid, const char *languageName, *sql_body_out = (Node *) q; } + /* + * We must put something in prosrc. For the moment, just record an + * empty string. It might be useful to store the original text of the + * CREATE FUNCTION statement --- but to make actual use of that in + * error reports, we'd also have to adjust readfuncs.c to not throw + * away node location fields when reading prosqlbody. + */ + *prosrc_str_p = pstrdup(""); + + /* But we definitely don't need probin. */ *probin_str_p = NULL; - *prosrc_str_p = NULL; } else { diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index b2e2df8773..2cf6dad768 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -195,6 +195,8 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags) palloc0(nParamExec * sizeof(ParamExecData)); } + /* We now require all callers to provide sourceText */ + Assert(queryDesc->sourceText != NULL); estate->es_sourceText = queryDesc->sourceText; /* diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c index 642683843e..39580f7d57 100644 --- a/src/backend/executor/functions.c +++ b/src/backend/executor/functions.c @@ -667,6 +667,15 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK) procedureTuple, Anum_pg_proc_prosrc, &isNull); + if (isNull) + elog(ERROR, "null prosrc for function %u", foid); + fcache->src = TextDatumGetCString(tmp); + + /* If we have prosqlbody, pay attention to that not prosrc. */ + tmp = SysCacheGetAttr(PROCOID, + procedureTuple, + Anum_pg_proc_prosqlbody, + &isNull); /* * Parse and rewrite the queries in the function text. Use sublists to @@ -678,18 +687,11 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK) * plancache.c. */ queryTree_list = NIL; - if (isNull) + if (!isNull) { Node *n; List *stored_query_list; - tmp = SysCacheGetAttr(PROCOID, - procedureTuple, - Anum_pg_proc_prosqlbody, - &isNull); - if (isNull) - elog(ERROR, "null prosrc and prosqlbody for function %u", foid); - n = stringToNode(TextDatumGetCString(tmp)); if (IsA(n, List)) stored_query_list = linitial_node(List, castNode(List, n)); @@ -710,8 +712,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK) { List *raw_parsetree_list; - fcache->src = TextDatumGetCString(tmp); - raw_parsetree_list = pg_parse_query(fcache->src); foreach(lc, raw_parsetree_list) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 526997327c..d9ad4efc5e 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -4317,32 +4317,37 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); + /* Fetch the function body */ + tmp = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosrc, + &isNull); + if (isNull) + elog(ERROR, "null prosrc for function %u", funcid); + src = TextDatumGetCString(tmp); + /* * Setup error traceback support for ereport(). This is so that we can * finger the function that bad information came from. */ callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = NULL; + callback_arg.prosrc = src; sqlerrcontext.callback = sql_inline_error_callback; sqlerrcontext.arg = (void *) &callback_arg; sqlerrcontext.previous = error_context_stack; error_context_stack = &sqlerrcontext; - /* Fetch the function body */ + /* If we have prosqlbody, pay attention to that not prosrc */ tmp = SysCacheGetAttr(PROCOID, func_tuple, - Anum_pg_proc_prosrc, + Anum_pg_proc_prosqlbody, &isNull); - if (isNull) + if (!isNull) { Node *n; List *querytree_list; - tmp = SysCacheGetAttr(PROCOID, func_tuple, Anum_pg_proc_prosqlbody, &isNull); - if (isNull) - elog(ERROR, "null prosrc and prosqlbody for function %u", funcid); - n = stringToNode(TextDatumGetCString(tmp)); if (IsA(n, List)) querytree_list = linitial_node(List, castNode(List, n)); @@ -4354,10 +4359,6 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, } else { - src = TextDatumGetCString(tmp); - - callback_arg.prosrc = src; - /* * Set up to handle parameters while parsing the function body. We need a * dummy FuncExpr node containing the already-simplified arguments to pass @@ -4658,15 +4659,12 @@ sql_inline_error_callback(void *arg) int syntaxerrposition; /* If it's a syntax error, convert to internal syntax error report */ - if (callback_arg->prosrc) + syntaxerrposition = geterrposition(); + if (syntaxerrposition > 0) { - syntaxerrposition = geterrposition(); - if (syntaxerrposition > 0) - { - errposition(0); - internalerrposition(syntaxerrposition); - internalerrquery(callback_arg->prosrc); - } + errposition(0); + internalerrposition(syntaxerrposition); + internalerrquery(callback_arg->prosrc); } errcontext("SQL function \"%s\" during inlining", callback_arg->proname); @@ -4778,6 +4776,7 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) Oid func_oid; HeapTuple func_tuple; Form_pg_proc funcform; + char *src; Datum tmp; bool isNull; MemoryContext oldcxt; @@ -4886,31 +4885,36 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) ALLOCSET_DEFAULT_SIZES); oldcxt = MemoryContextSwitchTo(mycxt); + /* Fetch the function body */ + tmp = SysCacheGetAttr(PROCOID, + func_tuple, + Anum_pg_proc_prosrc, + &isNull); + if (isNull) + elog(ERROR, "null prosrc for function %u", func_oid); + src = TextDatumGetCString(tmp); + /* * Setup error traceback support for ereport(). This is so that we can * finger the function that bad information came from. */ callback_arg.proname = NameStr(funcform->proname); - callback_arg.prosrc = NULL; + callback_arg.prosrc = src; sqlerrcontext.callback = sql_inline_error_callback; sqlerrcontext.arg = (void *) &callback_arg; sqlerrcontext.previous = error_context_stack; error_context_stack = &sqlerrcontext; - /* Fetch the function body */ + /* If we have prosqlbody, pay attention to that not prosrc */ tmp = SysCacheGetAttr(PROCOID, func_tuple, - Anum_pg_proc_prosrc, + Anum_pg_proc_prosqlbody, &isNull); - if (isNull) + if (!isNull) { Node *n; - tmp = SysCacheGetAttr(PROCOID, func_tuple, Anum_pg_proc_prosqlbody, &isNull); - if (isNull) - elog(ERROR, "null prosrc and prosqlbody for function %u", func_oid); - n = stringToNode(TextDatumGetCString(tmp)); if (IsA(n, List)) querytree_list = linitial_node(List, castNode(List, n)); @@ -4927,12 +4931,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte) } else { - char *src; - - src = TextDatumGetCString(tmp); - - callback_arg.prosrc = src; - /* * Set up to handle parameters while parsing the function body. We can * use the FuncExpr just created as the input for diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 391947340f..e397b76356 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -12247,7 +12247,7 @@ dumpFunc(Archive *fout, const FuncInfo *finfo) if (fout->remoteVersion >= 140000) appendPQExpBufferStr(query, - "CASE WHEN prosrc IS NULL AND lanname = 'sql' THEN pg_get_function_sqlbody(p.oid) END AS prosqlbody\n"); + "pg_get_function_sqlbody(p.oid) AS prosqlbody\n"); else appendPQExpBufferStr(query, "NULL AS prosqlbody\n"); diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index fdc2a89085..400b683859 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -512,7 +512,7 @@ describeFunctions(const char *functypes, const char *func_pattern, gettext_noop("Language")); if (pset.sversion >= 140000) appendPQExpBuffer(&buf, - ",\n COALESCE(p.prosrc, pg_catalog.pg_get_function_sqlbody(p.oid)) as \"%s\"", + ",\n COALESCE(pg_catalog.pg_get_function_sqlbody(p.oid), p.prosrc) as \"%s\"", gettext_noop("Source code")); else appendPQExpBuffer(&buf, diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 45722fdbb1..87e9596da5 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202104081 +#define CATALOG_VERSION_NO 202104151 #endif diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index 8d58067d03..448d9898cb 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -112,7 +112,7 @@ CATALOG(pg_proc,1255,ProcedureRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81,Proce Oid protrftypes[1] BKI_DEFAULT(_null_) BKI_LOOKUP(pg_type); /* procedure source text */ - text prosrc; + text prosrc BKI_FORCE_NOT_NULL; /* secondary procedure info (can be NULL) */ text probin BKI_DEFAULT(_null_); diff --git a/src/include/executor/functions.h b/src/include/executor/functions.h index dcb8e18437..b56ce26da0 100644 --- a/src/include/executor/functions.h +++ b/src/include/executor/functions.h @@ -17,9 +17,6 @@ #include "nodes/execnodes.h" #include "tcop/dest.h" -/* This struct is known only within executor/functions.c */ -typedef struct SQLFunctionParseInfo *SQLFunctionParseInfoPtr; - /* * Data structure needed by the parser callback hooks to resolve parameter * references during parsing of a SQL function's body. This is separate from @@ -35,6 +32,8 @@ typedef struct SQLFunctionParseInfo Oid collation; /* function's input collation, if known */ } SQLFunctionParseInfo; +typedef SQLFunctionParseInfo *SQLFunctionParseInfoPtr; + extern Datum fmgr_sql(PG_FUNCTION_ARGS); extern SQLFunctionParseInfoPtr prepare_sql_fn_parse_info(HeapTuple procedureTuple, diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out index 94ff7095e7..ce48089012 100644 --- a/src/test/regress/expected/create_function_3.out +++ b/src/test/regress/expected/create_function_3.out @@ -290,6 +290,12 @@ CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement LANGUAGE SQL RETURN x[1]; ERROR: SQL function with unquoted function body cannot have polymorphic arguments +-- check reporting of parse-analysis errors +CREATE FUNCTION functest_S_xx(x date) RETURNS boolean + LANGUAGE SQL + RETURN x > 1; +ERROR: operator does not exist: date > integer +HINT: No operator matches the given name and argument types. You might need to add explicit type casts. -- tricky parsing CREATE FUNCTION functest_S_15(x int) RETURNS boolean LANGUAGE SQL diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out index fa26bf7610..7a0d345b60 100644 --- a/src/test/regress/expected/opr_sanity.out +++ b/src/test/regress/expected/opr_sanity.out @@ -91,10 +91,17 @@ WHERE p1.prolang = 0 OR p1.prorettype = 0 OR -----+--------- (0 rows) --- prosrc should never be null or empty +-- prosrc should never be null; it can be empty only if prosqlbody isn't null SELECT p1.oid, p1.proname FROM pg_proc as p1 -WHERE prosrc IS NULL OR prosrc = '' OR prosrc = '-'; +WHERE prosrc IS NULL; + oid | proname +-----+--------- +(0 rows) + +SELECT p1.oid, p1.proname +FROM pg_proc as p1 +WHERE (prosrc = '' OR prosrc = '-') AND prosqlbody IS NULL; oid | proname -----+--------- (0 rows) diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql index 592a43b5ed..4b778999ed 100644 --- a/src/test/regress/sql/create_function_3.sql +++ b/src/test/regress/sql/create_function_3.sql @@ -191,6 +191,11 @@ CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement LANGUAGE SQL RETURN x[1]; +-- check reporting of parse-analysis errors +CREATE FUNCTION functest_S_xx(x date) RETURNS boolean + LANGUAGE SQL + RETURN x > 1; + -- tricky parsing CREATE FUNCTION functest_S_15(x int) RETURNS boolean LANGUAGE SQL diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql index 0469174598..393acdf8c3 100644 --- a/src/test/regress/sql/opr_sanity.sql +++ b/src/test/regress/sql/opr_sanity.sql @@ -96,10 +96,13 @@ WHERE p1.prolang = 0 OR p1.prorettype = 0 OR provolatile NOT IN ('i', 's', 'v') OR proparallel NOT IN ('s', 'r', 'u'); --- prosrc should never be null or empty +-- prosrc should never be null; it can be empty only if prosqlbody isn't null SELECT p1.oid, p1.proname FROM pg_proc as p1 -WHERE prosrc IS NULL OR prosrc = '' OR prosrc = '-'; +WHERE prosrc IS NULL; +SELECT p1.oid, p1.proname +FROM pg_proc as p1 +WHERE (prosrc = '' OR prosrc = '-') AND prosqlbody IS NULL; -- proretset should only be set for normal functions SELECT p1.oid, p1.proname -- 2.39.5