From: Tom Lane <tgl@sss.pgh.pa.us>
To: Erki Eessaar <erki.eessaar@taltech.ee>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: Re: References to parameters by name are lost in INSERT INTO ... SELECT <parameter value> .... statements in case of routines with the SQL-standard function body
Date: Tue, 16 Nov 2021 16:52:57 -0500
Message-ID: <2345766.1637099577@sss.pgh.pa.us> (raw)
In-Reply-To: <AM9PR01MB82684DD5CCD1C8653D42149CFE999@AM9PR01MB8268.eurprd01.prod.exchangelabs.com>
References: <AM9PR01MB8268347BED344848555167FAFE949@AM9PR01MB8268.eurprd01.prod.exchangelabs.com>
<CAD21AoCP04K5C_DhOqMdhcryoYxsh__oV3NaJmKbpyxirOeeBw@mail.gmail.com>
<AM9PR01MB82684DD5CCD1C8653D42149CFE999@AM9PR01MB8268.eurprd01.prod.exchangelabs.com>
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 body as well, i.e., the result 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
Attachments:fix-sql-function-param-decompile.patch (text/x-diff, 1.1K) downloaddiff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.cindex 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];
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, erki.eessaar@taltech.ee, sawada.mshk@gmail.com, pgsql-bugs@lists.postgresql.org, peter.eisentraut@enterprisedb.com
Subject: Re: References to parameters by name are lost in INSERT INTO ... SELECT <parameter value> .... statements in case of routines with the SQL-standard function body
In-Reply-To: <2345766.1637099577@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox