Re: prokind column (was Re: [HACKERS] SQL procedures)

[Peter Eisentraut]

On 2/28/18 17:37, Peter Eisentraut wrote:
> On 2/28/18 15:45, Tom Lane wrote:
>> I have reviewed this patch and attach an updated version below.
>> I've rebased it up to today, fixed a few minor errors, and adopted
>> most of Michael's suggestions.  Also, since I remain desperately
>> unhappy with putting zeroes into prorettype, I changed it to not
>> do that ;-) ... now procedures have VOIDOID as their prorettype,
>> and it will be substantially less painful to allow them to return
>> some other scalar result in future, should we wish to.  I believe
>> I've found all the places that were relying on prorettype == 0 as
>> a substitute for prokind == 'p'.
> 
> I have just posted "INOUT parameters in procedures", which contains some
> of those same changes.  So I think we're on the same page.  I will work
> on consolidating this.

Committed this so far.  More to come on touching this up.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Michael Paquier]

On Wed, Feb 28, 2018 at 05:37:11PM -0500, Peter Eisentraut wrote:
> On 2/28/18 15:45, Tom Lane wrote:
>> I have reviewed this patch and attach an updated version below.
>> I've rebased it up to today, fixed a few minor errors, and adopted
>> most of Michael's suggestions.  Also, since I remain desperately
>> unhappy with putting zeroes into prorettype, I changed it to not
>> do that ;-) ... now procedures have VOIDOID as their prorettype,
>> and it will be substantially less painful to allow them to return
>> some other scalar result in future, should we wish to.  I believe
>> I've found all the places that were relying on prorettype == 0 as
>> a substitute for prokind == 'p'.
> 
> I have just posted "INOUT parameters in procedures", which contains some
> of those same changes.  So I think we're on the same page.  I will work
> on consolidating this.

Thanks Peter.

I have read the patch set that Tom has posted here and hunted for other
inconsistencies.  It seems to me that you have spotted everything.

The changes in ProcedureCreate() are nice.

I was wondering as well if it would be worth checking the contents of
pg_proc with prorettype = 0 in the regression tests to always make sure
that this never happens...  Until I saw that opr_sanity.sql was actually
doing already that so procedures actually are breaking that check on
HEAD.
--
Michael


signature.asc
Description: PGP signature

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Peter Eisentraut]

On 2/28/18 15:45, Tom Lane wrote:
> I have reviewed this patch and attach an updated version below.
> I've rebased it up to today, fixed a few minor errors, and adopted
> most of Michael's suggestions.  Also, since I remain desperately
> unhappy with putting zeroes into prorettype, I changed it to not
> do that ;-) ... now procedures have VOIDOID as their prorettype,
> and it will be substantially less painful to allow them to return
> some other scalar result in future, should we wish to.  I believe
> I've found all the places that were relying on prorettype == 0 as
> a substitute for prokind == 'p'.

I have just posted "INOUT parameters in procedures", which contains some
of those same changes.  So I think we're on the same page.  I will work
on consolidating this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Michael Paquier]

On Tue, Feb 27, 2018 at 11:50:31AM -0500, Tom Lane wrote:
> We support the various psql/describe.c features against old servers,
> so it would be quite inconsistent for tab completion not to work
> similarly.  There are some gaps in that already, as per the other
> thread, but we should clean those up not add more.

I'll try to study this thread a bit more as I lack context.  So I'll
reply there.  What I am scared of is that Tomas Munro has done a heroic
effort to increase the readability of psql's tab completion during the
dev cycle of 9.6.  And it would be sad to reduce the quality of this
code.
--
Michael


signature.asc
Description: PGP signature

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Tom Lane]

Catalin Iacob  writes:
> On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier  wrote:
>> I would just recommend users to use a version of psql matching
>> the one of the server instead of putting an extra load of maintenance
>> into psql for years to come

> Breaking tab completion in new psql against old servers might be
> acceptable as it's a fringe feature, but I don't think your
> recommendation of matching versions is practical. Lots of people
> manage multiple server versions and using the latest psql for all of
> them is currently, as far as I know, a perfectly supported way of
> doing that, getting new psql features and keeping compatibility. I
> think it would be a pity to loose that.

