Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mn6O0-0006gC-S9 for pgsql-bugs@arkaria.postgresql.org; Tue, 16 Nov 2021 21:53:04 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1mn6Nz-0006Kg-JX for pgsql-bugs@arkaria.postgresql.org; Tue, 16 Nov 2021 21:53:03 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mn6Nz-0006KW-BH for pgsql-bugs@lists.postgresql.org; Tue, 16 Nov 2021 21:53:03 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mn6Nw-0003oL-N9 for pgsql-bugs@lists.postgresql.org; Tue, 16 Nov 2021 21:53:02 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 1AGLqvGY2345767; Tue, 16 Nov 2021 16:52:57 -0500 From: Tom Lane To: Erki Eessaar cc: Masahiko Sawada , "pgsql-bugs@lists.postgresql.org" , Peter Eisentraut Subject: Re: References to parameters by name are lost in INSERT INTO ... SELECT .... statements in case of routines with the SQL-standard function body In-reply-to: References: Comments: In-reply-to Erki Eessaar message dated "Tue, 16 Nov 2021 11:14:39 +0000" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <2345755.1637099569.0@sss.pgh.pa.us> Date: Tue, 16 Nov 2021 16:52:57 -0500 Message-ID: <2345766.1637099577@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <2345755.1637099569.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Erki Eessaar writes: > Indeed, re-execution of this code without any modifications in it produc= es 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 t= he order of parameters requires changing the body as well, i.e., the resul= t lost some of its qualities. Thus, one could argue that in this case the = input and output are not 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-sql-function-param-decompile.patch"; charset="us-ascii" Content-ID: <2345755.1637099569.2@sss.pgh.pa.us> Content-Description: fix-sql-function-param-decompile.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul= eutils.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 *conte= xt) * If it's an external parameter, see if the outermost namespace provide= s * function argument names. */ - if (param->paramkind =3D=3D PARAM_EXTERN) + if (param->paramkind =3D=3D PARAM_EXTERN && context->namespaces !=3D NIL= ) { - dpns =3D lfirst(list_tail(context->namespaces)); - if (dpns->argnames) + dpns =3D llast(context->namespaces); + if (dpns->argnames && + param->paramid > 0 && + param->paramid <=3D dpns->numargs) { char *argname =3D dpns->argnames[param->paramid - 1]; = ------- =_aaaaaaaaaa0--