Re: References to parameters by name are lost in INSERT INTO ... SELECT .... statements in case of routines with the SQL-standard function body

Erki Eessaar <erki.eessaar@taltech.ee> writes:
> Indeed, re-execution of this code without any modifications in it produces the same result.

Right, that printout is functionally equivalent to the original.

> Still I see here two problems.
>   *   Inconsistency - see INSERT vs. UPDATE.

Yeah, it's weird that the same parameter is printed two different ways.
I dug into it and found out that we're losing the "context->namespace"
list when recursing into the sub-SELECT from get_insert_query_def.
The fix is trivial (attached).  The other places where get_query_def is
invoked quasi-recursively all pass down the parent namespace list already.
The fact that this one is out of step is a very ancient oversight (it's
at least old enough to vote, according to some quick git archaeology).
But as far as I can see, it didn't have any visible consequences until
commit e717a9a18 taught get_parameter() to pay attention to the last
entry of the list.  So I'm inclined not to change it before v14.

I was also pretty unhappy that get_parameter() was so naively trusting
that an EXTERN Param must have something to do with the last entry's
argnames array, or even that there must be a last entry.  So the attached
makes that a little more paranoid, too.

>   *   The problem of positional references in general is that changing the order of parameters requires changing the
bodyas well, i.e., the result lost some of its qualities. Thus, one could argue that in this case the input and output
arenot equivalent. 

I don't think this argument holds a lot of water, because you could
reverse it too: if the author had originally written $N, maybe there
was a reason why she thought that would be more maintainable than a named
reference.  But we don't store any distinction between the two ways of
writing a Param reference, so we can't promise to regenerate it exactly
the way it was written.

Still, it's clear that the intent here was to print a reference-by-name
if possible, and the failure to do so within only the context of
INSERT/SELECT is therefore a bug.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1bb25738a5..6b4022c3bc 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6566,7 +6566,7 @@ get_insert_query_def(Query *query, deparse_context *context)
     if (select_rte)
     {
         /* Add the SELECT */
-        get_query_def(select_rte->subquery, buf, NIL, NULL,
+        get_query_def(select_rte->subquery, buf, context->namespaces, NULL,
                       context->prettyFlags, context->wrapColumn,
                       context->indentLevel);
     }
@@ -7919,10 +7919,12 @@ get_parameter(Param *param, deparse_context *context)
      * If it's an external parameter, see if the outermost namespace provides
      * function argument names.
      */
-    if (param->paramkind == PARAM_EXTERN)
+    if (param->paramkind == PARAM_EXTERN && context->namespaces != NIL)
     {
-        dpns = lfirst(list_tail(context->namespaces));
-        if (dpns->argnames)
+        dpns = llast(context->namespaces);
+        if (dpns->argnames &&
+            param->paramid > 0 &&
+            param->paramid <= dpns->numargs)
         {
             char       *argname = dpns->argnames[param->paramid - 1];