We support the various psql/describe.c features against old servers,
so it would be quite inconsistent for tab completion not to work
similarly.  There are some gaps in that already, as per the other
thread, but we should clean those up not add more.

regards, tom lane

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Catalin Iacob]

On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier  wrote:
> I would just recommend users to use a version of psql matching
> the one of the server instead of putting an extra load of maintenance
> into psql for years to come

Breaking tab completion in new psql against old servers might be
acceptable as it's a fringe feature, but I don't think your
recommendation of matching versions is practical. Lots of people
manage multiple server versions and using the latest psql for all of
them is currently, as far as I know, a perfectly supported way of
doing that, getting new psql features and keeping compatibility. I
think it would be a pity to loose that.

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Michael Paquier]

On Mon, Feb 26, 2018 at 02:03:19PM -0500, Tom Lane wrote:
> I'm not sure that other patch will get in; AFAICS it's incomplete and
> rather controversial.  But I guess we could put this issue on the
> open-items list so we don't forget.

+1.  We already know that we want to do a switch to prokind anyway,
while the other patch is still pending (don't think much about it
myself, I would just recommend users to use a version of psql matching
the one of the server instead of putting an extra load of maintenance
into psql for years to come).  So I would recommend to push forward with
this one and fix what we know we have to fix, then request a rebase.  We
gain nothing by letting things in a semi-broken state.  I'll be happy to
help those folks rebase and/or write the patch to update psql's tab
completion for prokind as need be.

> Anyway, once the release dust settles, I'll try to do a proper review
> of this patch.  It'd be good if we could get it in this week before
> the CF starts, so that any affected patches could be rebased.

Here is some input from me.  I don't like either that prorettype is used
to determine if the object used is a procedure or a function.  The patch
series is very sensitive to changes in pg_proc.h, still those apply
correctly when using bc1adc65 as base commit.

The original commit adding support for procedures had this diff in
clauses.c:
@@ -4401,6 +4401,7 @@ inline_function(Oid funcid, Oid result_type, Oid 
result_collid,
if (funcform->prolang != SQLlanguageId ||
funcform->prosecdef ||
funcform->proretset ||
+   funcform->prorettype == InvalidOid ||
funcform->prorettype == RECORDOID ||

Perhaps this should be replaced with a check on prokind?

It seems to me that the same applies to fmgr_sql_validator().  In
information_schema.sql, type_udt_catalog uses a similar comparison so
this should have a comment about why using prorettype makes more sense.
In short for all those tings, it is fine to use prorettype when directly
working on the result type, like what is done in plperl and plpython.
For all the code paths working on the object type, I think that using
prokind should be the way to go.

getProcedureTypeDescription() should use an enum instead, complaining
with elog(ERROR) if the type found is something else?

I think that get_func_isagg should be dropped and replaced by
get_func_prokind.
--
Michael


signature.asc
Description: PGP signature

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Tom Lane]

Peter Eisentraut  writes:
> On 2/24/18 14:08, Tom Lane wrote:
>> I took a quick look through this and noted a few small problems; the
>> only one worth mentioning here is that you've broken psql tab completion
>> for functions and aggregates when talking to a pre-v11 server.
>> I don't think that's acceptable; however, since tab-complete.c has no
>> existing provisions for adjusting its queries based on server version,
>> it's not really clear what to do to fix it.  I'm sure that's soluble
>> (and I think I recall a recent thread bumping up against the same
>> problem for another change), but it needs a bit more sweat.

> The patches proposed in the thread "PATCH: psql tab completion for
> SELECT" appear to add support for version-dependent tab completion, so
> this could be resolved "later".

I'm not sure that other patch will get in; AFAICS it's incomplete and
rather controversial.  But I guess we could put this issue on the
open-items list so we don't forget.

Anyway, once the release dust settles, I'll try to do a proper review
of this patch.  It'd be good if we could get it in this week before
the CF starts, so that any affected patches could be rebased.

regards, tom lane

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Peter Eisentraut]

