CALL versus procedures with output-only arguments

I'm not too happy with this:

regression=# create procedure p1(out x int) language plpgsql
regression-# as 'begin x := 42; end';
CREATE PROCEDURE

regression=# call p1();
ERROR: procedure p1() does not exist
LINE 1: call p1();
^
HINT: No procedure matches the given name and argument types. You might need to add explicit type casts.

regression=# call p1(null);
x
----
42
(1 row)

I can see that that makes some sense within plpgsql, where the CALL
ought to provide a plpgsql variable for each OUT argument. But it
seems moderately insane for calls from SQL. It certainly fails
to match the documentation [1], which says fairly explicitly that
the argument list items match the *input* arguments of the procedure,
and further notes that plpgsql handles output arguments differently.

I think we ought to fix this so that OUT-only arguments are ignored
when calling from SQL not plpgsql. This is less than simple, since
the parser doesn't actually have any context that would let it know
which one we're doing, but I think we could hack that up somehow.
(The RawParseMode mechanism seems like one way we could pass the
info, and there are probably others.)

Alternatively, if we're going to stick with this behavior, we have
to change the docs to explain it. Either way it seems like an
open item for v14. (For those who've forgotten, OUT-only procedure
arguments are a new thing in v14.)

regards, tom lane

[1] https://www.postgresql.org/docs/devel/sql-call.html

čt 20. 5. 2021 v 19:53 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> I'm not too happy with this:
>
> regression=# create procedure p1(out x int) language plpgsql
> regression-# as 'begin x := 42; end';
> CREATE PROCEDURE
>
> regression=# call p1();
> ERROR: procedure p1() does not exist
> LINE 1: call p1();
> ^
> HINT: No procedure matches the given name and argument types. You might
> need to add explicit type casts.
>
> regression=# call p1(null);
> x
> ----
> 42
> (1 row)
>
> I can see that that makes some sense within plpgsql, where the CALL
> ought to provide a plpgsql variable for each OUT argument. But it
> seems moderately insane for calls from SQL. It certainly fails
> to match the documentation [1], which says fairly explicitly that
> the argument list items match the *input* arguments of the procedure,
> and further notes that plpgsql handles output arguments differently.
>
> I think we ought to fix this so that OUT-only arguments are ignored
> when calling from SQL not plpgsql. This is less than simple, since
> the parser doesn't actually have any context that would let it know
> which one we're doing, but I think we could hack that up somehow.
> (The RawParseMode mechanism seems like one way we could pass the
> info, and there are probably others.)
>

+1

Pavel

> Alternatively, if we're going to stick with this behavior, we have
> to change the docs to explain it. Either way it seems like an
> open item for v14. (For those who've forgotten, OUT-only procedure
> arguments are a new thing in v14.)
>
> regards, tom lane
>
> [1] https://www.postgresql.org/docs/devel/sql-call.html
>
>
>

I wrote:
> I think we ought to fix this so that OUT-only arguments are ignored
> when calling from SQL not plpgsql.

I'm working on a patch to make it act that way. I've got some issues
yet to fix with named arguments (which seem rather undertested BTW,
since the patch is passing check-world even though I know it will
crash instantly on cases with CALL+named-args+out-only-args).

Before I spend too much time on it though, I wanted to mention that
it includes undoing 2453ea142's decision to include OUT arguments
in pg_proc.proargtypes for procedures (but not for any other kind of
routine). I thought that was a terrible decision and I'm very happy
to revert it, but is anyone likely to complain loudly?

regards, tom lane

On 5/23/21 8:01 PM, Tom Lane wrote:
> I wrote:
>> I think we ought to fix this so that OUT-only arguments are ignored
>> when calling from SQL not plpgsql.
> I'm working on a patch to make it act that way. I've got some issues
> yet to fix with named arguments (which seem rather undertested BTW,
> since the patch is passing check-world even though I know it will
> crash instantly on cases with CALL+named-args+out-only-args).
>
> Before I spend too much time on it though, I wanted to mention that
> it includes undoing 2453ea142's decision to include OUT arguments
> in pg_proc.proargtypes for procedures (but not for any other kind of
> routine). I thought that was a terrible decision and I'm very happy
> to revert it, but is anyone likely to complain loudly?
>
>

Possibly, Will take a look. IIRC we have based some other things on this.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 5/23/21 8:01 PM, Tom Lane wrote:
>> Before I spend too much time on it though, I wanted to mention that
>> it includes undoing 2453ea142's decision to include OUT arguments
>> in pg_proc.proargtypes for procedures (but not for any other kind of
>> routine). I thought that was a terrible decision and I'm very happy
>> to revert it, but is anyone likely to complain loudly?

> Possibly, Will take a look. IIRC we have based some other things on this.

There's 9213462c5, which I *think* just needs to be reverted along
with much of 2453ea142. But I don't have a JDBC setup to check it
with.

regards, tom lane

I wrote:
>> I think we ought to fix this so that OUT-only arguments are ignored
>> when calling from SQL not plpgsql.

Here's a draft patch for that. The docs probably need some more
fiddling, but I think the code is in good shape. (I'm unsure about
the JDBC compatibility issue, and would appreciate someone else
testing that.)

> I'm working on a patch to make it act that way. I've got some issues
> yet to fix with named arguments (which seem rather undertested BTW,
> since the patch is passing check-world even though I know it will
> crash instantly on cases with CALL+named-args+out-only-args).

After I'd finished fixing that, I realized that HEAD is really pretty
broken for the case. For example

regression=# CREATE PROCEDURE test_proc10(IN a int, OUT b int, IN c int)
regression-# LANGUAGE plpgsql
regression-# AS $$
regression$# BEGIN
regression$# RAISE NOTICE 'a: %, b: %, c: %', a, b, c;
regression$# b := a - c;
regression$# END;
regression$# $$;
CREATE PROCEDURE
regression=# DO $$
regression$# DECLARE _a int; _b int; _c int;
regression$# BEGIN
regression$# _a := 10; _b := 30; _c := 7;
regression$# CALL test_proc10(a => _a, b => _b, c => _c);
regression$# RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
regression$# END$$;
ERROR: procedure test_proc10(a => integer, b => integer, c => integer) does not exist
LINE 1: CALL test_proc10(a => _a, b => _b, c => _c)
^
HINT: No procedure matches the given name and argument types. You might need to add explicit type casts.
QUERY: CALL test_proc10(a => _a, b => _b, c => _c)
CONTEXT: PL/pgSQL function inline_code_block line 5 at CALL

So even if you object to what I'm trying to do here, there is
work to be done.

regards, tom lane

