Re: SQL-standard function body - Mailing list pgsql-hackers

On Fri, Apr 02, 2021 at 02:25:15PM +0200, Peter Eisentraut wrote:
> 
> New patch attached.

Thanks, it all looks good to me.  I just spot a few minor formatting issues:

@@ -2968,6 +2973,13 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
    }

    /* And finally the function definition ... */
+   tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+   if (proc->prolang == SQLlanguageId && !isnull)
+   {
+       print_function_sqlbody(&buf, proctup);
+   }
+   else
+   {
    appendStringInfoString(&buf, "AS ");

    tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_probin, &isnull);
@@ -2999,6 +3011,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
    appendBinaryStringInfo(&buf, dq.data, dq.len);
    appendStringInfoString(&buf, prosrc);
    appendBinaryStringInfo(&buf, dq.data, dq.len);
+   }

The curly braces could probably be removed for the if branch, and the code in
the else branch isn't properly indented.

Other occurences:

+   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
@@ -4317,6 +4337,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
    querytree = transformTopLevelStmt(pstate, linitial(raw_parsetree_list));

    free_parsestate(pstate);
+   }

and

+   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
@@ -4829,18 +4878,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
                                      (Node *) fexpr,
                                      fexpr->inputcollid);

-   /*

and

@@ -2968,6 +2973,13 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
    }

    /* And finally the function definition ... */
+   tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+   if (proc->prolang == SQLlanguageId && !isnull)
+   {
+       print_function_sqlbody(&buf, proctup);
+   }
+   else
+   {
    appendStringInfoString(&buf, "AS ");

    tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_probin, &isnull);
@@ -2999,6 +3011,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
    appendBinaryStringInfo(&buf, dq.data, dq.len);
    appendStringInfoString(&buf, prosrc);
    appendBinaryStringInfo(&buf, dq.data, dq.len);
+   }

and

@@ -12290,7 +12309,11 @@ dumpFunc(Archive *fout, const FuncInfo *finfo)
     * versions would set it to "-".  There are no known cases in which prosrc
     * is unused, so the tests below for "-" are probably useless.
     */
-   if (probin[0] != '\0' && strcmp(probin, "-") != 0)
+   if (prosqlbody)
+   {
+       appendPQExpBufferStr(asPart, prosqlbody);
+   }


Are you planning to run pg_indent before committing or would that add too much
noise?

Anyway since it's only stylistic issues and the feature freeze is getting
closer I'm marking the patch as ready for committer.