On 2/24/18 14:08, Tom Lane wrote:
> I took a quick look through this and noted a few small problems; the
> only one worth mentioning here is that you've broken psql tab completion
> for functions and aggregates when talking to a pre-v11 server.
> I don't think that's acceptable; however, since tab-complete.c has no
> existing provisions for adjusting its queries based on server version,
> it's not really clear what to do to fix it.  I'm sure that's soluble
> (and I think I recall a recent thread bumping up against the same
> problem for another change), but it needs a bit more sweat.

The patches proposed in the thread "PATCH: psql tab completion for
SELECT" appear to add support for version-dependent tab completion, so
this could be resolved "later".

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: prokind column (was Re: [HACKERS] SQL procedures)

[John Naylor]

On 2/25/18, Tom Lane  wrote:
> We need a plan for when/how to apply this, along with the proposed
> bootstrap data conversion patch, which obviously conflicts with it
> significantly.

The bulk changes in the bootstrap data patch are scripted rather than
patched, so the prokind patch will pose little in the way of
conflicts. I can't verify this just yet since Peter's second patch
doesn't apply for me against c4ba1bee68ab. Also, as of version 7 my
patch left out default values and human-readable oids, since I wanted
to get the new generated headers reviewed and up to project standards
first. Since I'll likely have to adjust the patches for those features
anyway, there's plenty of room for me to adjust to the changes to
pg_proc.h as well.

> My thought here is that the data conversion patch is going to break
> basically every pending patch that touches src/include/catalog/,
> so we ought to apply it at a point where that list of patches is short
> and there's lots of time for people to redo them.  Hence, end of the
> dev cycle is the right time.

I agree.

-John Naylor

Re: prokind column (was Re: [HACKERS] SQL procedures)

[Tom Lane]

Peter Eisentraut  writes:
> Here is this patch updated.  The client changes are now complete and all
> the tests pass.  I have also rolled back the places where the code used
> prorettype to detect procedures and replaced this by the new facility.

I took a quick look through this and noted a few small problems; the
only one worth mentioning here is that you've broken psql tab completion
for functions and aggregates when talking to a pre-v11 server.
I don't think that's acceptable; however, since tab-complete.c has no
existing provisions for adjusting its queries based on server version,
it's not really clear what to do to fix it.  I'm sure that's soluble
(and I think I recall a recent thread bumping up against the same
problem for another change), but it needs a bit more sweat.

We need a plan for when/how to apply this, along with the proposed
bootstrap data conversion patch, which obviously conflicts with it
significantly.  Looking through the stuff pending in next month's
commitfest, we are fortunate in that only a few of those patches
seem to need to touch pg_proc.h at all, and none have really large
deltas AFAICS (I might've missed something though).  I propose
proceeding as follows:

1. Get this patch in as soon as we can resolve its remaining weak
spots.  That will force rebasing of pending patches that touch
pg_proc.h, but I don't think it'll be painful, since the needed
changes are pretty small and obvious.

2. During the March commitfest, adjust the bootstrap data conversion
patch to handle this change, and review it generally.

3. At the end of the 'fest, or whenever we've dealt with all other
patches that need to touch the bootstrap source data, apply the
data conversion patch.

My thought here is that the data conversion patch is going to break
basically every pending patch that touches src/include/catalog/,
so we ought to apply it at a point where that list of patches is short
and there's lots of time for people to redo them.  Hence, end of the
dev cycle is the right time.

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 1/2/18 11:47, Tom Lane wrote:
> +1 --- seems like a new bool column is the thing.  Or may we should merge
> "proisprocedure" with proisagg and proiswindow into an enum prokind?
> Although that would break some existing client-side code.

prokind sounds good.  I'll look into that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Andrew Dunstan]

On 01/02/2018 01:45 PM, Robert Haas wrote:
> On Tue, Jan 2, 2018 at 11:47 AM, Tom Lane  wrote:
>>> Anyway, I think it would be better to invent an explicit way to
>>> represent whether something is a procedure rather than relying on
>>> overloading prorettype to tell us.
>> +1 --- seems like a new bool column is the thing.  Or may we should merge
>> "proisprocedure" with proisagg and proiswindow into an enum prokind?
>> Although that would break some existing client-side code.
> Assuming that we're confident that "procedure" is mutually exclusive
> with "aggregate" and "window function", that seems like the right way
> to go.  And I think that's probably the case, both because we're
> deciding not to let procedures be called from SELECT statements
> (though Oracle apparently does) and because we've chosen syntax that
> suggests distinctness: CREATE AGGREGATE, CREATE FUNCTION, CREATE
> PROCEDURE.  Window functions are less distinct from the others, since
> they go through CREATE FUNCTION, but it still seems unlikely that we'd
> ever want to try to support a "window procedure".
>


Yeah, but these things don't feel like they belong in the same category.
The fact that we have to ask this question is a symptom of that. A
boolean feels more natural to me here, although I acknowledge it will
result in a tiny amount of catalog bloat. Tom's point about client-side
code is also valid. I don't feel very strongly about it, though.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Robert Haas]

On Tue, Jan 2, 2018 at 11:47 AM, Tom Lane  wrote:
>> Anyway, I think it would be better to invent an explicit way to
>> represent whether something is a procedure rather than relying on
>> overloading prorettype to tell us.
>
> +1 --- seems like a new bool column is the thing.  Or may we should merge
> "proisprocedure" with proisagg and proiswindow into an enum prokind?
> Although that would break some existing client-side code.

Assuming that we're confident that "procedure" is mutually exclusive
with "aggregate" and "window function", that seems like the right way
to go.  And I think that's probably the case, both because we're
deciding not to let procedures be called from SELECT statements
(though Oracle apparently does) and because we've chosen syntax that
suggests distinctness: CREATE AGGREGATE, CREATE FUNCTION, CREATE
PROCEDURE.  Window functions are less distinct from the others, since
they go through CREATE FUNCTION, but it still seems unlikely that we'd
ever want to try to support a "window procedure".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] SQL procedures