On 24.05.21 02:01, Tom Lane wrote:
> I wrote:
>> I think we ought to fix this so that OUT-only arguments are ignored
>> when calling from SQL not plpgsql.
>
> I'm working on a patch to make it act that way. I've got some issues
> yet to fix with named arguments (which seem rather undertested BTW,
> since the patch is passing check-world even though I know it will
> crash instantly on cases with CALL+named-args+out-only-args).
>
> Before I spend too much time on it though, I wanted to mention that
> it includes undoing 2453ea142's decision to include OUT arguments
> in pg_proc.proargtypes for procedures (but not for any other kind of
> routine). I thought that was a terrible decision and I'm very happy
> to revert it, but is anyone likely to complain loudly?

I don't understand why you want to change this. The argument resolution
of CALL is specified in the SQL standard; we shouldn't just make up our
own system.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 24.05.21 02:01, Tom Lane wrote:
>>> I think we ought to fix this so that OUT-only arguments are ignored
>>> when calling from SQL not plpgsql.

> I don't understand why you want to change this. The argument resolution
> of CALL is specified in the SQL standard; we shouldn't just make up our
> own system.

I don't really see how you can argue that the existing behavior is
more spec-compliant than what I'm suggesting. What I read in the spec
(SQL:2021 10.4 <routine invocation> SR 9) h) iii) 1)) is

1) If Pi is an output SQL parameter, then XAi shall be a <target
specification>.

(where <target specification> more or less reduces to "variable").
Now, sure, that's what we've got in plpgsql, and I'm not proposing
to change that. But in plain SQL, as of HEAD, you are supposed to
write NULL, or a random literal, or indeed anything at all *except*
a variable. How is that more standard-compliant than not writing
anything?

Also, one could argue that the behavior I'm suggesting is completely
spec-compliant if one assumes that the OUT parameters have some sort
of default, allowing them to be omitted from the call.

More generally, there are enough deviations from spec in what we do
to perform ambiguous-call resolution that it seems rather silly to
hang your hat on this particular point.

Now as against that, we are giving up a whole lot of consistency.
As of HEAD:

* The rules for what is a conflict of signatures are different
for functions and procedures.

* The rules for how to identify a target routine in ALTER, DROP,
etc are different for functions and procedures. That's especially
nasty in ALTER/DROP ROUTINE, where we don't have a syntax cue
as to whether or not to ignore OUT parameters.

* The rules for how to call functions and procedures with OUT
parameters from SQL are different.

* Client code that looks at pg_proc.proargtypes is almost certainly
going to be broken.

I don't like any of those side-effects, and I don't want to pay
those prices for what seems to me to be a bogus claim of improved
spec compliance.

regards, tom lane

On 25.05.21 17:20, Tom Lane wrote:
> I don't really see how you can argue that the existing behavior is
> more spec-compliant than what I'm suggesting. What I read in the spec
> (SQL:2021 10.4 <routine invocation> SR 9) h) iii) 1)) is
>
> 1) If Pi is an output SQL parameter, then XAi shall be a <target
> specification>.
>
> (where <target specification> more or less reduces to "variable").
> Now, sure, that's what we've got in plpgsql, and I'm not proposing
> to change that. But in plain SQL, as of HEAD, you are supposed to
> write NULL, or a random literal, or indeed anything at all *except*
> a variable. How is that more standard-compliant than not writing
> anything?

I concede that the current implementation is not fully standards
compliant in this respect. Maybe we need to rethink how we can satisfy
this better. For example, in some other implementations, you write CALL
p(?), (where ? is the parameter placeholder), so it's sort of an output
parameter. However, changing it so that the entire way the parameters
are counted is different seems a much greater departure.

> More generally, there are enough deviations from spec in what we do
> to perform ambiguous-call resolution that it seems rather silly to
> hang your hat on this particular point.

I don't know what you mean by this. Some stuff is different in the
details, but you *can* write conforming code if you avoid the extremely
complicated cases. With your proposal, everything is always different,
and we might as well remove the CALL statement and name it something
else because users migrating from other systems won't be able to use it
properly.

> Now as against that, we are giving up a whole lot of consistency.
> As of HEAD:
>
> * The rules for what is a conflict of signatures are different
> for functions and procedures.

But that's the fault of the way it was done for functions. That doesn't
mean we have to repeat it for procedures. I mean, sure it would be
better if it were consistent. But SQL-standard syntax should behave in
SQL standard ways. Creating, altering, and dropping procedures is meant
to be portable between SQL implementations. If we change this in subtle
ways so that DROP PROCEDURE p(int, int) drops a different procedure in
different SQL implementations, that seems super-dangerous and annoying.

I wrote:
> * The rules for how to identify a target routine in ALTER, DROP,
> etc are different for functions and procedures. That's especially
> nasty in ALTER/DROP ROUTINE, where we don't have a syntax cue
> as to whether or not to ignore OUT parameters.

Just to enlarge on that point a bit:

regression=# create function foo(int, out int) language sql
regression-# as 'select $1';
CREATE FUNCTION
regression=# create procedure foo(int, out int) language sql
regression-# as 'select $1';
CREATE PROCEDURE

IMO this should have failed, but since it doesn't:

regression=# drop routine foo(int, out int);
DROP ROUTINE

Which object was dropped, and what is the argument for that one
being the right one?

Experinentation shows that in HEAD, what is dropped is the procedure,
and indeed the DROP will fail if you try to use it on the function.
That is a compatibility break, because in previous versions this
worked:

regression=# create function foo(int, out int) language sql
as 'select $1';
CREATE FUNCTION
regression=# drop routine foo(int, out int);
DROP ROUTINE

The fact that you now have to be aware of these details to use
ALTER/DROP ROUTINE seems like a pretty serious loss of user
friendliness, as well as compatibility.

regards, tom lane

On Tue, May 25, 2021 at 2:20 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Just to enlarge on that point a bit:
>
> regression=# create function foo(int, out int) language sql
> regression-# as 'select $1';
> CREATE FUNCTION
> regression=# create procedure foo(int, out int) language sql
> regression-# as 'select $1';
> CREATE PROCEDURE
>
> IMO this should have failed, but since it doesn't:
>
> regression=# drop routine foo(int, out int);
> DROP ROUTINE
>
> Which object was dropped, and what is the argument for that one
> being the right one?
>
> Experinentation shows that in HEAD, what is dropped is the procedure,
> and indeed the DROP will fail if you try to use it on the function.
> That is a compatibility break, because in previous versions this
> worked:
>
> regression=# create function foo(int, out int) language sql
> as 'select $1';
> CREATE FUNCTION
> regression=# drop routine foo(int, out int);
> DROP ROUTINE
>
> The fact that you now have to be aware of these details to use
> ALTER/DROP ROUTINE seems like a pretty serious loss of user
> friendliness, as well as compatibility.

I'm also concerned about the behavior here. I noticed it when this
commit went in, and it seemed concerning to me then, and it still
does. Nevertheless, I'm not convinced that your proposal is an
improvement. Suppose we have foo(int, out int) and also foo(int).
Then, if I understand correctly, under your proposal, foo(4) will call
the former within plpgsql code, because in that context the OUT
parameters must be included, and the latter from SQL code, because in
that context they must be emitted. I suspect in practice what will
happen is that you'll end up with both interpretations even within the
body of a plpgsql function, because plpgsql functions tend to include
SQL queries where, I presume, the SQL interpretation must apply. It
seems that it will be very difficult for users to know which set of
rules apply in which contexts.

