Re: Support external parameters in EXECUTE command

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> Continuing the discussion in [0], here is a patch that allows parameter
> references in the arguments of the EXECUTE command. The main purpose is
> submitting protocol-level parameters, but the added regression test case
> shows another way to exercise it.

I spent a bit of time looking at this.

> What's confusing is that the code already contains a reference that
> indicates that this should be possible:
> ...
> I'm not sure what this is supposed to do without my patch on top of it.
> If I remove the estate->es_param_list_info assignment, no tests fail
> (except the one I added). Either this is a leftover from previous
> variants of this code (as discussed in [0]), or there is something I
> haven't understood.

That case is reachable with an example like this:

create or replace function foo(int) returns int language plpgsql as $$
declare
x int := $1;
y int;
begin
execute 'execute fool($1 + 11)' into y using x;
return y;
end
$$;

deallocate fool;
prepare fool(int) as select $1 + 1;

select foo(42);

In the existing code this draws "ERROR: there is no parameter $1".
Your patch makes it work, which is an improvement.

There are a few things bothering me, nonetheless.

1. The patch only implements part of the API for dynamic ParamListInfos,
that is it honors paramFetch but not parserSetup. This seems not to
matter for any of the existing code paths (although we may just be lacking
a test case to reveal it); but I feel that it's just a matter of time
before somebody sets up a case where it would matter. We would have such
a problem today if plpgsql treated EXECUTE as a plain SQL command rather
than its own magic thing, because then "EXECUTE prepared_stmt(plpgsql_var)"
would require the parserSetup hook to be honored in order to resolve the
variable reference.

It's fairly simple to fix this in ExplainExecuteQuery, since that is
creating its own ParseState; it can just apply the plist's parserSetup
to that pstate. I did that in the attached v2 so you can see what
I'm talking about. However, it's a lot less clear what to do in
ExecuteQuery, which as it stands is re-using a passed-in ParseState;
how do we know that the parse hooks aren't already set up in that?
(Or if they are, what do we do to merge their effects?)

2. Actually that latter problem exists already in your patch, because
it's cavalierly overwriting the passed-in ParseState's p_paramref_hook
without regard for the possibility that that's set already. I added
an Assert that it's not set, and we get through check-world that way,
but even to write the assertion is to think that there is surely
going to be a code path that breaks it, soon if not already.

3. Both of the above points seem closely related to the vague worry
I had in the previous discussion about nested contexts all wanting
to control the resolution of parameters. We'll get away with this,
perhaps, as long as that situation never occurs; but once it does
we have issues.

4. I'm inclined to feel that the reason we have these problems is
that this patch handles parameter resolution in the wrong place.
It would likely be better if parameter resolution were already
set up in the ParseState passed to ExecuteQuery (and then we'd fix
ExplainExecuteQuery by likewise passing it a ParseState for the
EXPLAIN EXECUTE). However, that approach would probably result in
Params being available to any utility statement, and then we've
got issues for statements where expressions are only parsed and not
immediately executed: we have to define sane semantics for Param
references in such contexts, and make sure they get honored.

5. So that brings us back to the other point I made earlier, which
is that I'm not happy with patching this locally in EXECUTE rather
than having a design that works across-the-board for utility
statements. You had expressed similar concerns a ways upthread:
>> Come to think of it, it would probably also be useful if PREPARE did
>> parameter processing, again in order to allow use with PQexecParams().

I think it's possible that we could solve the semantics problem
by defining the behavior for Params in utility statements as
"the parameter value is immediately substituted at parse time,
producing a Const node". This doesn't change the behavior in EXECUTE,
because the expression will straightaway be evaluated and produce the
correct value. In situations like

ALTER TABLE foo ADD COLUMN newcol int DEFAULT ($1);

you would also get what seem sane semantics, ie the default is the
value provided as a Param.

I feel that perhaps a patch that does this wouldn't be tremendously
more code than you have here; but the param resolution hook would be
installed at some different more-global place, and it would be code
to generate a Const node not a Param.

I attach a v2 with the trivial mods mentioned above, but just for
illustration not because I think this is the way to go.

regards, tom lane