[Pavel Stehule]

2018-01-02 17:47 GMT+01:00 Tom Lane :

> Robert Haas  writes:
> > I agree that we need this, but using prorettype = InvalidOid to do it
> > might not be the best way, because it only works for procedures that
> > don't return anything.  If a procedure could return, say, an integer,
>
> Good point, because that is possible in some other systems, and so
> somebody is going to ask for it at some point.
>
> > Anyway, I think it would be better to invent an explicit way to
> > represent whether something is a procedure rather than relying on
> > overloading prorettype to tell us.
>
> +1 --- seems like a new bool column is the thing.  Or may we should merge
> "proisprocedure" with proisagg and proiswindow into an enum prokind?
> Although that would break some existing client-side code.
>

+1

Pavel


> PS: I still strongly disagree with allowing prorettype to be zero.
>
> regards, tom lane
>
>

Re: [HACKERS] SQL procedures

[Tom Lane]

Robert Haas  writes:
> I agree that we need this, but using prorettype = InvalidOid to do it
> might not be the best way, because it only works for procedures that
> don't return anything.  If a procedure could return, say, an integer,

Good point, because that is possible in some other systems, and so
somebody is going to ask for it at some point.

> Anyway, I think it would be better to invent an explicit way to
> represent whether something is a procedure rather than relying on
> overloading prorettype to tell us.

+1 --- seems like a new bool column is the thing.  Or may we should merge
"proisprocedure" with proisagg and proiswindow into an enum prokind?
Although that would break some existing client-side code.

PS: I still strongly disagree with allowing prorettype to be zero.

regards, tom lane

Re: [HACKERS] SQL procedures

[Robert Haas]

On Wed, Nov 8, 2017 at 9:21 AM, Peter Eisentraut
 wrote:
>> Why not use VOIDOID for the prorettype value?
>
> We need a way to distinguish functions that are callable by SELECT and
> procedures that are callable by CALL.

I agree that we need this, but using prorettype = InvalidOid to do it
might not be the best way, because it only works for procedures that
don't return anything.  If a procedure could return, say, an integer,
then it would fail, because we have two ways to say that something in
pg_proc returns nothing (InvalidOid, VOIDOID) but we have only one way
to say that it returns an integer (INT4OID).  Similarly, we'd have a
problem if we ever tried to use procedures for handling triggers or
(perhaps more likely) event triggers, since those special kinds of
functions also signal their purpose via the return type - which may
also not have been such a hot idea, but at least in those cases the
idea of returning any sort of real result is more or less known to be
nonsensical.