Now, that being said, the status quo is also pretty bad, because we
have one set of rules for functions and another for procedures. I
believe that users will expect those to behave in similar ways, and
will be sad and surprised when they don't.

But on the third hand, Peter is also correct when he says that there's
not much use in implementing standard features with non-standard
semantics. The fact that we've chosen to make OUT parameters do some
random thing that is not what other systems do is, indeed, not great
for migrations. So doubling down on that questionable choice is also
not great. In a green field I think we ought to go the other way and
make OUT parameters as consistent with the standard as we can, and
have that handling be the same for procedures and for functions, but
it seems impossible to imagine making such a large compatibility break
with our own previous releases, however much the spec may dictate it.

I don't see any really great choice here, but in some sense your
proposal seems like the worst of all the options. It does not reverse
the patch's choice to treat functions and procedures differently, so
users will still have to deal with that inconsistency. But in addition
the handling of procedures will itself be inconsistent based on
context.

--
Robert Haas
EDB: http://www.enterprisedb.com

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 25.05.21 17:20, Tom Lane wrote:
>> I don't really see how you can argue that the existing behavior is
>> more spec-compliant than what I'm suggesting. What I read in the spec
>> (SQL:2021 10.4 <routine invocation> SR 9) h) iii) 1)) is
>> 1) If Pi is an output SQL parameter, then XAi shall be a <target
>> specification>.

> I concede that the current implementation is not fully standards
> compliant in this respect. Maybe we need to rethink how we can satisfy
> this better. For example, in some other implementations, you write CALL
> p(?), (where ? is the parameter placeholder), so it's sort of an output
> parameter. However, changing it so that the entire way the parameters
> are counted is different seems a much greater departure.

I'd expect to be able to write something like that in contexts where
there's a reasonable way to name an output parameter. Like, say,
plpgsql. Or JDBC --- I think they already use a notation like that
for output parameters from functions, and transform it after the fact.
As things work in HEAD, they'll have to have a different special hack
for procedures than they do for functions. But none of this applies
to bare-SQL CALL.

>> More generally, there are enough deviations from spec in what we do
>> to perform ambiguous-call resolution that it seems rather silly to
>> hang your hat on this particular point.

> I don't know what you mean by this.

Well, let's take an example. If OUT parameters are part of the
signature, then I'm allowed to do this:

regression=# create procedure p1(in x int, out y int)
regression-# language sql as 'select $1';
CREATE PROCEDURE
regression=# create procedure p1(in x int, out y float8)
language sql as 'select $1';
CREATE PROCEDURE
regression=# call p1(42, null);
y
----
42
(1 row)

I'm surprised that that worked rather than throwing an ambiguity
error. I wonder which procedure it called, and where in the spec
you can find chapter and verse saying that that one and not the other
one is right.

It gets even sillier though, because experimentation shows that it
was the int one that was preferred:

regression=# create or replace procedure p1(in x int, out y float8)
language sql as 'select $1+1';
CREATE PROCEDURE
regression=# call p1(42, null);
y
----
42
(1 row)

That seems kind of backwards really, considering that float8 is
further up the numeric hierarchy. But let's keep going:

regression=# create procedure p1(in x int, out y text)
language sql as 'select $1+2';
CREATE PROCEDURE
regression=# call p1(42, null);
y
----
44
(1 row)

So text is preferred to either int or float8. I know why that
happened: we have a preference for matching UNKNOWN to string types.
But I challenge you to provide any argument that this behavior is
spec-compliant.

More generally, the point I'm trying to make is that our rules
for resolving an ambiguous function differ in a whole lot of
details from what SQL says. That ship sailed a couple of
decades ago, so I'm not excited about adopting a fundamentally
bad design in pursuit of trying to make one small detail of
that behavior slightly closer to SQL.

[ thinks a bit ]

A lot of what I'm exercised about here is not the question of
how many parameters we write in CALL, but the choice to redefine
proargtypes (and thereby change what is considered the routine's
signature). With the infrastructure in the patch I proposed,
it'd be possible to revert the signature changes and still
write dummy output parameters in CALL -- we'd just make CALL
set include_out_parameters=true all the time. I do not think that
solution is superior to what I did in the patch, but if we can't
have a meeting of the minds on CALL, doing that much would still
be an improvement.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm also concerned about the behavior here. I noticed it when this
> commit went in, and it seemed concerning to me then, and it still
> does. Nevertheless, I'm not convinced that your proposal is an
> improvement. Suppose we have foo(int, out int) and also foo(int).
> Then, if I understand correctly, under your proposal, foo(4) will call
> the former within plpgsql code, because in that context the OUT
> parameters must be included, and the latter from SQL code, because in
> that context they must be emitted.

No, you misunderstand my proposal. The thing that I most urgently
want to do is to prevent that situation from ever arising, by not
allowing those two procedures to coexist, just as you can't have
two functions with such signatures.

If procedures are required to have distinct signatures when considering
input parameters only, then a fortiori they are distinct when also
considering output parameters. So my proposal cannot make a CALL
that includes output parameters ambiguous if it was not before.

> I don't see any really great choice here, but in some sense your
> proposal seems like the worst of all the options. It does not reverse
> the patch's choice to treat functions and procedures differently, so
> users will still have to deal with that inconsistency.

You're definitely confused, because reversing that choice is *exactly*
what I'm on about. The question of whether SQL-level CALL should act
differently from plpgsql CALL is pretty secondary.

regards, tom lane

On Tue, May 25, 2021 at 3:10 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> No, you misunderstand my proposal. The thing that I most urgently
> want to do is to prevent that situation from ever arising, by not
> allowing those two procedures to coexist, just as you can't have
> two functions with such signatures.
>
> If procedures are required to have distinct signatures when considering
> input parameters only, then a fortiori they are distinct when also
> considering output parameters. So my proposal cannot make a CALL
> that includes output parameters ambiguous if it was not before.

Oh, OK.

I'm not sure what I think about that yet. It certainly seems to make
things less confusing. But on the other hand, I think that the
standard - or some competing systems - may have cases where they
disambiguate calls based on output arguments only. Granted, if we
prohibit that now, we can always change our minds and allow it later
if we are sure we've got everything figured out, whereas if we don't
prohibit now, backward compatibility will make it hard to prohibit it
later. But on the other hand I don't really fully understand Peter's
thinking here, so I'm a little reluctant to jump to the conclusion
that he's lost the way.

> > I don't see any really great choice here, but in some sense your
> > proposal seems like the worst of all the options. It does not reverse
> > the patch's choice to treat functions and procedures differently, so
> > users will still have to deal with that inconsistency.
>
> You're definitely confused, because reversing that choice is *exactly*
> what I'm on about. The question of whether SQL-level CALL should act
> differently from plpgsql CALL is pretty secondary.

