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

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2656786d1e6dc59e48650fa997e41537449bf918..1345791e96381f802821bfb378db587a6f697732 100644 (file)
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6007,8 +6007,9 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <structfield>prosqlbody</structfield> <type>pg_node_tree</type>
       </para>
       <para>
-       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.
        </para></entry>
      </row>
 
@@ -6036,9 +6037,16 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
   <para>
    For compiled functions, both built-in and dynamically loaded,
    <structfield>prosrc</structfield> contains the function's C-language
-   name (link symbol).  For all other currently-known language types,
+   name (link symbol).
+   For SQL-language functions, <structfield>prosrc</structfield> 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,
+   <structfield>prosrc</structfield> is unused (typically it's an empty
+   string) and <structfield>prosqlbody</structfield> contains the
+   pre-parsed definition.
+   For all other currently-known language types,
    <structfield>prosrc</structfield> contains the function's source
-   text.  <structfield>probin</structfield> is unused except for
+   text.  <structfield>probin</structfield> is null except for
    dynamically-loaded C functions, for which it gives the name of the
    shared library file containing the function.
   </para>

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index cd13e63852a940bba1f213eab4bcdfc5c64ca38c..478dbde3fe67d52a877ec5005cf51d519ffea121 100644 (file)
--- 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 199029b7a8518620d5d422f20d6a8059d16fa4b1..dc317c83afc36487168192eebc4a898c0aba41f6 100644 (file)
--- 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 b2e2df877331248977624fe9329d148942940d03..2cf6dad7685817c40a0378b41f636bf8a7ce7b09 100644 (file)
--- 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 642683843ed41ab569fdd6bcd461559194f1a73c..39580f7d5776aa42b1147f2af794e66c28d08b76 100644 (file)
--- 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 526997327c6e18a5c714f364e26e742042db5ebf..d9ad4efc5eac54d127577f69bed69d50839d9b72 100644 (file)
--- 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 391947340f4d2a51b2b946195494d3391395778a..e397b7635680c846dc1297fd41b93bfbdf6f016e 100644 (file)
--- 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 fdc2a89085a0c42bd5183b1a705e3a4992330f54..400b683859c35f6797006d2d2c5893a1ea0bf763 100644 (file)
--- 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 45722fdbb139e09109f13e2dfedf5f89418bb358..87e9596da5691a2cbd181416da914b45d19d9d2c 100644 (file)
--- 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 8d58067d0344e613f9b513b223290b5a7a190d2a..448d9898cb31f91817a0414e30315c5263b58ddd 100644 (file)
--- 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 dcb8e18437fe10a2566fee4673502b792109de98..b56ce26da07f9173635597dff5d736560420d523 100644 (file)
--- 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 94ff7095e7d0bd193a7028e03665b6e8f1e6704b..ce480890127a68a5cf9509f242d9b9320489d19d 100644 (file)
--- 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 fa26bf761046a2e24d99758d2d26ef383a4e03ac..7a0d345b608cf281bc4106521ad5d304bda5a0b8 100644 (file)
--- 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 592a43b5ed2eef169481f3a8300fbffe000a79e0..4b778999ed7fc93cd200ace3c2e13da400ed4a5b 100644 (file)
--- 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 04691745981f9addae46b1ee1379523d9f09a961..393acdf8c3cf3e9fd77528072f26b10d8ed7b339 100644 (file)
--- 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