Anyway, I think it would be better to invent an explicit way to
represent whether something is a procedure rather than relying on
overloading prorettype to tell us.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] SQL procedures

[Simon Riggs]

On 2 December 2017 at 01:31, Peter Eisentraut
 wrote:
> On 11/30/17 15:50, Thomas Munro wrote:
>> postgres=# \df
>>List of functions
>>  Schema | Name | Result data type | Argument data types | Type
>> +--+--+-+--
>>  public | bar  | integer  | i integer   | func
>>  public | foo  |  | i integer   | proc
>> (2 rows)
>>
>> Should this now be called a "List of routines"?
>
> Maybe, but I hesitate to go around and change all mentions of "function"
> like that.  That might just confuse people.

Agreed

\dfn shows functions only
so we might want to consider having
\df say "List of functions and procedures"
\dfn say "List of functions"

and a new option to list procedures only
\dfp say "List of procedures"

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Robert Haas]

On Fri, Dec 1, 2017 at 9:31 AM, Peter Eisentraut
 wrote:
> On 11/30/17 15:50, Thomas Munro wrote:
>> postgres=# \df
>>List of functions
>>  Schema | Name | Result data type | Argument data types | Type
>> +--+--+-+--
>>  public | bar  | integer  | i integer   | func
>>  public | foo  |  | i integer   | proc
>> (2 rows)
>>
>> Should this now be called a "List of routines"?
>
> Maybe, but I hesitate to go around and change all mentions of "function"
> like that.  That might just confuse people.

Yeah, this is not unlike the problems we have deciding whether to say
"relation" or "table".  It's a problem that comes when most foos are
bars but there are multiple types of exotic foo that are not bars.
That's pretty much the case here -- most functions are probably just
functions, but a few might be procedures or aggregates.  I think
leaving this and similar cases as "functions" is fine.  I wonder
whether it was really necessary for the SQL standards committee (or
Oracle) to invent both procedures and functions to represent very
similar things, but they did.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/30/17 15:50, Thomas Munro wrote:
> postgres=# \df
>List of functions
>  Schema | Name | Result data type | Argument data types | Type
> +--+--+-+--
>  public | bar  | integer  | i integer   | func
>  public | foo  |  | i integer   | proc
> (2 rows)
> 
> Should this now be called a "List of routines"?

Maybe, but I hesitate to go around and change all mentions of "function"
like that.  That might just confuse people.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/29/17 14:17, Andrew Dunstan wrote:
> On 11/28/2017 10:03 AM, Peter Eisentraut wrote:
>> Here is a new patch that addresses the previous review comments.
>>
>> If there are no new comments, I think this might be ready to go.
> 
> Looks good to me. Marking ready for committer.

committed


-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/28/17 10:34, Simon Riggs wrote:
> Is ERRCODE_INVALID_FUNCTION_DEFINITION still appropriate?

Well, maybe not, but changing that is likely more trouble than it's worth.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Simon Riggs]

On 29 November 2017 at 02:03, Peter Eisentraut
 wrote:
> Here is a new patch that addresses the previous review comments.
>
> If there are no new comments, I think this might be ready to go.

Is ERRCODE_INVALID_FUNCTION_DEFINITION still appropriate?

Other than that, looks ready to go.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Craig Ringer]

On 23 November 2017 at 03:47, Andrew Dunstan  wrote:

>
>
> On 11/22/2017 02:39 PM, Corey Huinker wrote:
> >
> >
> > T-SQL procedures returns data or OUT variables.
> >
> > I remember, it was very frustrating
> >
> > Maybe first result can be reserved for OUT variables, others for
> > multi result set
> >
> >
> > It's been many years, but if I recall correctly, T-SQL returns a
> > series of result sets, with no description of the result sets to be
> > returned, each of which must be consumed fully before the client can
> > move onto the next result set. Then and only then can the output
> > parameters be read. Which was very frustrating because the OUT
> > parameters seemed like a good place to put values for things like
> > "result set 1 has 205 rows" and "X was false so we omitted one result
> > set entirely" so you couldn't, y'know easily omit entire result sets.
> >
>
>
>
> Exactly. If we want to handle OUT params this way they really need to be
> the first resultset for just this reason. That could possibly be done by
> the glue code reserving a spot in the resultset list and filling it in
> at the end of the procedure.
>