I understood the reverse from the first post on the thread, so perhaps
it is more that your thinking has developed than that I am confused.

However, it's possible that I only think that because I'm confused.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, May 25, 2021 at 3:10 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> You're definitely confused, because reversing that choice is *exactly*
>> what I'm on about. The question of whether SQL-level CALL should act
>> differently from plpgsql CALL is pretty secondary.

> I understood the reverse from the first post on the thread, so perhaps
> it is more that your thinking has developed than that I am confused.

Yeah, the odd behavior of CALL is where I started from, but now I think
the main problem is with the signature (ie, allowing procedures with
signatures that differ only in OUT parameter positions). If we got
rid of that choice then it'd be possible to document that you should
only ever write NULL for OUT-parameter positions, because the type
of such an argument would never be significant for disambiguation.

We could consider going further and actually enforcing use of NULL,
or inventing some other syntactic placeholder such as the '?' that
Peter was speculating about. But I'm not sure that that adds much.

Relevant to this is that my proposed patch gets rid of the existing
behavior that such arguments actually get evaluated. That would
need to be documented, unless we go with the placeholder approach.
But I've not spent time on the documentation yet.

regards, tom lane

On 25.05.21 22:21, Tom Lane wrote:
> Yeah, the odd behavior of CALL is where I started from, but now I think
> the main problem is with the signature (ie, allowing procedures with
> signatures that differ only in OUT parameter positions). If we got
> rid of that choice then it'd be possible to document that you should
> only ever write NULL for OUT-parameter positions, because the type
> of such an argument would never be significant for disambiguation.

AFAICT, your patch does not main the property that

CREATE PROCEDURE p1(OUT int, OUT int)

corresponds to

DROP PROCEDURE p1(int, int)

which would be bad.

I'm not opposed to reverting the feature if we can't find a good
solution in a hurry. The main value is of this feature is for
migrations, so I want to be sure that whatever we settle on doesn't back
us into a corner with respect to that.

We could perhaps also just disable the SQL-level calling until a better
solution arises. AFAICT, things work okay in PL/pgSQL, because OUT
parameters are tied to a typed target there.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> AFAICT, your patch does not main the property that
> CREATE PROCEDURE p1(OUT int, OUT int)
> corresponds to
> DROP PROCEDURE p1(int, int)
> which would be bad.

Why? If it actually works that way right now, I'd maintain
strenously that it's broken. The latter should be referring
to a procedure with two IN arguments. Even if the SQL spec
allows fuzziness about that, we cannot afford to, because we
have a more generous view of overloading than the spec does.
(As far as I could tell from looking at the spec yesterday,
they think that you aren't allowed to have two procedures
with the same name/schema and same number of arguments,
regardless of the details of those arguments. Up with that
I will not put.)

> I'm not opposed to reverting the feature if we can't find a good
> solution in a hurry.

I'm not looking to revert the feature. I mainly want a saner catalog
representation, and less inconsistency in object naming (which is
tightly tied to the first thing).

regards, tom lane

Here's a stripped-down patch that drops the change in what should be
in CALL argument lists, and just focuses on reverting the change in
pg_proc.proargtypes and the consequent mess for ALTER/DROP ROUTINE.
I spent some more effort on the docs, too.

regards, tom lane

On 26.05.21 19:28, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
>> AFAICT, your patch does not main the property that
>> CREATE PROCEDURE p1(OUT int, OUT int)
>> corresponds to
>> DROP PROCEDURE p1(int, int)
>> which would be bad.
>
> Why? If it actually works that way right now, I'd maintain
> strenously that it's broken. The latter should be referring
> to a procedure with two IN arguments. Even if the SQL spec
> allows fuzziness about that, we cannot afford to, because we
> have a more generous view of overloading than the spec does.

There is no fuzziness in the spec about this. See subclause <specific
routine designator>. It just talks about arguments, nothing about input
or output arguments. I don't find any ambiguity there. I don't see why
we want to reinvent this here.

If I have two procedures

p1(IN int, IN int, OUT int, OUT int)
p1(OUT int, OUT int)

then a DROP, or ALTER, or GRANT, etc. on p1(int, int) should operate on
the second one in a spec-compliant implementation, but you propose to
have it operate on the first one. That kind of discrepancy would be
really bad to have. It would be very difficult for migration tools to
check or handle this in a robust way.

> (As far as I could tell from looking at the spec yesterday,
> they think that you aren't allowed to have two procedures
> with the same name/schema and same number of arguments,
> regardless of the details of those arguments. Up with that
> I will not put.)

I don't see that.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 26.05.21 19:28, Tom Lane wrote:
>> Why? If it actually works that way right now, I'd maintain
>> strenously that it's broken. The latter should be referring
>> to a procedure with two IN arguments. Even if the SQL spec
>> allows fuzziness about that, we cannot afford to, because we
>> have a more generous view of overloading than the spec does.

> There is no fuzziness in the spec about this. See subclause <specific
> routine designator>. It just talks about arguments, nothing about input
> or output arguments. I don't find any ambiguity there. I don't see why
> we want to reinvent this here.

I agree that the spec isn't ambiguous: it says that you should be able
to uniquely identify a routine from the list of only its argument types,
without distinguishing whether those arguments are IN or OUT or INOUT,
*and* without distinguishing whether the routine is a procedure or
function.

However, that doesn't work for Postgres functions, nor for Postgres
routines (since those must include functions). I do not think that we
should confuse our users and effectively break ALTER/DROP ROUTINE in
order to make it sort-of work for procedures. The are-we-exactly-
compatible-with-the-spec ship sailed a couple of decades ago.

> If I have two procedures
> p1(IN int, IN int, OUT int, OUT int)
> p1(OUT int, OUT int)
> then a DROP, or ALTER, or GRANT, etc. on p1(int, int) should operate on
> the second one in a spec-compliant implementation, but you propose to
> have it operate on the first one. That kind of discrepancy would be
> really bad to have.

We already have that situation for functions. I think having procedures
work differently from functions is much worse than your complaint here;
and I do not see why being spec-compliant for one case when we are not
for the other is a good situation to be in.

We could, perhaps, insist that ALTER/DROP include OUT parameters when
it is being applied to a procedure, rather than treating them as being
effectively noise words as we do now. I'd still want to revert the
definition of proargtypes, which would have implications for which
procedure signatures are considered distinct --- but it looks to me
like we would still be allowing more combinations than the spec does.

>> (As far as I could tell from looking at the spec yesterday,
>> they think that you aren't allowed to have two procedures
>> with the same name/schema and same number of arguments,
>> regardless of the details of those arguments. Up with that
>> I will not put.)

> I don't see that.

It's under CREATE PROCEDURE. 11.60 <SQL-invoked routine> SR 20 says

20) Case:

a) If R is an SQL-invoked procedure, then S shall not include another
SQL-invoked procedure whose <schema qualified routine name> is
equivalent to RN and whose number of SQL parameters is PN.