I fail to understand how that can work though. Wouldn't we have to buffer
all the resultset contents on the server in tuplestores or similar, so we
can send the parameters and then the result sets?

Isn't that going to cause a whole different set of painful difficulties
instead?

What it comes down to is that if we want to see output params before result
sets, but the output params are only emitted when the proc returns, then
someone has to buffer. We get to choose if it's the client or the server.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] SQL procedures

[Pavel Stehule]

2017-11-22 19:01 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 11/20/17 16:25, Andrew Dunstan wrote:
> > I've been through this fairly closely, and I think it's pretty much
> > committable. The only big item that stands out for me is the issue of
> > OUT parameters.
>
> I figured that that's something that would come up.  I had intentionally
> prohibited OUT parameters for now so that we can come up with something
> for them without having to break any backward compatibility.
>
> From reading some of the references so far, I think it could be
> sufficient to return a one-row result set and have the drivers adapt the
> returned data into their interfaces.  The only thing that would be a bit
> weird about that is that a CALL command with OUT parameters would return
> a result set and a CALL command without any OUT parameters would return
> CommandComplete instead.  Maybe that's OK.
>

T-SQL procedures returns data or OUT variables.

I remember, it was very frustrating

Maybe first result can be reserved for OUT variables, others for multi
result set

Regards

Pavel



> > Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> > here, we should probably advise using ROUTINES, as FUNCTIONS could
> > easily be take to mean "functions but not procedures".
>
> OK, I'll update the documentation a bit more.
>
> > CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> > attributes that are irrelevant to procedures in these statements. The
> > docco states "for compatibility with ALTER FUNCTION" but why do we want
> > such compatibility if it's meaningless? If we can manage it without too
> > much violence I'd prefer to see an exception raised if these are used.
>
> We can easily be more restrictive here.  I'm open to suggestions.  There
> is a difference between options that don't make sense for procedures
> (e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
> might make sense sometime, but are currently not used.  But maybe that's
> too confusing and we should just prohibit options that are not currently
> used.
>
> > In create_function.sgml, we have:
> >
> > If a schema name is included, then the function is created in the
> > specified schema.  Otherwise it is created in the current schema.
> > -   The name of the new function must not match any existing function
> > +   The name of the new function must not match any existing
> > function or procedure
> > with the same input argument types in the same schema.  However,
> > functions of different argument types can share a name (this is
> > called overloading).
> >
> > The last sentence should probably say "functions and procedures of
> > different argument types" There's a similar issue in
> create_procedure.sqml.
>
> fixing that
>
> > In grant.sgml, there is:
> >
> > +   The FUNCTION syntax also works for
> aggregate
> > +   functions.  Or use ROUTINE to refer to a
> > function,
> > +   aggregate function, or procedure regardless of what it is.
> >
> >
> > I would replace "Or" by "Alternatively,". I think it reads better that
> way.
>
> fixing that
>
> > In functions.c, there is:
> >
> > /* Should only get here for VOID functions */
> > -   Assert(fcache->rettype == VOIDOID);
> > +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> > == VOIDOID);
> > fcinfo->isnull = true;
> > result = (Datum) 0;
> >
> > The comment should also refer to procedures.
>
> fixing that
>
> > It took me a minute to work out what is going on with the new code in
> > aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.
>
> improving that
>
> > We should document where returned values in PL procedures are ignored
> > (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> > should think about possibly being more consistent here, too.
>
> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.
>
> > The context line here looks odd:
> >
> > CREATE PROCEDURE test_proc2()
> > LANGUAGE plpythonu
> > AS $$
> > return 5
> > $$;
> > CALL test_proc2();
> > ERROR:  PL/Python procedure did not return None
> > CONTEXT:  PL/Python function "test_proc2"
> >
> > Perhaps we need to change plpython_error_callback() so that "function"
> > isn't hardcoded.
>
> fixing that
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & 

Re: [HACKERS] SQL procedures

[Tom Lane]

Peter Eisentraut  writes:
> On 11/20/17 16:25, Andrew Dunstan wrote:
>> We should document where returned values in PL procedures are ignored
>> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
>> should think about possibly being more consistent here, too.

> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.

Tcl has the same approach as Perl: the return value of a proc is the
same as the value of its last command.  There's no such thing as a
proc that doesn't return a value.

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/20/17 16:25, Andrew Dunstan wrote:
> I've been through this fairly closely, and I think it's pretty much
> committable. The only big item that stands out for me is the issue of
> OUT parameters.

I figured that that's something that would come up.  I had intentionally
prohibited OUT parameters for now so that we can come up with something
for them without having to break any backward compatibility.

>From reading some of the references so far, I think it could be
sufficient to return a one-row result set and have the drivers adapt the
returned data into their interfaces.  The only thing that would be a bit
weird about that is that a CALL command with OUT parameters would return
a result set and a CALL command without any OUT parameters would return
CommandComplete instead.  Maybe that's OK.

> Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> here, we should probably advise using ROUTINES, as FUNCTIONS could
> easily be take to mean "functions but not procedures".

OK, I'll update the documentation a bit more.

> CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> attributes that are irrelevant to procedures in these statements. The
> docco states "for compatibility with ALTER FUNCTION" but why do we want
> such compatibility if it's meaningless? If we can manage it without too
> much violence I'd prefer to see an exception raised if these are used.

We can easily be more restrictive here.  I'm open to suggestions.  There
is a difference between options that don't make sense for procedures
(e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
might make sense sometime, but are currently not used.  But maybe that's
too confusing and we should just prohibit options that are not currently
used.

> In create_function.sgml, we have:
> 
>     If a schema name is included, then the function is created in the
>     specified schema.  Otherwise it is created in the current schema.
> -   The name of the new function must not match any existing function
> +   The name of the new function must not match any existing
> function or procedure
>     with the same input argument types in the same schema.  However,
>     functions of different argument types can share a name (this is
>     called overloading).
> 
> The last sentence should probably say "functions and procedures of
> different argument types" There's a similar issue in create_procedure.sqml.

fixing that

> In grant.sgml, there is:
> 
> +   The FUNCTION syntax also works for aggregate
> +   functions.  Or use ROUTINE to refer to a
> function,
> +   aggregate function, or procedure regardless of what it is.
> 
> 
> I would replace "Or" by "Alternatively,". I think it reads better that way.

fixing that

> In functions.c, there is:
> 
>     /* Should only get here for VOID functions */
> -   Assert(fcache->rettype == VOIDOID);
> +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> == VOIDOID);
>     fcinfo->isnull = true;
>     result = (Datum) 0;
> 
> The comment should also refer to procedures.

fixing that

> It took me a minute to work out what is going on with the new code in
> aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.

improving that

> We should document where returned values in PL procedures are ignored
> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> should think about possibly being more consistent here, too.

Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
disallow return values that would end up being ignored, because the only
way a return value could arise is if user code explicit calls
RETURN/return.  However, in Perl, the return value is the result of the
last statement, so prohibiting a return value would be very
inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
makes sense.  Documentation is surely needed.

> The context line here looks odd:
> 
> CREATE PROCEDURE test_proc2()
> LANGUAGE plpythonu
> AS $$
> return 5
> $$;
> CALL test_proc2();
> ERROR:  PL/Python procedure did not return None
> CONTEXT:  PL/Python function "test_proc2"
> 
> Perhaps we need to change plpython_error_callback() so that "function"
> isn't hardcoded.

fixing that

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Andrew Dunstan]

On 11/20/2017 04:25 PM, I wrote:
> I've been through this fairly closely, and I think it's pretty much
> committable. The only big item that stands out for me is the issue of
> OUT parameters.
>
> While returning multiple result sets will be a useful feature, it's also
> very common in my experience for stored procedures to have scalar out
> params as well. I'm not sure how we should go about providing for it,
> but I think we need to be sure we're not closing any doors.
>
> Here, for example, is how the MySQL stored procedure feature works with
> JDBC:
> 
> I think it will be OK if we use cursors to return multiple result sets,
> along the lines of Peter's next patch, but we shouldn't regard that as
> the end of the story. Even if we don't provide for it in this go round
> we should aim at eventually providing for stored procedure OUT params.
>
>
>