Case b) has different and laxer rules for what you can do with functions,
but it still looks like they'd forbid a lot of situations that we allow.

I think that these restrictive overloading rules have a whole lot to do
with the fact that they feel that you don't need IN/OUT argument labeling
to correctly identify a function or procedure. But, as I said, that ship
sailed for us a long time ago.

regards, tom lane

I wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
>> I don't see that.

> It's under CREATE PROCEDURE. 11.60 <SQL-invoked routine> SR 20 says

Oh... just noticed something else relevant to this discussion: SR 13
in the same section saith

13) If R is an SQL-invoked function, then no <SQL parameter declaration>
in NPL shall contain a <parameter mode>.

In other words, the spec does not have OUT or INOUT parameters for
functions. So, again, their notion of what is sufficient to identify
a routine is based on a very different model than what we are using.

regards, tom lane

On Mon, 2021-05-31 at 15:55 -0400, Tom Lane wrote:
> > If I have two procedures
> > p1(IN int, IN int, OUT int, OUT int)
> > p1(OUT int, OUT int)
> > then a DROP, or ALTER, or GRANT, etc. on p1(int, int) should operate on
> > the second one in a spec-compliant implementation, but you propose to
> > have it operate on the first one. That kind of discrepancy would be
> > really bad to have.
>
> We already have that situation for functions. I think having procedures
> work differently from functions is much worse than your complaint here;
> and I do not see why being spec-compliant for one case when we are not
> for the other is a good situation to be in.

+1

Yours,
Laurenz Albe

On Monday, May 31, 2021, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:

> On Mon, 2021-05-31 at 15:55 -0400, Tom Lane wrote:
> > > If I have two procedures
> > > p1(IN int, IN int, OUT int, OUT int)
> > > p1(OUT int, OUT int)
> > > then a DROP, or ALTER, or GRANT, etc. on p1(int, int) should operate
> on
> > > the second one in a spec-compliant implementation, but you propose to
> > > have it operate on the first one. That kind of discrepancy would be
> > > really bad to have.
> >
> > We already have that situation for functions. I think having procedures
> > work differently from functions is much worse than your complaint here;
> > and I do not see why being spec-compliant for one case when we are not
> > for the other is a good situation to be in.
>
> +1
>

When this discussion concludes a review of the compatibility sections of
the create/drop “routine” reference pages would be appreciated.

I agree that being consistent with our long-standing function behavior is
more important than being standards compliant. FWIW this being DDL lessens
any non-compliance reservations I may have.

David J.

"David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> When this discussion concludes a review of the compatibility sections of
> the create/drop “routine” reference pages would be appreciated.

Good idea, whichever answer we settle on. But it's notable that
the existing text gives no hint that the rules are different
for functions and procedures. That will need work if we leave
the code as it stands.

regards, tom lane

On 5/31/21 4:25 PM, Tom Lane wrote:
>
> Oh... just noticed something else relevant to this discussion: SR 13
> in the same section saith
>
> 13) If R is an SQL-invoked function, then no <SQL parameter declaration>
> in NPL shall contain a <parameter mode>.
>
> In other words, the spec does not have OUT or INOUT parameters for
> functions. So, again, their notion of what is sufficient to identify
> a routine is based on a very different model than what we are using.
>
>

Historical note: this might have had its origin in Ada, where it was the
rule. It's thus amusing that as of the 2012 revision Ada no longer has
this rule, and functions as well as procedures can have IN OUT and OUT
parameters (although there the return value is separate from any OUT
parameter). Ada probably dropped the rule because it was simply a
hindrance rather than a help - certainly I remember finding that it
forced somewhat unnatural expressions back when I was an Ada programmer
(mid 90s). Maybe the SQL spec needs to catch up :-)

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 31.05.21 22:25, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
>>> I don't see that.
>
>> It's under CREATE PROCEDURE. 11.60 <SQL-invoked routine> SR 20 says
>
> Oh... just noticed something else relevant to this discussion: SR 13
> in the same section saith
>
> 13) If R is an SQL-invoked function, then no <SQL parameter declaration>
> in NPL shall contain a <parameter mode>.
>
> In other words, the spec does not have OUT or INOUT parameters for
> functions. So, again, their notion of what is sufficient to identify
> a routine is based on a very different model than what we are using.

Yeah, I figured that was known, but maybe it is good to point it out in
this thread.

The OUT and INOUT parameters for functions and how they affect
signatures was "invented here" for PostgreSQL.

The OUT and INOUT parameters for procedures is something that exists in
the standard and other implementations.

Unfortunately, these two things are not consistent.

So now when we add OUT parameters for procedures in PostgreSQL, we are
forced to make a choice: Do we choose consistency with precedent A or
precedent B? That's the point we disagree on, and I'm not sure how to
resolve it.

Another dimension to this question of what things are consistent with is
how you reference versus how you invoke these things.

If you have a function f1(IN xt, OUT yt), you reference it as f1(xt) and
you invoke it as SELECT f1(xv).

If you have a procedure p1(IN xt, OUT yt), you invoke it as CALL
p1(something, something). So in my mind, it would also make sense to
reference it as p1(something, something).

So while I understand the argument of

- Function signatures should work consistently with procedure signatures.

I find the arguments of

- Procedure signatures should match the SQL standard, and
- Signature for invoking should match signature for calling.

a more appealing combination.

Does that summarize the issue correctly?

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> So while I understand the argument of
> - Function signatures should work consistently with procedure signatures.
> I find the arguments of
> - Procedure signatures should match the SQL standard, and
> - Signature for invoking should match signature for calling.
> a more appealing combination.
> Does that summarize the issue correctly?

Well, mumble ... I think you've left out a couple of significant
problems. The two things that I'm seriously unhappy about are:

1. ALTER/DROP ROUTINE is basically broken. It does not work the
same as it did before for functions; as I showed upthread, there
are cases that worked in prior versions and fail in HEAD. Moreover
it's impossible to make it work in any remotely consistent fashion,
because there are two incompatible standards for it to follow.

2. I really do not like considering OUT arguments as part of a
procedure's unique signature, because that means that you can
have both of
create procedure p1(IN x int, IN y int, OUT z int) ...
create procedure p1(IN x int, IN y int, OUT z text) ...
The key problem with this is that it breaks the advice that
"you can just write NULL for the output argument(s)". Sometimes
you'll have to write something else to select the procedure you
wanted. That's not per the documentation, and it's also going
to be a thorn in the side of client software that would like
to use "?" or some other type-free syntax for OUT parameters.

Given the fact that the spec won't allow you to have two procedures
with the same number of parameters (never mind their types), there's
no argument that this scenario needs to be allowed per spec. So
I think we would be very well advised to prevent it. This is why
I'm so hot about reverting the definition of proargtypes.

It's possible that we could revert proargtypes and still accommodate
the spec's definition for ALTER/DROP ROUTINE/PROCEDURE. I'm imagining
some rules along the line of:

1. If arg list contains any parameter modes, then it must be PG
syntax, so interpret it according to our traditional rules.

2. Otherwise, try to match the given arg types against *both*
proargtypes and proallargtypes. If we get multiple matches,
complain that the command is ambiguous. (In the case of DROP
PROCEDURE, it's probably OK to consider only proallargtypes.)

This is just handwaving at this point, so it might need some
refinement, but perhaps it could lead to an acceptable compromise.

regards, tom lane

I wrote:
> It's possible that we could revert proargtypes and still accommodate
> the spec's definition for ALTER/DROP ROUTINE/PROCEDURE. I'm imagining
> some rules along the line of:
> 1. If arg list contains any parameter modes, then it must be PG
> syntax, so interpret it according to our traditional rules.
> 2. Otherwise, try to match the given arg types against *both*
> proargtypes and proallargtypes. If we get multiple matches,
> complain that the command is ambiguous. (In the case of DROP
> PROCEDURE, it's probably OK to consider only proallargtypes.)

Hmm, actually we could make step 2 a shade tighter: if a candidate
routine is a function, match against proargtypes. If it's a procedure,
match against coalesce(proallargtypes, proargtypes). If we find
multiple matches, raise ambiguity error.

The cases where you get the error could be resolved by either
using traditional PG syntax, or (in most cases) by saying
FUNCTION or PROCEDURE instead of ROUTINE.

An interesting point here is that if you did, say,
create procedure p1(IN x int, IN y float8, OUT z int)
create procedure p1(IN x int, OUT y float8, IN z int)
these would be allowed by my preferred catalog design (since
proargtypes would be different), but their proallargtypes are
the same so you could not drop one using SQL-spec syntax.
You'd be forced into using traditional PG syntax. Since the
spec would disallow the case anyway, I don't see an argument
that this is a problem for spec compliance.

I'm not very sure offhand how thoroughly this approach
covers the expectations of the spec. There may be combinations
of procedure/function signatures that the spec thinks should
be allowed but would be ambiguous according to these rules for
DROP ROUTINE. But I believe that any such cases would be
pretty corner-ish, and we could get away with saying "too bad".

regards, tom lane

I wrote:
> Hmm, actually we could make step 2 a shade tighter: if a candidate
> routine is a function, match against proargtypes. If it's a procedure,
> match against coalesce(proallargtypes, proargtypes). If we find
> multiple matches, raise ambiguity error.

Where do we stand on this topic?

I'm willing to have a go at implementing things that way, but
time's a-wasting.

regards, tom lane

On 6/3/21 2:29 PM, Tom Lane wrote:
> I wrote:
>> Hmm, actually we could make step 2 a shade tighter: if a candidate
>> routine is a function, match against proargtypes. If it's a procedure,
>> match against coalesce(proallargtypes, proargtypes). If we find
>> multiple matches, raise ambiguity error.
> Where do we stand on this topic?
>
> I'm willing to have a go at implementing things that way, but
> time's a-wasting.
>
>

So AIUI your suggestion is that ALTER/DROP ROUTINE will look for an
ambiguity. If it doesn't find one it proceeds, otherwise it complains in
which case the user will have to fall back to ALTER/DROP
FUNCTION/PROCEDURE. Is that right? It seems a reasonable approach, and I
wouldn't expect to find too many ambiguous cases in practice.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> So AIUI your suggestion is that ALTER/DROP ROUTINE will look for an
> ambiguity. If it doesn't find one it proceeds, otherwise it complains in
> which case the user will have to fall back to ALTER/DROP
> FUNCTION/PROCEDURE. Is that right? It seems a reasonable approach, and I
> wouldn't expect to find too many ambiguous cases in practice.

Yeah, I think that practical problems would be pretty rare. My impression
is that users tend not to use function/procedure name overloading too much
in the first place, and none of this affects you at all till you do.

Once you do, you'll possibly notice that PG's rules for which combinations
of signatures are allowed are different from the spec's. I believe that
we're largely more generous than the spec, but there are a few cases where
this proposal isn't. An example is that (AFAICT) the spec allows having
both
create procedure divide(x int, y int, OUT q int) ...
create procedure divide(x int, y int, OUT q int, OUT r int) ...
which I want to reject because they have the same input parameters.
This is perhaps annoying. But seeing that the spec won't allow you to
also have divide() procedures for other datatypes, I'm having a hard
time feeling that this is losing on the overloading-flexibility front.

regards, tom lane

On 6/3/21 4:21 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> So AIUI your suggestion is that ALTER/DROP ROUTINE will look for an
>> ambiguity. If it doesn't find one it proceeds, otherwise it complains in
>> which case the user will have to fall back to ALTER/DROP
>> FUNCTION/PROCEDURE. Is that right? It seems a reasonable approach, and I
>> wouldn't expect to find too many ambiguous cases in practice.
> Yeah, I think that practical problems would be pretty rare. My impression
> is that users tend not to use function/procedure name overloading too much
> in the first place, and none of this affects you at all till you do.
>
> Once you do, you'll possibly notice that PG's rules for which combinations
> of signatures are allowed are different from the spec's. I believe that
> we're largely more generous than the spec, but there are a few cases where
> this proposal isn't. An example is that (AFAICT) the spec allows having
> both
> create procedure divide(x int, y int, OUT q int) ...
> create procedure divide(x int, y int, OUT q int, OUT r int) ...
> which I want to reject because they have the same input parameters.
> This is perhaps annoying. But seeing that the spec won't allow you to
> also have divide() procedures for other datatypes, I'm having a hard
> time feeling that this is losing on the overloading-flexibility front.
>
>

Not sure I follow the "other datatypes" bit. Are you saying the spec
won't let you have this?:

create procedure divide(x int, y int, OUT q int);
create procedure divide(x int, y int, OUT q float);

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Not sure I follow the "other datatypes" bit. Are you saying the spec
> won't let you have this?:

> create procedure divide(x int, y int, OUT q int);
> create procedure divide(x int, y int, OUT q float);

In fact it won't, because the spec's rule is simply "you can't have
two procedures with the same name and same number of parameters"
(where they count OUT parameters, I believe). However the case
I was considering was wanting to have

create procedure divide(x int, y int, OUT q int) ...
create procedure divide(x numeric, y numeric, OUT q numeric) ...

which likewise falls foul of the spec's restriction, but which
IMO must be allowed in Postgres.

regards, tom lane

On 02.06.21 02:04, Tom Lane wrote:
> I wrote:
>> It's possible that we could revert proargtypes and still accommodate
>> the spec's definition for ALTER/DROP ROUTINE/PROCEDURE. I'm imagining
>> some rules along the line of:
>> 1. If arg list contains any parameter modes, then it must be PG
>> syntax, so interpret it according to our traditional rules.
>> 2. Otherwise, try to match the given arg types against *both*
>> proargtypes and proallargtypes. If we get multiple matches,
>> complain that the command is ambiguous. (In the case of DROP
>> PROCEDURE, it's probably OK to consider only proallargtypes.)
>
> Hmm, actually we could make step 2 a shade tighter: if a candidate
> routine is a function, match against proargtypes. If it's a procedure,
> match against coalesce(proallargtypes, proargtypes). If we find
> multiple matches, raise ambiguity error.
>
> The cases where you get the error could be resolved by either
> using traditional PG syntax, or (in most cases) by saying
> FUNCTION or PROCEDURE instead of ROUTINE.

I'm ok with this proposal.

On 6/3/21 4:50 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Not sure I follow the "other datatypes" bit. Are you saying the spec
>> won't let you have this?:
>> create procedure divide(x int, y int, OUT q int);
>> create procedure divide(x int, y int, OUT q float);
> In fact it won't, because the spec's rule is simply "you can't have
> two procedures with the same name and same number of parameters"
> (where they count OUT parameters, I believe).

Oh. That's a truly awful rule.

> However the case
> I was considering was wanting to have
>
> create procedure divide(x int, y int, OUT q int) ...
> create procedure divide(x numeric, y numeric, OUT q numeric) ...
>
> which likewise falls foul of the spec's restriction, but which
> IMO must be allowed in Postgres.
>

Right, we should certainly allow that.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 02.06.21 02:04, Tom Lane wrote:
>> Hmm, actually we could make step 2 a shade tighter: if a candidate
>> routine is a function, match against proargtypes. If it's a procedure,
>> match against coalesce(proallargtypes, proargtypes). If we find
>> multiple matches, raise ambiguity error.

> I'm ok with this proposal.

Cool. Do you want to try to implement it, or shall I?

A question that maybe we should refer to the RMT is whether it's
too late for this sort of redesign for v14. I dislike reverting
the OUT-procedure feature altogether in v14, but perhaps that's
the sanest way to proceed.

regards, tom lane

On 03.06.21 22:21, Tom Lane wrote:
> Once you do, you'll possibly notice that PG's rules for which combinations
> of signatures are allowed are different from the spec's. I believe that
> we're largely more generous than the spec, but there are a few cases where
> this proposal isn't. An example is that (AFAICT) the spec allows having
> both
> create procedure divide(x int, y int, OUT q int) ...
> create procedure divide(x int, y int, OUT q int, OUT r int) ...
> which I want to reject because they have the same input parameters.
> This is perhaps annoying. But seeing that the spec won't allow you to
> also have divide() procedures for other datatypes, I'm having a hard
> time feeling that this is losing on the overloading-flexibility front.

I'm okay with disallowing this. In my experience, overloading of
procedures is done even more rarely than of functions, so this probably
won't affect anything in practice.

(I'm by no means suggesting this, but I could imagine a catalog
representation that allows this but still checks that you can't have
multiple candidates that differ only by the type of an OUT parameters.
Say with some kind of bitmap or boolean array that indicates where the
OUT parameters are. Then you can only have one candidate with a given
number of arguments, but the above could be allowed. Again, I'm not
suggesting this, but it's a possibility in theory.)

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 03.06.21 22:21, Tom Lane wrote:
>> An example is that (AFAICT) the spec allows having both
>> create procedure divide(x int, y int, OUT q int) ...
>> create procedure divide(x int, y int, OUT q int, OUT r int) ...
>> which I want to reject because they have the same input parameters.

> (I'm by no means suggesting this, but I could imagine a catalog
> representation that allows this but still checks that you can't have
> multiple candidates that differ only by the type of an OUT parameters.
> Say with some kind of bitmap or boolean array that indicates where the
> OUT parameters are. Then you can only have one candidate with a given
> number of arguments, but the above could be allowed. Again, I'm not
> suggesting this, but it's a possibility in theory.)

We could certainly do something like that in a green field. But one
of the reasons I'm unhappy about the current design is that I'm convinced
that altering the definition of pg_proc.proargtypes will break client-side
code that's looking at the catalogs. I don't think we get to monkey with
such fundamental bits of the catalog data without a really good reason.
Allowing different OUT parameters for the same IN parameters doesn't seem
to me to qualify, given that there are other reasons why that's dubious.

regards, tom lane

On 03.06.21 23:29, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
>> On 02.06.21 02:04, Tom Lane wrote:
>>> Hmm, actually we could make step 2 a shade tighter: if a candidate
>>> routine is a function, match against proargtypes. If it's a procedure,
>>> match against coalesce(proallargtypes, proargtypes). If we find
>>> multiple matches, raise ambiguity error.
>
>> I'm ok with this proposal.
>
> Cool. Do you want to try to implement it, or shall I?
>
> A question that maybe we should refer to the RMT is whether it's
> too late for this sort of redesign for v14. I dislike reverting
> the OUT-procedure feature altogether in v14, but perhaps that's
> the sanest way to proceed.

I'll take a look at this. I'm not clear on the beta schedule, but the
next beta is probably still a few weeks away.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 02.06.21 02:04, Tom Lane wrote:
>>> It's possible that we could revert proargtypes and still accommodate
>>> the spec's definition for ALTER/DROP ROUTINE/PROCEDURE. I'm imagining
>>> some rules along the line of:
>>> 1. If arg list contains any parameter modes, then it must be PG
>>> syntax, so interpret it according to our traditional rules.
>>> 2. Otherwise, try to match the given arg types against *both*
>>> proargtypes and proallargtypes. If we get multiple matches,
>>> complain that the command is ambiguous. (In the case of DROP
>>> PROCEDURE, it's probably OK to consider only proallargtypes.)

>> Hmm, actually we could make step 2 a shade tighter: if a candidate
>> routine is a function, match against proargtypes. If it's a procedure,
>> match against coalesce(proallargtypes, proargtypes). If we find
>> multiple matches, raise ambiguity error.

> I'm ok with this proposal.

I spent some time playing with this, and ran into a problem.
Given the example we discussed upthread:

d1=# create procedure p1(int, int) language sql as 'select 1';
CREATE PROCEDURE
d1=# create procedure p1(out int, out int) language sql as 'select 1,2';
CREATE PROCEDURE

you can uniquely refer to the first p1 by writing (IN int, IN int),
and you can uniquely refer to the second p1 by writing an empty parameter
list or by writing (OUT int, OUT int). If you write just (int, int),
you get an ambiguity error as discussed.

The problem is that we have a lot of existing code that expects
p1(int, int) to work for the first p1. Notably, this scenario breaks
"pg_dump --clean", which emits commands like

DROP PROCEDURE public.p1(integer, integer);
DROP PROCEDURE public.p1(OUT integer, OUT integer);

It would likely not be very hard to fix pg_dump to include explicit
IN markers. I don't think this results in a compatibility problem
for existing dumps, since they won't be taken from databases in
which there are procedures with OUT arguments.

I'm concerned however about what other client code might get side-swiped.
We (or users) would not be likely to hit the ambiguity right away,
so that sort of issue could go unnoticed for a long time.

So I'm unsure right now whether this is going to be an acceptable
change. I feel like it's still a better situation than what we
have in HEAD, but it's not as cost-free as I'd hoped.

regards, tom lane

I wrote:
> It would likely not be very hard to fix pg_dump to include explicit
> IN markers. I don't think this results in a compatibility problem
> for existing dumps, since they won't be taken from databases in
> which there are procedures with OUT arguments.

Actually, all we have to do to fix pg_dump is to tweak ruleutils.c
(although this has some effects on existing regression test outputs,
of course). So maybe it's not as bad as all that.

Here's a draft-quality patch to handle ALTER/DROP this way. I think
the code may be finished, but I've not looked at the docs at all.

0001 is the same patch I posted earlier, 0002 is a delta to enable
handling ALTER/DROP per spec.

regards, tom lane

On 04.06.21 23:07, Tom Lane wrote:
> I wrote:
>> It would likely not be very hard to fix pg_dump to include explicit
>> IN markers. I don't think this results in a compatibility problem
>> for existing dumps, since they won't be taken from databases in
>> which there are procedures with OUT arguments.
>
> Actually, all we have to do to fix pg_dump is to tweak ruleutils.c
> (although this has some effects on existing regression test outputs,
> of course). So maybe it's not as bad as all that.
>
> Here's a draft-quality patch to handle ALTER/DROP this way. I think
> the code may be finished, but I've not looked at the docs at all.
>
> 0001 is the same patch I posted earlier, 0002 is a delta to enable
> handling ALTER/DROP per spec.

I checked these patches. They appear to match what was talked about. I
didn't find anything surprising. I couldn't apply the 0002 after
applying 0001 to today's master, so I wasn't able to do more exploratory
testing. What are these patches based on? Are there are any more open
issues to focus on?

One thing I was wondering is whether we should force CALL arguments in
direct SQL to be null rather than allowing arbitrary expressions. Since
there is more elaborate code now to process the CALL arguments, maybe it
would be easier than before to integrate that.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 04.06.21 23:07, Tom Lane wrote:
>> 0001 is the same patch I posted earlier, 0002 is a delta to enable
>> handling ALTER/DROP per spec.

> I checked these patches. They appear to match what was talked about. I
> didn't find anything surprising. I couldn't apply the 0002 after
> applying 0001 to today's master, so I wasn't able to do more exploratory
> testing. What are these patches based on? Are there are any more open
> issues to focus on?

Hmm, these are atop HEAD from a week or so back. The cfbot seems to
think they still apply. In any case, I was about to spend some effort
on the docs, so I'll post an updated version soon (hopefully today).

> One thing I was wondering is whether we should force CALL arguments in
> direct SQL to be null rather than allowing arbitrary expressions. Since
> there is more elaborate code now to process the CALL arguments, maybe it
> would be easier than before to integrate that.

Yeah. We could possibly do that, but at first glance it seems like it
would be adding code for little purpose except nanny-ism.

One angle that maybe needs discussion is what about CALL in SQL-language
functions. I see that's disallowed right now. If we're willing to keep
it that way until somebody implements local variables a la SQL/PSM,
then we could transition smoothly to having the same definition as in
plpgsql, where you MUST write a variable. If we wanted to open it up
sooner, we'd have to plan on ending with a definition like "write either
a variable, or NULL to discard the value", so that enforcing
must-be-NULL in the interim would make sense to prevent future
surprises. But IMO that would be best done as a SQL-language-function
specific restriction.

I suppose if you imagine that we might someday have variables in
top-level SQL, then the same argument would apply there. But we already
guaranteed ourselves some conversion pain for that scenario with respect
to INOUT parameters, so I doubt that locking down OUT parameters will
help much.

My inclination is to not bother adding the restriction, but it's
only a mild preference.

regards, tom lane

I wrote:
> Hmm, these are atop HEAD from a week or so back. The cfbot seems to
> think they still apply. In any case, I was about to spend some effort
> on the docs, so I'll post an updated version soon (hopefully today).

Here is said update (rolled up into one patch this time; maybe that will
avoid the apply problems you had).

I noticed that there is one other loose end in the patch: should
LookupFuncName() really be passing OBJECT_ROUTINE to
LookupFuncNameInternal()? This matches its old behavior, in which
no particular routine type restriction was applied; but I think that
the callers are nearly all expecting that only plain functions will
be returned. That's more of a possible pre-existing bug than it
is the fault of the patch, but nonetheless this might be a good
time to resolve it.

regards, tom lane

On 08.06.21 01:10, Tom Lane wrote:
> I wrote:
>> Hmm, these are atop HEAD from a week or so back. The cfbot seems to
>> think they still apply. In any case, I was about to spend some effort
>> on the docs, so I'll post an updated version soon (hopefully today).
>
> Here is said update (rolled up into one patch this time; maybe that will
> avoid the apply problems you had).

This patch looks good to me.

A minor comment: You changed the docs in some places like this:

- </itemizedlist></para>
+ </itemizedlist>
+ </para>

The original layout is required to avoid spurious whitespace in the
output (mainly affecting man pages).

> I noticed that there is one other loose end in the patch: should
> LookupFuncName() really be passing OBJECT_ROUTINE to
> LookupFuncNameInternal()? This matches its old behavior, in which
> no particular routine type restriction was applied; but I think that
> the callers are nearly all expecting that only plain functions will
> be returned. That's more of a possible pre-existing bug than it
> is the fault of the patch, but nonetheless this might be a good
> time to resolve it.

It appears that all uses of LookupFuncName() are lookups of internal
support functions (with one exception in pltcl), so using
OBJECT_FUNCTION would be okay.

It might be good to keep the argument order of LookupFuncNameInternal()
consistent with LookupFuncWithArgs() with respect to the new ObjectType
argument.

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> On 08.06.21 01:10, Tom Lane wrote:
>> Here is said update (rolled up into one patch this time; maybe that will
>> avoid the apply problems you had).

> This patch looks good to me.

Thanks for reviewing!

> A minor comment: You changed the docs in some places like this:
> - </itemizedlist></para>
> + </itemizedlist>
> + </para>
> The original layout is required to avoid spurious whitespace in the
> output (mainly affecting man pages).

Ugh, that seems like a toolchain bug. We're certainly not consistent
about formatting things that way. But I'll refrain from changing these.

>> I noticed that there is one other loose end in the patch: should
>> LookupFuncName() really be passing OBJECT_ROUTINE to
>> LookupFuncNameInternal()?

> It appears that all uses of LookupFuncName() are lookups of internal
> support functions (with one exception in pltcl), so using
> OBJECT_FUNCTION would be okay.

OK, I'll take a closer look at that.

> It might be good to keep the argument order of LookupFuncNameInternal()
> consistent with LookupFuncWithArgs() with respect to the new ObjectType
> argument.

Good point, thanks.

regards, tom lane