Of course it's true that we could replace a scalar OUT parameter with a
one row resultset if we have return of multiple resultsets from SPs. But
it's different from the common use pattern and a darn sight more
cumbersome to use.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Tom Lane]

Peter Eisentraut  writes:
> On 11/14/17 11:14, Tom Lane wrote:
>> ... Do we really want the existence of
>> a function foo(int) to mean that you can't create a SQL procedure named
>> foo and taking one int argument?

> Yes, that is defined that way by the SQL standard.

Meh.  OK, then it has to be one catalog.

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/14/17 11:14, Tom Lane wrote:
>> The common functionality between functions and procedures is like 98%
>> [citation needed], so they definitely belong there, even more so than
>> aggregates, for example.
> No, I don't think so.  The core reason why not is that in
> 
>   SELECT foo(...) FROM ...
> 
> foo() might be either a plain function or an aggregate, so it's important
> that functions and aggregates share the same namespace.  *That* is why
> they are in the same catalog.  On the other hand, since the above syntax
> is not usable to call a SQL procedure, putting SQL procedures into pg_proc
> just creates namespacing conflicts.  Do we really want the existence of
> a function foo(int) to mean that you can't create a SQL procedure named
> foo and taking one int argument?

Yes, that is defined that way by the SQL standard.

The point about the overlap refers more to the internals.  The entire
parsing, look-up, type-resolution, DDL handling, and other things are
the same.

So for both of these reasons I think it's appropriate to use the same
catalog.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Tom Lane]

Peter Eisentraut  writes:
> On 11/8/17 09:54, Tom Lane wrote:
>> Do procedures of this ilk belong in pg_proc at all?  It seems like a large
>> fraction of the attributes tracked in pg_proc are senseless for this
>> purpose.  A new catalog might be a better approach.

> The common functionality between functions and procedures is like 98%
> [citation needed], so they definitely belong there, even more so than
> aggregates, for example.

No, I don't think so.  The core reason why not is that in

SELECT foo(...) FROM ...

foo() might be either a plain function or an aggregate, so it's important
that functions and aggregates share the same namespace.  *That* is why
they are in the same catalog.  On the other hand, since the above syntax
is not usable to call a SQL procedure, putting SQL procedures into pg_proc
just creates namespacing conflicts.  Do we really want the existence of
a function foo(int) to mean that you can't create a SQL procedure named
foo and taking one int argument?

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/8/17 09:54, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 10/31/17 14:23, Tom Lane wrote:
>>> Why not use VOIDOID for the prorettype value?
> 
>> We need a way to distinguish functions that are callable by SELECT and
>> procedures that are callable by CALL.
> 
> Do procedures of this ilk belong in pg_proc at all?  It seems like a large
> fraction of the attributes tracked in pg_proc are senseless for this
> purpose.  A new catalog might be a better approach.

The common functionality between functions and procedures is like 98%
[citation needed], so they definitely belong there, even more so than
aggregates, for example.

> In any case, I buy none of your arguments that 0 is a better choice than a
> new pseudotype.

Well, I haven't heard any reasons for doing it differently, so I can't
judge the relative merits of either approach.  Ultimately, it would be a
minor detail as far as the code is concerned, I think.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/8/17 12:15, Merlin Moncure wrote:
> On Wed, Nov 8, 2017 at 11:03 AM, Peter Eisentraut
>  wrote:
>> On 11/8/17 11:11, Merlin Moncure wrote:
>>> On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
>>>  wrote:
 I have already submitted a separate patch that addresses these questions.
>>>
>>> Maybe I'm obtuse, but I'm not seeing it? In very interested in the
>>> general approach to transaction management; if you've described it in
>>> the patch I'll read it there.  Thanks for doing this.
>>
>> https://www.postgresql.org/message-id/178d3380-0fae-2982-00d6-c43100bc8...@2ndquadrant.com
> 
> All right, thanks.  So,

Could we have this discussion in that other thread?  This thread is just
about procedures at mostly a syntax level.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services