Обсуждение: [HACKERS] SQL procedures

I've been working on SQL procedures.  (Some might call them "stored
procedures", but I'm not aware of any procedures that are not stored, so
that's not a term that I'm using here.)

Everything that follows is intended to align with the SQL standard, at
least in spirit.

This first patch does a bunch of preparation work.  It adds the
CREATE/ALTER/DROP PROCEDURE commands and the CALL statement to call a
procedure.  It also adds ROUTINE syntax which can refer to a function or
procedure.  I have extended that to include aggregates.  And then there
is a bunch of leg work, such as psql and pg_dump support.  The
documentation is a lot of copy-and-paste right now; that can be
revisited sometime.  The provided procedural languages (an ever more
confusing term) each needed a small touch-up to handle pg_proc entries
with prorettype == 0.

Right now, there is no support for returning values from procedures via
OUT parameters.  That will need some definitional pondering; and see
also below for a possible alternative.

With this, you can write procedures that are somewhat compatible with
DB2, MySQL, and to a lesser extent Oracle.

Separately, I will send patches that implement (the beginnings of) two
separate features on top of this:

- Transaction control in procedure bodies

- Returning multiple result sets

(In various previous discussions on "real stored procedures" or
something like that, most people seemed to have one of these two
features in mind.  I think that depends on what other SQL systems one
has worked with previously.)

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 31 October 2017 at 18:23, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

> I've been working on SQL procedures.  (Some might call them "stored
> procedures", but I'm not aware of any procedures that are not stored, so
> that's not a term that I'm using here.)

I guess that the DO command might have a variant to allow you to
execute a procedure that isn't stored?

Not suggesting you implement that, just thinking about why/when the
"stored" word would be appropriate.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I've been working on SQL procedures.

No comment yet on the big picture here, but ...

> The provided procedural languages (an ever more
> confusing term) each needed a small touch-up to handle pg_proc entries
> with prorettype == 0.

Putting 0 in prorettype seems like a pretty bad idea.  Why not use VOIDOID
for the prorettype value?  Or if there is some reason why "void" isn't the
right pseudotype, maybe you should invent a new one, analogous to the
"trigger" and "event_trigger" pseudotypes.
        regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 31 October 2017 at 17:23, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I've been working on SQL procedures.  (Some might call them "stored
> procedures", but I'm not aware of any procedures that are not stored, so
> that's not a term that I'm using here.)

Looks good

> Everything that follows is intended to align with the SQL standard, at
> least in spirit.

+1

> This first patch does a bunch of preparation work.  It adds the
> CREATE/ALTER/DROP PROCEDURE commands and the CALL statement to call a
> procedure.

I guess it would be really useful to have a cut-down language to use
as an example, but its probably easier to just wait for PLpgSQL.

You mention PARALLEL SAFE is not used for procedures. Isn't it an
architectural restriction that procedures would not be able to execute
in parallel? (At least this year)

> It also adds ROUTINE syntax which can refer to a function or
> procedure.

I think we need an explanatory section of the docs, but there doesn't
seem to be one for Functions, so there is no place to add some text
that says the above.

I found it confusing that ALTER and DROP ROUTINE exists but not CREATE
ROUTINE. At very least we should say somewhere "there is no CREATE
ROUTINE", so its absence is clearly intentional. I did wonder whether
we should have it as well, but its just one less thing to review, so
good.

Was surprised that pg_dump didn't use DROP ROUTINE, when appropriate.

> I have extended that to include aggregates.  And then there
> is a bunch of leg work, such as psql and pg_dump support.  The
> documentation is a lot of copy-and-paste right now; that can be
> revisited sometime.  The provided procedural languages (an ever more
> confusing term) each needed a small touch-up to handle pg_proc entries
> with prorettype == 0.
>
> Right now, there is no support for returning values from procedures via
> OUT parameters.  That will need some definitional pondering; and see
> also below for a possible alternative.
>
> With this, you can write procedures that are somewhat compatible with
> DB2, MySQL, and to a lesser extent Oracle.
>
> Separately, I will send patches that implement (the beginnings of) two
> separate features on top of this:
>
> - Transaction control in procedure bodies
>
> - Returning multiple result sets

Both of those would be good, though my suggested priority would be
transaction control first and then multiple result sets, if we cannot
have both this release.

> (In various previous discussions on "real stored procedures" or
> something like that, most people seemed to have one of these two
> features in mind.  I think that depends on what other SQL systems one
> has worked with previously.)

Almost all of the meat happens in later patches, so no other review comments.

That seems seems strange in a patch of this size, but its true.
Procedures are just a new type of object with very little interaction
with replication, persistence or optimization.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/31/17 14:23, Tom Lane wrote:
> Putting 0 in prorettype seems like a pretty bad idea.

It seemed like the natural thing to do, since we use a zero OID to
indicate "nothing" in many other places.

> 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.

> Or if there is some reason why "void" isn't the
> right pseudotype, maybe you should invent a new one, analogous to the
> "trigger" and "event_trigger" pseudotypes.

I guess that would be doable, but I think it would make things more
complicated without any gain that I can see.  In the case of the
pseudotypes you mention, those are the actual types mentioned in the
CREATE FUNCTION command.  If we invented a new pseudotype, that would
run the risk of existing code creating nonsensical reverse compilations
like CREATE FUNCTION RETURNS PROCEDURE.  Catalog queries using
prorettype == 0 would behave sensibly by default.  For example, an inner
or outer join against pg_type would automatically make sense.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/31/17 16:50, Pavel Stehule wrote:
> Not sure if disabling RETURN is good idea. I can imagine so optional
> returning something like int status can be good idea. Cheaper than
> raising a exception.

We could allow a RETURN without argument in PL/pgSQL, if you just want
to exit early.  That syntax is currently not available, but it should
not be hard to add.

I don't understand the point about wanting to return an int.  How would
you pass that around, since there is no declared return type?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 31, 2017 at 12:23 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> - Transaction control in procedure bodies

This feature is really key, since it enables via SQL lots of things
that are not possible without external coding, including:
*) very long running processes in a single routine
*) transaction isolation control inside the procedure (currently
client app has to declare this)
*) certain error handling cases that require client side support
*) simple in-database threading
*) simple construction of daemon scripts (yeah, you can use bgworker
for this, but pure sql daemon with a cron heartbeat hook is hard to
beat for simplicity)

I do wonder how transaction control could be added later.

The last time I (lightly) looked at this, I was starting to think that
working transaction control into the SPI interface was the wrong
approach; pl/pgsql would have to adopt a very different set of
behaviors if it was called in a function or a proc.  If you restricted
language choice to purely SQL, you could work around this problem; SPI
languages would be totally abstracted from those sets of
considerations and you could always call an arbitrary language
function if you needed to.  SQL has no flow control but I'm not too
concerned about that.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 08.11.2017 17:23, Merlin Moncure wrote:
> On Tue, Oct 31, 2017 at 12:23 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> - Transaction control in procedure bodies
> This feature is really key, since it enables via SQL lots of things
> that are not possible without external coding, including:
> *) very long running processes in a single routine
> *) transaction isolation control inside the procedure (currently
> client app has to declare this)
> *) certain error handling cases that require client side support
> *) simple in-database threading
> *) simple construction of daemon scripts (yeah, you can use bgworker
> for this, but pure sql daemon with a cron heartbeat hook is hard to
> beat for simplicity)
>
> I do wonder how transaction control could be added later.
>
> The last time I (lightly) looked at this, I was starting to think that
> working transaction control into the SPI interface was the wrong
> approach; pl/pgsql would have to adopt a very different set of
> behaviors if it was called in a function or a proc.  If you restricted
> language choice to purely SQL, you could work around this problem; SPI
> languages would be totally abstracted from those sets of
> considerations and you could always call an arbitrary language
> function if you needed to.  SQL has no flow control but I'm not too
> concerned about that.
>
> merlin
>
>
I am also very interested in answer on this question: how you are going 
to implement transaction control inside procedure?
Right now in PostgresPRO EE supports autonomous transactions. Them are 
supported both for SQL and plpgsql/plpython APIs.
Them are implemented by saving/restoring transaction context, so unlike 
most of other ATX implementations, in pgpro autonomous
transaction is executed by the same backend. But it is not so easy to 
do: in Postgres almost any module have its own static variables which 
keeps transaction specific data.
So we have to provide a dozen of suspend/resume functions: 
SuspendSnapshot(),  SuspendPredicate(), SuspendStorage(), 
SuspendInvalidationInfo(), SuspendPgXact(), PgStatSuspend(), 
TriggerSuspend(), SuspendSPI()... and properly handle local cache 
invalidation. Patch consists of more than 5 thousand lines.

So my question is whether you are going to implement something similar 
or use completely different approach?
In first case it will be good to somehow unite our efforts... For 
example we can publish our ATX patch for Postgres 10.
We have not done it yet, because there seems to be no chances to push 
this patch to community.








-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/6/17 16:27, Simon Riggs wrote:
> You mention PARALLEL SAFE is not used for procedures. Isn't it an
> architectural restriction that procedures would not be able to execute
> in parallel? (At least this year)

I'm not sure what you are referring to here.  I don't think the
functionality I'm proposing does anything in parallel or has any
interaction with it.

> I think we need an explanatory section of the docs, but there doesn't
> seem to be one for Functions, so there is no place to add some text
> that says the above.
> 
> I found it confusing that ALTER and DROP ROUTINE exists but not CREATE
> ROUTINE. At very least we should say somewhere "there is no CREATE
> ROUTINE", so its absence is clearly intentional. I did wonder whether
> we should have it as well, but its just one less thing to review, so
> good.

I'll look for a place to add some documentation around this.

> Was surprised that pg_dump didn't use DROP ROUTINE, when appropriate.

It's not clear to me why that would be preferred.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/8/17 09:33, Pavel Stehule wrote:
>     We can create auto session variable STATUS. This variable can be 0
>     if procedure was returned without explicit RETURN value. Or it can
>     hold different value specified by RETURN expr.
> 
>     This value can be read by GET DIAGNOSTICS xxx = STATUS
> 
>     or some similar.
> 
> The motivation is allow some mechanism cheaper than our exceptions.

I suppose this could be a separately discussed feature.  We'd also want
to consider various things that PL/pgSQL pretends to be compatible with.

One of the main motivations for procedures is to do more complex and
expensive things including transaction control.  So saving exception
overhead is not really on the priority list there.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/8/17 09:23, Merlin Moncure wrote:
> I do wonder how transaction control could be added later.
> 
> The last time I (lightly) looked at this, I was starting to think that
> working transaction control into the SPI interface was the wrong
> approach; pl/pgsql would have to adopt a very different set of
> behaviors if it was called in a function or a proc.  If you restricted
> language choice to purely SQL, you could work around this problem; SPI
> languages would be totally abstracted from those sets of
> considerations and you could always call an arbitrary language
> function if you needed to.  SQL has no flow control but I'm not too
> concerned about that.

I have already submitted a separate patch that addresses these questions.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/8/17 11:11, Merlin Moncure wrote:
> On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> 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-c43100bc8748@2ndquadrant.com

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 8, 2017 at 11:03 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/8/17 11:11, Merlin Moncure wrote:
>> On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> 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-c43100bc8748@2ndquadrant.com

All right, thanks.  So,
*) Are you sure you want to go the SPI route?  'sql' language
(non-spi) procedures might be simpler from implementation standpoint
and do not need any language adjustments?

*) Is it possible to jump into SPI without having a snapshot already
set up. For example? If I wanted to set isolation level in a
procedure, would I get impacted by this error?
ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/8/17 12:15, Merlin Moncure wrote:
> On Wed, Nov 8, 2017 at 11:03 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 11/8/17 11:11, Merlin Moncure wrote:
>>> On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
>>> <peter.eisentraut@2ndquadrant.com> 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-c43100bc8748@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

Here is an updated patch.  It's updated for the recent documentation
format changes.  I added some more documentation as suggested by reviewers.

I also added more tests about how the various privilege commands (GRANT,
GRANT on ALL, default privileges) would work with the new object type.
I had not looked at that in much detail with the previous version of the
patch, but it seems to work the way I would have wanted without any code
changes, so it's all documentation additions and new tests.

As part of this I have also developed additional tests for how the same
privilege commands apply to aggregates, which didn't appear to be
covered yet, and I was worried that I might have broken it, which it
seems I did not.  This is included in the big patch, but I have also
included it here as a separate patch, as it could be committed
independently as additional tests for existing functionality.

It this point, this patch has no more open TODOs or
need-to-think-about-this-later's that I'm aware of.

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

On 11/8/17 09:54, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

    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?

Isn't it pretty much implied by the
ALTER | DROP ROUTINE foo(...)
commands where foo(...) may be either a procedure
or a function? It doesn't look like it could be both.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

On 14 November 2017 at 12:56, Daniel Verite <daniel@manitou-mail.org> wrote:
>         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?
>
> Isn't it pretty much implied by the
> ALTER | DROP ROUTINE foo(...)
> commands where foo(...) may be either a procedure
> or a function? It doesn't look like it could be both.

It doesn't seem particularly troublesome to create another catalog
table, if needed, so that shouldn't drive our thinking.

It would seem to be implied by the SQLStandard that Functions and
Procedures occupy the same namespace, since they are both Routines.

I can't see any benefit from having foo() function AND foo() procedure
at same time. It would certainly confuse most people that come from
programming languages without that distinction, but maybe someone
knows some Oracle-foo that I don't?

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

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

Simon Riggs wrote:
> It would seem to be implied by the SQLStandard that Functions and
> Procedures occupy the same namespace, since they are both Routines.
> 
> I can't see any benefit from having foo() function AND foo() procedure
> at same time. It would certainly confuse most people that come from
> programming languages without that distinction, but maybe someone
> knows some Oracle-foo that I don't?

The question already has been decided on the (better) basis of
the SQL standard, but for what it is worth:

Oracle has functions and procedures in the same name space.

Yours,
Laurenz Albe

On 11/14/2017 10:54 AM, Peter Eisentraut wrote:
> Here is an updated patch.  It's updated for the recent documentation
> format changes.  I added some more documentation as suggested by reviewers.
>
> I also added more tests about how the various privilege commands (GRANT,
> GRANT on ALL, default privileges) would work with the new object type.
> I had not looked at that in much detail with the previous version of the
> patch, but it seems to work the way I would have wanted without any code
> changes, so it's all documentation additions and new tests.
>
> As part of this I have also developed additional tests for how the same
> privilege commands apply to aggregates, which didn't appear to be
> covered yet, and I was worried that I might have broken it, which it
> seems I did not.  This is included in the big patch, but I have also
> included it here as a separate patch, as it could be committed
> independently as additional tests for existing functionality.
>
> It this point, this patch has no more open TODOs or
> need-to-think-about-this-later's that I'm aware of.
>



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:
<https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-usagenotes-statements-callable.html>
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.


Apart from that here are a few fairly minor nitpicks:

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".

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.

In create_function.sgml, we have:
       If a schema name is included, then the function is created in the       specified schema.  Otherwise it is
createdin the current schema.   -   The name of the new function must not match any existing function   +   The name of
thenew function must not match any existing   function or procedure       with the same input argument types in the
sameschema.  However,       functions of different argument types can share a name (this is       called
<firstterm>overloading</firstterm>).

The last sentence should probably say "functions and procedures of
different argument types" There's a similar issue in create_procedure.sqml.

In grant.sgml, there is:
   +       The <literal>FUNCTION</literal> syntax also works for aggregate   +       functions.  Or use
<literal>ROUTINE</literal>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.

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.

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.

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.

The context line here looks odd:
   CREATE PROCEDURE test_proc2()   LANGUAGE plpythonu   AS $$   return 5   $$;   CALL test_proc2();   ERROR:  PL/Python
proceduredid not return None   CONTEXT:  PL/Python function "test_proc2"
 

Perhaps we need to change plpython_error_callback() so that "function"
isn't hardcoded.

cheers

andrew

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

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:
> <https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-usagenotes-statements-callable.html>
> 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 Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 <firstterm>overloading</firstterm>).
> 
> 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 <literal>FUNCTION</literal> syntax also works for aggregate
>     +       functions.  Or use <literal>ROUTINE</literal> 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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

On 11/22/2017 01:10 PM, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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.
>
>             


Right. We probably just need to document the behaviour here. I'd be
satisfied with that.

cheers

andrew


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

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.

cheers

andrew

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

On 11/23/17 00:59, Craig Ringer wrote:
>     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?

The current PoC in the other thread puts the extra result sets in
cursors.  That's essentially the buffer you are referring to.  So it
seems possible, but there are some details to be worked out.

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

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.

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

On 29 November 2017 at 02:03, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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 Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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.

cheers

andrew

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

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

On Fri, Dec 1, 2017 at 7:48 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> 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

postgres=# \df                      List of functionsSchema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------public | bar  | integer          | i integer           |
funcpublic| foo  |                  | i integer           | proc
 
(2 rows)

Should this now be called a "List of routines"?

-- 
Thomas Munro
http://www.enterprisedb.com

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

On Fri, Dec 1, 2017 at 9:31 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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

On 2 December 2017 at 01:31, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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 Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Nov 8, 2017 at 9:21 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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

Robert Haas <robertmhaas@gmail.com> 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

On Tue, Jan 2, 2018 at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On 01/02/2018 01:45 PM, Robert Haas wrote:
> On Tue, Jan 2, 2018 at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 2, 2018 at 1:57 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> 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.

Well, that's got to be asked about any representation we choose - that
question is the motivation for not liking the use of prorettype for
this purpose, so it's only fair to ask whether any alternative has the
same problem.

> 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.

I think the catalog bloat is too minor to care about, but if these
things really are mutually exclusive, it's more natural to have them
use a single flag character rather than a series of Booleans.
Otherwise, it may be unclear to the casual observer (or hacker) that
at most one of the Booleans can be true, possibly leading to user
confusion (or bugs).

It's pretty well impossible to introduce new features without
occasionally changing the catalog representation.  We had several
people grumble when I replaced relistemp with relpersistence, and we
(rightly, IMHO) told those people to suck it up and deal.  I don't
think we should react any differently here.  I recognize that it's a
pain, but it's not that much of a pain, and it may even be helpful to
tool authors who actually need to handle procedures differently than
functions, which is probably a lot of them.  pgAdmin for example seems
like it will certainly need to care.

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

On 01/02/2018 02:14 PM, Robert Haas wrote:
> On Tue, Jan 2, 2018 at 1:57 PM, Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>> 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.
> Well, that's got to be asked about any representation we choose - that
> question is the motivation for not liking the use of prorettype for
> this purpose, so it's only fair to ask whether any alternative has the
> same problem.


I think there's broad agreement about not liking use of prorettype for
this purpose.


>
>> 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.
> I think the catalog bloat is too minor to care about, but if these
> things really are mutually exclusive, it's more natural to have them
> use a single flag character rather than a series of Booleans.
> Otherwise, it may be unclear to the casual observer (or hacker) that
> at most one of the Booleans can be true, possibly leading to user
> confusion (or bugs).



Fair point. I don't recall if we discussed anything like this when
window functions were introduced.


>
> It's pretty well impossible to introduce new features without
> occasionally changing the catalog representation.  We had several
> people grumble when I replaced relistemp with relpersistence, and we
> (rightly, IMHO) told those people to suck it up and deal.  I don't
> think we should react any differently here.  I recognize that it's a
> pain, but it's not that much of a pain, and it may even be helpful to
> tool authors who actually need to handle procedures differently than
> functions, which is probably a lot of them.  pgAdmin for example seems
> like it will certainly need to care.



I agree.


cheers

andrew

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

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

On 1/2/18 17:47, Peter Eisentraut wrote:
> 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.

Here is a patch set for that.  (I just kept the pg_proc.h changes
separate for easier viewing.)  It's not quite complete; some frontend
code still needs adjusting; but the idea is clear.

I think this would be a pretty good change.  It doesn't appear to save a
ton amount of code, although a couple of cases where inconsistent
settings of proisagg and proiswindow had to be handed could be removed.
Because window functions are a separate kind in pg_proc but not in the
object address system, inconsistencies will remain in the system, but I
guess that's just the way it is.

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

On 1/9/18 13:08, Peter Eisentraut wrote:
> On 1/2/18 17:47, Peter Eisentraut wrote:
>> 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.
> 
> Here is a patch set for that.  (I just kept the pg_proc.h changes
> separate for easier viewing.)  It's not quite complete; some frontend
> code still needs adjusting; but the idea is clear.
> 
> I think this would be a pretty good change.  It doesn't appear to save a
> ton amount of code, although a couple of cases where inconsistent
> settings of proisagg and proiswindow had to be handed could be removed.
> Because window functions are a separate kind in pg_proc but not in the
> object address system, inconsistencies will remain in the system, but I
> guess that's just the way it is.

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.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

On 2/25/18, Tom Lane <tgl@sss.pgh.pa.us> 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

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier <michael@paquier.xyz> 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.

Catalin Iacob <iacobcatalin@gmail.com> writes:
> On Tue, Feb 27, 2018 at 4:03 AM, Michael Paquier <michael@paquier.xyz> 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

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

Michael Paquier <michael@paquier.xyz> writes:
> Here is some input from me. ...

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 not touched the tab-complete.c changes.  It doesn't really make
sense to worry about that until we see what comes out of the other thread
discussing support for server-version-sensitive tab completion.  In the
meantime let's just add an open-item entry reminding us to do something
about it before v11 freezes.

Rather than manually audit the 0002 patch, I made a brain-dead little
Perl script to convert the DATA lines automatically, and attach it
below.  If pg_proc.h moves further before this gets committed, the
script could be used to generate an updated 0002 patch.

One point worth noting is that in the attached, I made some edits to
pl_gram.y to preserve the existing handling of RETURN-with-a-value
in a plpgsql procedure.  However, that existing handling is pretty
stupid.  If we simply drop the pl_gram.y diffs below, what happens
is that RETURN-with-a-value is complained of at compile time not
execution time, which seems far superior to me (and more like what
happens in a normal function returning VOID).  I think we should
do that and adjust the regression tests accordingly, but I have not
done the latter here.

I also wonder whether we should drop the stanza at the start of
pg_get_function_result() that causes it to return NULL for a procedure.
Without that, it would now return "void" which I think is at least
as defensible, and certainly more upward-compatible with any future
extension in that area.  (I did make some changes in the
information_schema that cause it to report "void" not NULL in the
same case.)

Other than those points, I think this is committable, and I think we
should get it in quickly.

            regards, tom lane

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 71e20f2..a0e6d70 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
*************** SCRAM-SHA-256$<replaceable><iteration
*** 5062,5076 ****
    </indexterm>

    <para>
!    The catalog <structname>pg_proc</structname> stores information about functions (or procedures).
!    See <xref linkend="sql-createfunction"/>
!    and <xref linkend="xfunc"/> for more information.
    </para>

    <para>
!    The table contains data for aggregate functions as well as plain functions.
!    If <structfield>proisagg</structfield> is true, there should be a matching
!    row in <structfield>pg_aggregate</structfield>.
    </para>

    <table>
--- 5062,5078 ----
    </indexterm>

    <para>
!    The catalog <structname>pg_proc</structname> stores information about
!    functions, procedures, aggregate functions, and window functions
!    (collectively also known as routines).  See <xref
!    linkend="sql-createfunction"/>, <xref linkend="sql-createprocedure"/>, and
!    <xref linkend="xfunc"/> for more information.
    </para>

    <para>
!    If <structfield>prokind</structfield> indicates that the entry is for an
!    aggregate function, there should be a matching row in
!    <structfield>pg_aggregate</structfield>.
    </para>

    <table>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 5157,5173 ****
       </row>

       <row>
!       <entry><structfield>proisagg</structfield></entry>
!       <entry><type>bool</type></entry>
!       <entry></entry>
!       <entry>Function is an aggregate function</entry>
!      </row>
!
!      <row>
!       <entry><structfield>proiswindow</structfield></entry>
!       <entry><type>bool</type></entry>
        <entry></entry>
!       <entry>Function is a window function</entry>
       </row>

       <row>
--- 5159,5170 ----
       </row>

       <row>
!       <entry><structfield>prokind</structfield></entry>
!       <entry><type>char</type></entry>
        <entry></entry>
!       <entry><literal>f</literal> for a normal function, <literal>p</literal>
!       for a procedure, <literal>a</literal> for an aggregate function, or
!       <literal>w</literal> for a window function</entry>
       </row>

       <row>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 5264,5270 ****
        <entry><structfield>prorettype</structfield></entry>
        <entry><type>oid</type></entry>
        <entry><literal><link linkend="catalog-pg-type"><structname>pg_type</structname></link>.oid</literal></entry>
!       <entry>Data type of the return value, or null for a procedure</entry>
       </row>

       <row>
--- 5261,5267 ----
        <entry><structfield>prorettype</structfield></entry>
        <entry><type>oid</type></entry>
        <entry><literal><link linkend="catalog-pg-type"><structname>pg_type</structname></link>.oid</literal></entry>
!       <entry>Data type of the return value</entry>
       </row>

       <row>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 1156627..3f2c629 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** objectsInSchemaToOids(ObjectType objtype
*** 830,850 ****
                                  BTEqualStrategyNumber, F_OIDEQ,
                                  ObjectIdGetDatum(namespaceId));

-                     /*
-                      * When looking for functions, check for return type <>0.
-                      * When looking for procedures, check for return type ==0.
-                      * When looking for routines, don't check the return type.
-                      */
                      if (objtype == OBJECT_FUNCTION)
                          ScanKeyInit(&key[keycount++],
!                                     Anum_pg_proc_prorettype,
!                                     BTEqualStrategyNumber, F_OIDNE,
!                                     InvalidOid);
                      else if (objtype == OBJECT_PROCEDURE)
                          ScanKeyInit(&key[keycount++],
!                                     Anum_pg_proc_prorettype,
!                                     BTEqualStrategyNumber, F_OIDEQ,
!                                     InvalidOid);

                      rel = heap_open(ProcedureRelationId, AccessShareLock);
                      scan = heap_beginscan_catalog(rel, keycount, key);
--- 830,846 ----
                                  BTEqualStrategyNumber, F_OIDEQ,
                                  ObjectIdGetDatum(namespaceId));

                      if (objtype == OBJECT_FUNCTION)
+                         /* includes aggregates and window functions */
                          ScanKeyInit(&key[keycount++],
!                                     Anum_pg_proc_prokind,
!                                     BTEqualStrategyNumber, F_CHARNE,
!                                     CharGetDatum(PROKIND_PROCEDURE));
                      else if (objtype == OBJECT_PROCEDURE)
                          ScanKeyInit(&key[keycount++],
!                                     Anum_pg_proc_prokind,
!                                     BTEqualStrategyNumber, F_CHAREQ,
!                                     CharGetDatum(PROKIND_PROCEDURE));

                      rel = heap_open(ProcedureRelationId, AccessShareLock);
                      scan = heap_beginscan_catalog(rel, keycount, key);
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 686528c..9a6bf1b 100644
*** a/src/backend/catalog/information_schema.sql
--- b/src/backend/catalog/information_schema.sql
*************** CREATE VIEW routines AS
*** 1413,1419 ****
             CAST(current_database() AS sql_identifier) AS routine_catalog,
             CAST(n.nspname AS sql_identifier) AS routine_schema,
             CAST(p.proname AS sql_identifier) AS routine_name,
!            CAST(CASE WHEN p.prorettype <> 0 THEN 'FUNCTION' ELSE 'PROCEDURE' END
               AS character_data) AS routine_type,
             CAST(null AS sql_identifier) AS module_catalog,
             CAST(null AS sql_identifier) AS module_schema,
--- 1413,1419 ----
             CAST(current_database() AS sql_identifier) AS routine_catalog,
             CAST(n.nspname AS sql_identifier) AS routine_schema,
             CAST(p.proname AS sql_identifier) AS routine_name,
!            CAST(CASE p.prokind WHEN 'f' THEN 'FUNCTION' WHEN 'p' THEN 'PROCEDURE' END
               AS character_data) AS routine_type,
             CAST(null AS sql_identifier) AS module_catalog,
             CAST(null AS sql_identifier) AS module_schema,
*************** CREATE VIEW routines AS
*** 1423,1430 ****
             CAST(null AS sql_identifier) AS udt_name,

             CAST(
!              CASE WHEN p.prorettype = 0 THEN NULL
!                   WHEN t.typelem <> 0 AND t.typlen = -1 THEN 'ARRAY'
                    WHEN nt.nspname = 'pg_catalog' THEN format_type(t.oid, null)
                    ELSE 'USER-DEFINED' END AS character_data)
               AS data_type,
--- 1423,1429 ----
             CAST(null AS sql_identifier) AS udt_name,

             CAST(
!              CASE WHEN t.typelem <> 0 AND t.typlen = -1 THEN 'ARRAY'
                    WHEN nt.nspname = 'pg_catalog' THEN format_type(t.oid, null)
                    ELSE 'USER-DEFINED' END AS character_data)
               AS data_type,
*************** CREATE VIEW routines AS
*** 1442,1448 ****
             CAST(null AS cardinal_number) AS datetime_precision,
             CAST(null AS character_data) AS interval_type,
             CAST(null AS cardinal_number) AS interval_precision,
!            CAST(CASE WHEN p.prorettype <> 0 THEN current_database() END AS sql_identifier) AS type_udt_catalog,
             CAST(nt.nspname AS sql_identifier) AS type_udt_schema,
             CAST(t.typname AS sql_identifier) AS type_udt_name,
             CAST(null AS sql_identifier) AS scope_catalog,
--- 1441,1447 ----
             CAST(null AS cardinal_number) AS datetime_precision,
             CAST(null AS character_data) AS interval_type,
             CAST(null AS cardinal_number) AS interval_precision,
!            CAST(current_database() AS sql_identifier) AS type_udt_catalog,
             CAST(nt.nspname AS sql_identifier) AS type_udt_schema,
             CAST(t.typname AS sql_identifier) AS type_udt_name,
             CAST(null AS sql_identifier) AS scope_catalog,
*************** CREATE VIEW routines AS
*** 1464,1470 ****
             CAST('GENERAL' AS character_data) AS parameter_style,
             CAST(CASE WHEN p.provolatile = 'i' THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_deterministic,
             CAST('MODIFIES' AS character_data) AS sql_data_access,
!            CAST(CASE WHEN p.prorettype <> 0 THEN
               CASE WHEN p.proisstrict THEN 'YES' ELSE 'NO' END END AS yes_or_no) AS is_null_call,
             CAST(null AS character_data) AS sql_path,
             CAST('YES' AS yes_or_no) AS schema_level_routine,
--- 1463,1469 ----
             CAST('GENERAL' AS character_data) AS parameter_style,
             CAST(CASE WHEN p.provolatile = 'i' THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_deterministic,
             CAST('MODIFIES' AS character_data) AS sql_data_access,
!            CAST(CASE WHEN p.prokind <> 'p' THEN
               CASE WHEN p.proisstrict THEN 'YES' ELSE 'NO' END END AS yes_or_no) AS is_null_call,
             CAST(null AS character_data) AS sql_path,
             CAST('YES' AS yes_or_no) AS schema_level_routine,
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 80f561d..119297b 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
*************** getProcedureTypeDescription(StringInfo b
*** 4047,4057 ****
          elog(ERROR, "cache lookup failed for procedure %u", procid);
      procForm = (Form_pg_proc) GETSTRUCT(procTup);

!     if (procForm->proisagg)
          appendStringInfoString(buffer, "aggregate");
!     else if (procForm->prorettype == InvalidOid)
          appendStringInfoString(buffer, "procedure");
!     else
          appendStringInfoString(buffer, "function");

      ReleaseSysCache(procTup);
--- 4047,4057 ----
          elog(ERROR, "cache lookup failed for procedure %u", procid);
      procForm = (Form_pg_proc) GETSTRUCT(procTup);

!     if (procForm->prokind == PROKIND_AGGREGATE)
          appendStringInfoString(buffer, "aggregate");
!     else if (procForm->prokind == PROKIND_PROCEDURE)
          appendStringInfoString(buffer, "procedure");
!     else                        /* function or window function */
          appendStringInfoString(buffer, "function");

      ReleaseSysCache(procTup);
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index f14ea26..50d8d81 100644
*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
*************** AggregateCreate(const char *aggName,
*** 616,623 ****
                               InvalidOid,    /* no validator */
                               "aggregate_dummy", /* placeholder proc */
                               NULL,    /* probin */
!                              true,    /* isAgg */
!                              false, /* isWindowFunc */
                               false, /* security invoker (currently not
                                       * definable for agg) */
                               false, /* isLeakProof */
--- 616,622 ----
                               InvalidOid,    /* no validator */
                               "aggregate_dummy", /* placeholder proc */
                               NULL,    /* probin */
!                              PROKIND_AGGREGATE,
                               false, /* security invoker (currently not
                                       * definable for agg) */
                               false, /* isLeakProof */
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index b59fadb..f7952b9 100644
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
*************** ProcedureCreate(const char *procedureNam
*** 74,81 ****
                  Oid languageValidator,
                  const char *prosrc,
                  const char *probin,
!                 bool isAgg,
!                 bool isWindowFunc,
                  bool security_definer,
                  bool isLeakProof,
                  bool isStrict,
--- 74,80 ----
                  Oid languageValidator,
                  const char *prosrc,
                  const char *probin,
!                 char prokind,
                  bool security_definer,
                  bool isLeakProof,
                  bool isStrict,
*************** ProcedureCreate(const char *procedureNam
*** 335,342 ****
      values[Anum_pg_proc_prorows - 1] = Float4GetDatum(prorows);
      values[Anum_pg_proc_provariadic - 1] = ObjectIdGetDatum(variadicType);
      values[Anum_pg_proc_protransform - 1] = ObjectIdGetDatum(InvalidOid);
!     values[Anum_pg_proc_proisagg - 1] = BoolGetDatum(isAgg);
!     values[Anum_pg_proc_proiswindow - 1] = BoolGetDatum(isWindowFunc);
      values[Anum_pg_proc_prosecdef - 1] = BoolGetDatum(security_definer);
      values[Anum_pg_proc_proleakproof - 1] = BoolGetDatum(isLeakProof);
      values[Anum_pg_proc_proisstrict - 1] = BoolGetDatum(isStrict);
--- 334,340 ----
      values[Anum_pg_proc_prorows - 1] = Float4GetDatum(prorows);
      values[Anum_pg_proc_provariadic - 1] = ObjectIdGetDatum(variadicType);
      values[Anum_pg_proc_protransform - 1] = ObjectIdGetDatum(InvalidOid);
!     values[Anum_pg_proc_prokind - 1] = CharGetDatum(prokind);
      values[Anum_pg_proc_prosecdef - 1] = BoolGetDatum(security_definer);
      values[Anum_pg_proc_proleakproof - 1] = BoolGetDatum(isLeakProof);
      values[Anum_pg_proc_proisstrict - 1] = BoolGetDatum(isStrict);
*************** ProcedureCreate(const char *procedureNam
*** 403,408 ****
--- 401,421 ----
              aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_FUNCTION,
                             procedureName);

+         /* Not okay to change routine kind */
+         if (oldproc->prokind != prokind)
+             ereport(ERROR,
+                     (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                      errmsg("cannot change routine type"),
+                      (oldproc->prokind == PROKIND_AGGREGATE ?
+                       errdetail("\"%s\" is an aggregate function.", procedureName) :
+                       oldproc->prokind == PROKIND_FUNCTION ?
+                       errdetail("\"%s\" is a function.", procedureName) :
+                       oldproc->prokind == PROKIND_PROCEDURE ?
+                       errdetail("\"%s\" is a procedure.", procedureName) :
+                       oldproc->prokind == PROKIND_WINDOW ?
+                       errdetail("\"%s\" is a window function.", procedureName) :
+                       0)));
+
          /*
           * Not okay to change the return type of the existing proc, since
           * existing rules, views, etc may depend on the return type.
*************** ProcedureCreate(const char *procedureNam
*** 535,568 ****
              }
          }

-         /* Can't change aggregate or window-function status, either */
-         if (oldproc->proisagg != isAgg)
-         {
-             if (oldproc->proisagg)
-                 ereport(ERROR,
-                         (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                          errmsg("function \"%s\" is an aggregate function",
-                                 procedureName)));
-             else
-                 ereport(ERROR,
-                         (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                          errmsg("function \"%s\" is not an aggregate function",
-                                 procedureName)));
-         }
-         if (oldproc->proiswindow != isWindowFunc)
-         {
-             if (oldproc->proiswindow)
-                 ereport(ERROR,
-                         (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                          errmsg("function \"%s\" is a window function",
-                                 procedureName)));
-             else
-                 ereport(ERROR,
-                         (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                          errmsg("function \"%s\" is not a window function",
-                                 procedureName)));
-         }
-
          /*
           * Do not change existing ownership or permissions, either.  Note
           * dependency-update code below has to agree with this decision.
--- 548,553 ----
*************** fmgr_sql_validator(PG_FUNCTION_ARGS)
*** 857,864 ****

      /* Disallow pseudotype result */
      /* except for RECORD, VOID, or polymorphic */
!     if (proc->prorettype &&
!         get_typtype(proc->prorettype) == TYPTYPE_PSEUDO &&
          proc->prorettype != RECORDOID &&
          proc->prorettype != VOIDOID &&
          !IsPolymorphicType(proc->prorettype))
--- 842,848 ----

      /* Disallow pseudotype result */
      /* except for RECORD, VOID, or polymorphic */
!     if (get_typtype(proc->prorettype) == TYPTYPE_PSEUDO &&
          proc->prorettype != RECORDOID &&
          proc->prorettype != VOIDOID &&
          !IsPolymorphicType(proc->prorettype))
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5652e9e..5e6e8a6 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** WHERE
*** 332,340 ****
  UNION ALL
  SELECT
      l.objoid, l.classoid, l.objsubid,
!     CASE WHEN pro.proisagg = true THEN 'aggregate'::text
!          WHEN pro.proisagg = false THEN 'function'::text
!     END AS objtype,
      pro.pronamespace AS objnamespace,
      CASE WHEN pg_function_is_visible(pro.oid)
           THEN quote_ident(pro.proname)
--- 332,342 ----
  UNION ALL
  SELECT
      l.objoid, l.classoid, l.objsubid,
!     CASE pro.prokind
!             WHEN 'a' THEN 'aggregate'::text
!             WHEN 'f' THEN 'function'::text
!             WHEN 'p' THEN 'procedure'::text
!             WHEN 'w' THEN 'window'::text END AS objtype,
      pro.pronamespace AS objnamespace,
      CASE WHEN pg_function_is_visible(pro.oid)
           THEN quote_ident(pro.proname)
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index fc4ce8d..4b38ef6 100644
*** a/src/backend/commands/dropcmds.c
--- b/src/backend/commands/dropcmds.c
*************** RemoveObjects(DropStmt *stmt)
*** 92,98 ****
           */
          if (stmt->removeType == OBJECT_FUNCTION)
          {
!             if (get_func_isagg(address.objectId))
                  ereport(ERROR,
                          (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                           errmsg("\"%s\" is an aggregate function",
--- 92,98 ----
           */
          if (stmt->removeType == OBJECT_FUNCTION)
          {
!             if (get_func_prokind(address.objectId) == PROKIND_AGGREGATE)
                  ereport(ERROR,
                          (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                           errmsg("\"%s\" is an aggregate function",
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index abdfa24..b1f87d0 100644
*** a/src/backend/commands/functioncmds.c
--- b/src/backend/commands/functioncmds.c
*************** CreateFunction(ParseState *pstate, Creat
*** 1003,1011 ****

      if (stmt->is_procedure)
      {
          Assert(!stmt->returnType);
!
!         prorettype = InvalidOid;
          returnsSet = false;
      }
      else if (stmt->returnType)
--- 1003,1014 ----

      if (stmt->is_procedure)
      {
+         /*
+          * Sometime in the future, procedures might be allowed to return
+          * results; for now, they all return VOID.
+          */
          Assert(!stmt->returnType);
!         prorettype = VOIDOID;
          returnsSet = false;
      }
      else if (stmt->returnType)
*************** CreateFunction(ParseState *pstate, Creat
*** 1097,1104 ****
                             languageValidator,
                             prosrc_str,    /* converted to text later */
                             probin_str,    /* converted to text later */
!                            false,    /* not an aggregate */
!                            isWindowFunc,
                             security,
                             isLeakProof,
                             isStrict,
--- 1100,1106 ----
                             languageValidator,
                             prosrc_str,    /* converted to text later */
                             probin_str,    /* converted to text later */
!                            stmt->is_procedure ? PROKIND_PROCEDURE : (isWindowFunc ? PROKIND_WINDOW :
PROKIND_FUNCTION),
                             security,
                             isLeakProof,
                             isStrict,
*************** RemoveFunctionById(Oid funcOid)
*** 1126,1132 ****
  {
      Relation    relation;
      HeapTuple    tup;
!     bool        isagg;

      /*
       * Delete the pg_proc tuple.
--- 1128,1134 ----
  {
      Relation    relation;
      HeapTuple    tup;
!     char        prokind;

      /*
       * Delete the pg_proc tuple.
*************** RemoveFunctionById(Oid funcOid)
*** 1137,1143 ****
      if (!HeapTupleIsValid(tup)) /* should not happen */
          elog(ERROR, "cache lookup failed for function %u", funcOid);

!     isagg = ((Form_pg_proc) GETSTRUCT(tup))->proisagg;

      CatalogTupleDelete(relation, &tup->t_self);

--- 1139,1145 ----
      if (!HeapTupleIsValid(tup)) /* should not happen */
          elog(ERROR, "cache lookup failed for function %u", funcOid);

!     prokind = ((Form_pg_proc) GETSTRUCT(tup))->prokind;

      CatalogTupleDelete(relation, &tup->t_self);

*************** RemoveFunctionById(Oid funcOid)
*** 1148,1154 ****
      /*
       * If there's a pg_aggregate tuple, delete that too.
       */
!     if (isagg)
      {
          relation = heap_open(AggregateRelationId, RowExclusiveLock);

--- 1150,1156 ----
      /*
       * If there's a pg_aggregate tuple, delete that too.
       */
!     if (prokind == PROKIND_AGGREGATE)
      {
          relation = heap_open(AggregateRelationId, RowExclusiveLock);

*************** AlterFunction(ParseState *pstate, AlterF
*** 1203,1215 ****
          aclcheck_error(ACLCHECK_NOT_OWNER, stmt->objtype,
                         NameListToString(stmt->func->objname));

!     if (procForm->proisagg)
          ereport(ERROR,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is an aggregate function",
                          NameListToString(stmt->func->objname))));

!     is_procedure = (procForm->prorettype == InvalidOid);

      /* Examine requested actions. */
      foreach(l, stmt->actions)
--- 1205,1217 ----
          aclcheck_error(ACLCHECK_NOT_OWNER, stmt->objtype,
                         NameListToString(stmt->func->objname));

!     if (procForm->prokind == PROKIND_AGGREGATE)
          ereport(ERROR,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is an aggregate function",
                          NameListToString(stmt->func->objname))));

!     is_procedure = (procForm->prokind == PROKIND_PROCEDURE);

      /* Examine requested actions. */
      foreach(l, stmt->actions)
*************** CreateCast(CreateCastStmt *stmt)
*** 1525,1538 ****
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                       errmsg("cast function must not be volatile")));
  #endif
!         if (procstruct->proisagg)
!             ereport(ERROR,
!                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                      errmsg("cast function must not be an aggregate function")));
!         if (procstruct->proiswindow)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                      errmsg("cast function must not be a window function")));
          if (procstruct->proretset)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
--- 1527,1536 ----
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                       errmsg("cast function must not be volatile")));
  #endif
!         if (procstruct->prokind != PROKIND_FUNCTION)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                      errmsg("cast function must be a normal function")));
          if (procstruct->proretset)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
*************** check_transform_function(Form_pg_proc pr
*** 1777,1790 ****
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                   errmsg("transform function must not be volatile")));
!     if (procstruct->proisagg)
!         ereport(ERROR,
!                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                  errmsg("transform function must not be an aggregate function")));
!     if (procstruct->proiswindow)
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                  errmsg("transform function must not be a window function")));
      if (procstruct->proretset)
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
--- 1775,1784 ----
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                   errmsg("transform function must not be volatile")));
!     if (procstruct->prokind != PROKIND_FUNCTION)
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
!                  errmsg("transform function must be a normal function")));
      if (procstruct->proretset)
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
diff --git a/src/backend/commands/proclang.c b/src/backend/commands/proclang.c
index 2ec9624..447bd49 100644
*** a/src/backend/commands/proclang.c
--- b/src/backend/commands/proclang.c
*************** CreateProceduralLanguage(CreatePLangStmt
*** 129,136 ****
                                        F_FMGR_C_VALIDATOR,
                                        pltemplate->tmplhandler,
                                        pltemplate->tmpllibrary,
!                                       false,    /* isAgg */
!                                       false,    /* isWindowFunc */
                                        false,    /* security_definer */
                                        false,    /* isLeakProof */
                                        false,    /* isStrict */
--- 129,135 ----
                                        F_FMGR_C_VALIDATOR,
                                        pltemplate->tmplhandler,
                                        pltemplate->tmpllibrary,
!                                       PROKIND_FUNCTION,
                                        false,    /* security_definer */
                                        false,    /* isLeakProof */
                                        false,    /* isStrict */
*************** CreateProceduralLanguage(CreatePLangStmt
*** 169,176 ****
                                            F_FMGR_C_VALIDATOR,
                                            pltemplate->tmplinline,
                                            pltemplate->tmpllibrary,
!                                           false,    /* isAgg */
!                                           false,    /* isWindowFunc */
                                            false,    /* security_definer */
                                            false,    /* isLeakProof */
                                            true, /* isStrict */
--- 168,174 ----
                                            F_FMGR_C_VALIDATOR,
                                            pltemplate->tmplinline,
                                            pltemplate->tmpllibrary,
!                                           PROKIND_FUNCTION,
                                            false,    /* security_definer */
                                            false,    /* isLeakProof */
                                            true, /* isStrict */
*************** CreateProceduralLanguage(CreatePLangStmt
*** 212,219 ****
                                            F_FMGR_C_VALIDATOR,
                                            pltemplate->tmplvalidator,
                                            pltemplate->tmpllibrary,
!                                           false,    /* isAgg */
!                                           false,    /* isWindowFunc */
                                            false,    /* security_definer */
                                            false,    /* isLeakProof */
                                            true, /* isStrict */
--- 210,216 ----
                                            F_FMGR_C_VALIDATOR,
                                            pltemplate->tmplvalidator,
                                            pltemplate->tmpllibrary,
!                                           PROKIND_FUNCTION,
                                            false,    /* security_definer */
                                            false,    /* isLeakProof */
                                            true, /* isStrict */
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 899a5c4..bf3cd3a 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** makeRangeConstructors(const char *name,
*** 1672,1679 ****
                                   F_FMGR_INTERNAL_VALIDATOR, /* language validator */
                                   prosrc[i], /* prosrc */
                                   NULL,    /* probin */
!                                  false, /* isAgg */
!                                  false, /* isWindowFunc */
                                   false, /* security_definer */
                                   false, /* leakproof */
                                   false, /* isStrict */
--- 1672,1678 ----
                                   F_FMGR_INTERNAL_VALIDATOR, /* language validator */
                                   prosrc[i], /* prosrc */
                                   NULL,    /* probin */
!                                  PROKIND_FUNCTION,
                                   false, /* security_definer */
                                   false, /* leakproof */
                                   false, /* isStrict */
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 7e249f5..78bc4ab 100644
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
*************** init_execution_state(List *queryTree_lis
*** 573,580 ****
       *
       * Note: don't set setsResult if the function returns VOID, as evidenced
       * by not having made a junkfilter.  This ensures we'll throw away any
!      * output from a utility statement that check_sql_fn_retval deemed to not
!      * have output.
       */
      if (lasttages && fcache->junkFilter)
      {
--- 573,579 ----
       *
       * Note: don't set setsResult if the function returns VOID, as evidenced
       * by not having made a junkfilter.  This ensures we'll throw away any
!      * output from the last statement in such a function.
       */
      if (lasttages && fcache->junkFilter)
      {
*************** init_sql_fcache(FmgrInfo *finfo, Oid col
*** 659,666 ****
      fcache->rettype = rettype;

      /* Fetch the typlen and byval info for the result type */
!     if (rettype)
!         get_typlenbyval(rettype, &fcache->typlen, &fcache->typbyval);

      /* Remember whether we're returning setof something */
      fcache->returnsSet = procedureStruct->proretset;
--- 658,664 ----
      fcache->rettype = rettype;

      /* Fetch the typlen and byval info for the result type */
!     get_typlenbyval(rettype, &fcache->typlen, &fcache->typbyval);

      /* Remember whether we're returning setof something */
      fcache->returnsSet = procedureStruct->proretset;
*************** fmgr_sql(PG_FUNCTION_ARGS)
*** 1324,1331 ****
          }
          else
          {
!             /* Should only get here for procedures and VOID functions */
!             Assert(fcache->rettype == InvalidOid || fcache->rettype == VOIDOID);
              fcinfo->isnull = true;
              result = (Datum) 0;
          }
--- 1322,1329 ----
          }
          else
          {
!             /* Should only get here for VOID functions and procedures */
!             Assert(fcache->rettype == VOIDOID);
              fcinfo->isnull = true;
              result = (Datum) 0;
          }
*************** check_sql_fn_retval(Oid func_id, Oid ret
*** 1549,1557 ****
      if (modifyTargetList)
          *modifyTargetList = false;    /* initialize for no change */
      if (junkFilter)
!         *junkFilter = NULL;        /* initialize in case of procedure/VOID result */

!     if (!rettype)
          return false;

      /*
--- 1547,1559 ----
      if (modifyTargetList)
          *modifyTargetList = false;    /* initialize for no change */
      if (junkFilter)
!         *junkFilter = NULL;        /* initialize in case of VOID result */

!     /*
!      * If it's declared to return VOID, we don't care what's in the function.
!      * (This takes care of the procedure case, as well.)
!      */
!     if (rettype == VOIDOID)
          return false;

      /*
*************** check_sql_fn_retval(Oid func_id, Oid ret
*** 1597,1617 ****
      else
      {
          /* Empty function body, or last statement is a utility command */
!         if (rettype && rettype != VOIDOID)
!             ereport(ERROR,
!                     (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
!                      errmsg("return type mismatch in function declared to return %s",
!                             format_type_be(rettype)),
!                      errdetail("Function's final statement must be SELECT or INSERT/UPDATE/DELETE RETURNING.")));
!         return false;
      }

      /*
       * OK, check that the targetlist returns something matching the declared
!      * type.  (We used to insist that the declared type not be VOID in this
!      * case, but that makes it hard to write a void function that exits after
!      * calling another void function.  Instead, we insist that the tlist
!      * return void ... so void is treated as if it were a scalar type below.)
       */

      /*
--- 1599,1615 ----
      else
      {
          /* Empty function body, or last statement is a utility command */
!         ereport(ERROR,
!                 (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
!                  errmsg("return type mismatch in function declared to return %s",
!                         format_type_be(rettype)),
!                  errdetail("Function's final statement must be SELECT or INSERT/UPDATE/DELETE RETURNING.")));
!         return false;            /* keep compiler quiet */
      }

      /*
       * OK, check that the targetlist returns something matching the declared
!      * type.
       */

      /*
*************** check_sql_fn_retval(Oid func_id, Oid ret
*** 1624,1631 ****
      if (fn_typtype == TYPTYPE_BASE ||
          fn_typtype == TYPTYPE_DOMAIN ||
          fn_typtype == TYPTYPE_ENUM ||
!         fn_typtype == TYPTYPE_RANGE ||
!         rettype == VOIDOID)
      {
          /*
           * For scalar-type returns, the target list must have exactly one
--- 1622,1628 ----
      if (fn_typtype == TYPTYPE_BASE ||
          fn_typtype == TYPTYPE_DOMAIN ||
          fn_typtype == TYPTYPE_ENUM ||
!         fn_typtype == TYPTYPE_RANGE)
      {
          /*
           * For scalar-type returns, the target list must have exactly one
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 89f27ce..a9a09af 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** inline_function(Oid funcid, Oid result_t
*** 4484,4495 ****

      /*
       * Forget it if the function is not SQL-language or has other showstopper
!      * properties.  (The nargs check is just paranoia.)
       */
      if (funcform->prolang != SQLlanguageId ||
          funcform->prosecdef ||
          funcform->proretset ||
-         funcform->prorettype == InvalidOid ||
          funcform->prorettype == RECORDOID ||
          !heap_attisnull(func_tuple, Anum_pg_proc_proconfig) ||
          funcform->pronargs != list_length(args))
--- 4484,4495 ----

      /*
       * Forget it if the function is not SQL-language or has other showstopper
!      * properties.  (The prokind and nargs checks are just paranoia.)
       */
      if (funcform->prolang != SQLlanguageId ||
          funcform->prosecdef ||
+         funcform->prokind != PROKIND_FUNCTION ||
          funcform->proretset ||
          funcform->prorettype == RECORDOID ||
          !heap_attisnull(func_tuple, Anum_pg_proc_proconfig) ||
          funcform->pronargs != list_length(args))
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 085aa87..665d332 100644
*** a/src/backend/parser/parse_coerce.c
--- b/src/backend/parser/parse_coerce.c
*************** build_coercion_expression(Node *node,
*** 834,841 ****
           */
          /* Assert(targetTypeId == procstruct->prorettype); */
          Assert(!procstruct->proretset);
!         Assert(!procstruct->proisagg);
!         Assert(!procstruct->proiswindow);
          nargs = procstruct->pronargs;
          Assert(nargs >= 1 && nargs <= 3);
          /* Assert(procstruct->proargtypes.values[0] == exprType(node)); */
--- 834,840 ----
           */
          /* Assert(targetTypeId == procstruct->prorettype); */
          Assert(!procstruct->proretset);
!         Assert(procstruct->prokind == PROKIND_FUNCTION);
          nargs = procstruct->pronargs;
          Assert(nargs >= 1 && nargs <= 3);
          /* Assert(procstruct->proargtypes.values[0] == exprType(node)); */
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 2a4ac09..ea5d521 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*************** func_get_detail(List *funcname,
*** 1614,1627 ****
                  *argdefaults = defaults;
              }
          }
!         if (pform->proisagg)
!             result = FUNCDETAIL_AGGREGATE;
!         else if (pform->proiswindow)
!             result = FUNCDETAIL_WINDOWFUNC;
!         else if (pform->prorettype == InvalidOid)
!             result = FUNCDETAIL_PROCEDURE;
!         else
!             result = FUNCDETAIL_NORMAL;
          ReleaseSysCache(ftup);
          return result;
      }
--- 1614,1640 ----
                  *argdefaults = defaults;
              }
          }
!
!         switch (pform->prokind)
!         {
!             case PROKIND_AGGREGATE:
!                 result = FUNCDETAIL_AGGREGATE;
!                 break;
!             case PROKIND_FUNCTION:
!                 result = FUNCDETAIL_NORMAL;
!                 break;
!             case PROKIND_PROCEDURE:
!                 result = FUNCDETAIL_PROCEDURE;
!                 break;
!             case PROKIND_WINDOW:
!                 result = FUNCDETAIL_WINDOWFUNC;
!                 break;
!             default:
!                 elog(ERROR, "unrecognized prokind: %c", pform->prokind);
!                 result = FUNCDETAIL_NORMAL; /* keep compiler quiet */
!                 break;
!         }
!
          ReleaseSysCache(ftup);
          return result;
      }
*************** LookupFuncWithArgs(ObjectType objtype, O
*** 2067,2073 ****
      if (objtype == OBJECT_FUNCTION)
      {
          /* Make sure it's a function, not a procedure */
!         if (oid && get_func_rettype(oid) == InvalidOid)
          {
              if (noError)
                  return InvalidOid;
--- 2080,2086 ----
      if (objtype == OBJECT_FUNCTION)
      {
          /* Make sure it's a function, not a procedure */
!         if (oid && get_func_prokind(oid) == PROKIND_PROCEDURE)
          {
              if (noError)
                  return InvalidOid;
*************** LookupFuncWithArgs(ObjectType objtype, O
*** 2098,2104 ****
          }

          /* Make sure it's a procedure */
!         if (get_func_rettype(oid) != InvalidOid)
          {
              if (noError)
                  return InvalidOid;
--- 2111,2117 ----
          }

          /* Make sure it's a procedure */
!         if (get_func_prokind(oid) != PROKIND_PROCEDURE)
          {
              if (noError)
                  return InvalidOid;
*************** LookupFuncWithArgs(ObjectType objtype, O
*** 2134,2140 ****
          }

          /* Make sure it's an aggregate */
!         if (!get_func_isagg(oid))
          {
              if (noError)
                  return InvalidOid;
--- 2147,2153 ----
          }

          /* Make sure it's an aggregate */
!         if (get_func_prokind(oid) != PROKIND_AGGREGATE)
          {
              if (noError)
                  return InvalidOid;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3697466..b58ee3c 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2481,2492 ****
      proc = (Form_pg_proc) GETSTRUCT(proctup);
      name = NameStr(proc->proname);

!     if (proc->proisagg)
          ereport(ERROR,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is an aggregate function", name)));

!     isfunction = (proc->prorettype != InvalidOid);

      /*
       * We always qualify the function name, to ensure the right function gets
--- 2481,2492 ----
      proc = (Form_pg_proc) GETSTRUCT(proctup);
      name = NameStr(proc->proname);

!     if (proc->prokind == PROKIND_AGGREGATE)
          ereport(ERROR,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is an aggregate function", name)));

!     isfunction = (proc->prokind != PROKIND_PROCEDURE);

      /*
       * We always qualify the function name, to ensure the right function gets
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2513,2519 ****
      /* Emit some miscellaneous options on one line */
      oldlen = buf.len;

!     if (proc->proiswindow)
          appendStringInfoString(&buf, " WINDOW");
      switch (proc->provolatile)
      {
--- 2513,2519 ----
      /* Emit some miscellaneous options on one line */
      oldlen = buf.len;

!     if (proc->prokind == PROKIND_WINDOW)
          appendStringInfoString(&buf, " WINDOW");
      switch (proc->provolatile)
      {
*************** pg_get_function_result(PG_FUNCTION_ARGS)
*** 2717,2723 ****
      if (!HeapTupleIsValid(proctup))
          PG_RETURN_NULL();

!     if (((Form_pg_proc) GETSTRUCT(proctup))->prorettype == InvalidOid)
      {
          ReleaseSysCache(proctup);
          PG_RETURN_NULL();
--- 2717,2723 ----
      if (!HeapTupleIsValid(proctup))
          PG_RETURN_NULL();

!     if (((Form_pg_proc) GETSTRUCT(proctup))->prokind == PROKIND_PROCEDURE)
      {
          ReleaseSysCache(proctup);
          PG_RETURN_NULL();
*************** print_function_arguments(StringInfo buf,
*** 2817,2823 ****
      }

      /* Check for special treatment of ordered-set aggregates */
!     if (proc->proisagg)
      {
          HeapTuple    aggtup;
          Form_pg_aggregate agg;
--- 2817,2823 ----
      }

      /* Check for special treatment of ordered-set aggregates */
!     if (proc->prokind == PROKIND_AGGREGATE)
      {
          HeapTuple    aggtup;
          Form_pg_aggregate agg;
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 51b6b4f..bba595a 100644
*** a/src/backend/utils/cache/lsyscache.c
--- b/src/backend/utils/cache/lsyscache.c
*************** func_parallel(Oid funcid)
*** 1600,1619 ****
  }

  /*
!  * get_func_isagg
!  *       Given procedure id, return the function's proisagg field.
   */
! bool
! get_func_isagg(Oid funcid)
  {
      HeapTuple    tp;
!     bool        result;

      tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
      if (!HeapTupleIsValid(tp))
          elog(ERROR, "cache lookup failed for function %u", funcid);

!     result = ((Form_pg_proc) GETSTRUCT(tp))->proisagg;
      ReleaseSysCache(tp);
      return result;
  }
--- 1600,1619 ----
  }

  /*
!  * get_func_prokind
!  *       Given procedure id, return the routine kind.
   */
! char
! get_func_prokind(Oid funcid)
  {
      HeapTuple    tp;
!     char        result;

      tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
      if (!HeapTupleIsValid(tp))
          elog(ERROR, "cache lookup failed for function %u", funcid);

!     result = ((Form_pg_proc) GETSTRUCT(tp))->prokind;
      ReleaseSysCache(tp);
      return result;
  }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2c934e6..566cbf2 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** getAggregates(Archive *fout, int *numAgg
*** 5407,5417 ****
--- 5407,5421 ----
          PQExpBuffer racl_subquery = createPQExpBuffer();
          PQExpBuffer initacl_subquery = createPQExpBuffer();
          PQExpBuffer initracl_subquery = createPQExpBuffer();
+         const char *agg_check;

          buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
                          initracl_subquery, "p.proacl", "p.proowner", "'f'",
                          dopt->binary_upgrade);

+         agg_check = (fout->remoteVersion >= 110000 ? "p.prokind = 'a'"
+                      : "p.proisagg");
+
          appendPQExpBuffer(query, "SELECT p.tableoid, p.oid, "
                            "p.proname AS aggname, "
                            "p.pronamespace AS aggnamespace, "
*************** getAggregates(Archive *fout, int *numAgg
*** 5426,5432 ****
                            "(p.oid = pip.objoid "
                            "AND pip.classoid = 'pg_proc'::regclass "
                            "AND pip.objsubid = 0) "
!                           "WHERE p.proisagg AND ("
                            "p.pronamespace != "
                            "(SELECT oid FROM pg_namespace "
                            "WHERE nspname = 'pg_catalog') OR "
--- 5430,5436 ----
                            "(p.oid = pip.objoid "
                            "AND pip.classoid = 'pg_proc'::regclass "
                            "AND pip.objsubid = 0) "
!                           "WHERE %s AND ("
                            "p.pronamespace != "
                            "(SELECT oid FROM pg_namespace "
                            "WHERE nspname = 'pg_catalog') OR "
*************** getAggregates(Archive *fout, int *numAgg
*** 5435,5441 ****
                            acl_subquery->data,
                            racl_subquery->data,
                            initacl_subquery->data,
!                           initracl_subquery->data);
          if (dopt->binary_upgrade)
              appendPQExpBufferStr(query,
                                   " OR EXISTS(SELECT 1 FROM pg_depend WHERE "
--- 5439,5446 ----
                            acl_subquery->data,
                            racl_subquery->data,
                            initacl_subquery->data,
!                           initracl_subquery->data,
!                           agg_check);
          if (dopt->binary_upgrade)
              appendPQExpBufferStr(query,
                                   " OR EXISTS(SELECT 1 FROM pg_depend WHERE "
*************** getFuncs(Archive *fout, int *numFuncs)
*** 5616,5626 ****
--- 5621,5635 ----
          PQExpBuffer racl_subquery = createPQExpBuffer();
          PQExpBuffer initacl_subquery = createPQExpBuffer();
          PQExpBuffer initracl_subquery = createPQExpBuffer();
+         const char *not_agg_check;

          buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
                          initracl_subquery, "p.proacl", "p.proowner", "'f'",
                          dopt->binary_upgrade);

+         not_agg_check = (fout->remoteVersion >= 110000 ? "p.prokind <> 'a'"
+                          : "NOT p.proisagg");
+
          appendPQExpBuffer(query,
                            "SELECT p.tableoid, p.oid, p.proname, p.prolang, "
                            "p.pronargs, p.proargtypes, p.prorettype, "
*************** getFuncs(Archive *fout, int *numFuncs)
*** 5635,5641 ****
                            "(p.oid = pip.objoid "
                            "AND pip.classoid = 'pg_proc'::regclass "
                            "AND pip.objsubid = 0) "
!                           "WHERE NOT proisagg"
                            "\n  AND NOT EXISTS (SELECT 1 FROM pg_depend "
                            "WHERE classid = 'pg_proc'::regclass AND "
                            "objid = p.oid AND deptype = 'i')"
--- 5644,5650 ----
                            "(p.oid = pip.objoid "
                            "AND pip.classoid = 'pg_proc'::regclass "
                            "AND pip.objsubid = 0) "
!                           "WHERE %s"
                            "\n  AND NOT EXISTS (SELECT 1 FROM pg_depend "
                            "WHERE classid = 'pg_proc'::regclass AND "
                            "objid = p.oid AND deptype = 'i')"
*************** getFuncs(Archive *fout, int *numFuncs)
*** 5655,5660 ****
--- 5664,5670 ----
                            initacl_subquery->data,
                            initracl_subquery->data,
                            username_subquery,
+                           not_agg_check,
                            g_last_builtin_oid,
                            g_last_builtin_oid);
          if (dopt->binary_upgrade)
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11424,11435 ****
      char       *funcargs;
      char       *funciargs;
      char       *funcresult;
-     bool        is_procedure;
      char       *proallargtypes;
      char       *proargmodes;
      char       *proargnames;
      char       *protrftypes;
!     char       *proiswindow;
      char       *provolatile;
      char       *proisstrict;
      char       *prosecdef;
--- 11434,11444 ----
      char       *funcargs;
      char       *funciargs;
      char       *funcresult;
      char       *proallargtypes;
      char       *proargmodes;
      char       *proargnames;
      char       *protrftypes;
!     char       *prokind;
      char       *provolatile;
      char       *proisstrict;
      char       *prosecdef;
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11459,11465 ****
      asPart = createPQExpBuffer();

      /* Fetch function-specific details */
!     if (fout->remoteVersion >= 90600)
      {
          /*
           * proparallel was added in 9.6
--- 11468,11493 ----
      asPart = createPQExpBuffer();

      /* Fetch function-specific details */
!     if (fout->remoteVersion >= 110000)
!     {
!         /*
!          * prokind was added in 11
!          */
!         appendPQExpBuffer(query,
!                           "SELECT proretset, prosrc, probin, "
!                           "pg_catalog.pg_get_function_arguments(oid) AS funcargs, "
!                           "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
!                           "pg_catalog.pg_get_function_result(oid) AS funcresult, "
!                           "array_to_string(protrftypes, ' ') AS protrftypes, "
!                           "prokind, provolatile, proisstrict, prosecdef, "
!                           "proleakproof, proconfig, procost, prorows, "
!                           "proparallel, "
!                           "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
!                           "FROM pg_catalog.pg_proc "
!                           "WHERE oid = '%u'::pg_catalog.oid",
!                           finfo->dobj.catId.oid);
!     }
!     else if (fout->remoteVersion >= 90600)
      {
          /*
           * proparallel was added in 9.6
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11470,11476 ****
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
                            "array_to_string(protrftypes, ' ') AS protrftypes, "
!                           "proiswindow, provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "proparallel, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
--- 11498,11505 ----
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
                            "array_to_string(protrftypes, ' ') AS protrftypes, "
!                           "CASE WHEN proiswindow THEN 'w' ELSE 'f' END AS prokind, "
!                           "provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "proparallel, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11489,11495 ****
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
                            "array_to_string(protrftypes, ' ') AS protrftypes, "
!                           "proiswindow, provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
                            "FROM pg_catalog.pg_proc "
--- 11518,11525 ----
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
                            "array_to_string(protrftypes, ' ') AS protrftypes, "
!                           "CASE WHEN proiswindow THEN 'w' ELSE 'f' END AS prokind, "
!                           "provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
                            "FROM pg_catalog.pg_proc "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11506,11512 ****
                            "pg_catalog.pg_get_function_arguments(oid) AS funcargs, "
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
!                           "proiswindow, provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
                            "FROM pg_catalog.pg_proc "
--- 11536,11543 ----
                            "pg_catalog.pg_get_function_arguments(oid) AS funcargs, "
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
!                           "CASE WHEN proiswindow THEN 'w' ELSE 'f' END AS prokind, "
!                           "provolatile, proisstrict, prosecdef, "
                            "proleakproof, proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
                            "FROM pg_catalog.pg_proc "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11524,11530 ****
                            "pg_catalog.pg_get_function_arguments(oid) AS funcargs, "
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
!                           "proiswindow, provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            " proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
--- 11555,11562 ----
                            "pg_catalog.pg_get_function_arguments(oid) AS funcargs, "
                            "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs, "
                            "pg_catalog.pg_get_function_result(oid) AS funcresult, "
!                           "CASE WHEN proiswindow THEN 'w' ELSE 'f' END AS prokind, "
!                           "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            " proconfig, procost, prorows, "
                            "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11537,11543 ****
          appendPQExpBuffer(query,
                            "SELECT proretset, prosrc, probin, "
                            "proallargtypes, proargmodes, proargnames, "
!                           "false AS proiswindow, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "proconfig, procost, prorows, "
--- 11569,11575 ----
          appendPQExpBuffer(query,
                            "SELECT proretset, prosrc, probin, "
                            "proallargtypes, proargmodes, proargnames, "
!                           "'f' AS prokind, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "proconfig, procost, prorows, "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11551,11557 ****
          appendPQExpBuffer(query,
                            "SELECT proretset, prosrc, probin, "
                            "proallargtypes, proargmodes, proargnames, "
!                           "false AS proiswindow, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "null AS proconfig, 0 AS procost, 0 AS prorows, "
--- 11583,11589 ----
          appendPQExpBuffer(query,
                            "SELECT proretset, prosrc, probin, "
                            "proallargtypes, proargmodes, proargnames, "
!                           "'f' AS prokind, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "null AS proconfig, 0 AS procost, 0 AS prorows, "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11567,11573 ****
                            "null AS proallargtypes, "
                            "null AS proargmodes, "
                            "proargnames, "
!                           "false AS proiswindow, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "null AS proconfig, 0 AS procost, 0 AS prorows, "
--- 11599,11605 ----
                            "null AS proallargtypes, "
                            "null AS proargmodes, "
                            "proargnames, "
!                           "'f' AS prokind, "
                            "provolatile, proisstrict, prosecdef, "
                            "false AS proleakproof, "
                            "null AS proconfig, 0 AS procost, 0 AS prorows, "
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11586,11596 ****
      {
          funcargs = PQgetvalue(res, 0, PQfnumber(res, "funcargs"));
          funciargs = PQgetvalue(res, 0, PQfnumber(res, "funciargs"));
!         is_procedure = PQgetisnull(res, 0, PQfnumber(res, "funcresult"));
!         if (is_procedure)
!             funcresult = NULL;
!         else
!             funcresult = PQgetvalue(res, 0, PQfnumber(res, "funcresult"));
          proallargtypes = proargmodes = proargnames = NULL;
      }
      else
--- 11618,11624 ----
      {
          funcargs = PQgetvalue(res, 0, PQfnumber(res, "funcargs"));
          funciargs = PQgetvalue(res, 0, PQfnumber(res, "funciargs"));
!         funcresult = PQgetvalue(res, 0, PQfnumber(res, "funcresult"));
          proallargtypes = proargmodes = proargnames = NULL;
      }
      else
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11599,11611 ****
          proargmodes = PQgetvalue(res, 0, PQfnumber(res, "proargmodes"));
          proargnames = PQgetvalue(res, 0, PQfnumber(res, "proargnames"));
          funcargs = funciargs = funcresult = NULL;
-         is_procedure = false;
      }
      if (PQfnumber(res, "protrftypes") != -1)
          protrftypes = PQgetvalue(res, 0, PQfnumber(res, "protrftypes"));
      else
          protrftypes = NULL;
!     proiswindow = PQgetvalue(res, 0, PQfnumber(res, "proiswindow"));
      provolatile = PQgetvalue(res, 0, PQfnumber(res, "provolatile"));
      proisstrict = PQgetvalue(res, 0, PQfnumber(res, "proisstrict"));
      prosecdef = PQgetvalue(res, 0, PQfnumber(res, "prosecdef"));
--- 11627,11638 ----
          proargmodes = PQgetvalue(res, 0, PQfnumber(res, "proargmodes"));
          proargnames = PQgetvalue(res, 0, PQfnumber(res, "proargnames"));
          funcargs = funciargs = funcresult = NULL;
      }
      if (PQfnumber(res, "protrftypes") != -1)
          protrftypes = PQgetvalue(res, 0, PQfnumber(res, "protrftypes"));
      else
          protrftypes = NULL;
!     prokind = PQgetvalue(res, 0, PQfnumber(res, "prokind"));
      provolatile = PQgetvalue(res, 0, PQfnumber(res, "provolatile"));
      proisstrict = PQgetvalue(res, 0, PQfnumber(res, "proisstrict"));
      prosecdef = PQgetvalue(res, 0, PQfnumber(res, "prosecdef"));
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11731,11737 ****

      funcsig_tag = format_function_signature(fout, finfo, false);

!     keyword = is_procedure ? "PROCEDURE" : "FUNCTION";

      appendPQExpBuffer(delqry, "DROP %s %s.%s;\n",
                        keyword,
--- 11758,11767 ----

      funcsig_tag = format_function_signature(fout, finfo, false);

!     if (prokind[0] == PROKIND_PROCEDURE)
!         keyword = "PROCEDURE";
!     else
!         keyword = "FUNCTION";    /* works for window functions too */

      appendPQExpBuffer(delqry, "DROP %s %s.%s;\n",
                        keyword,
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11744,11751 ****
                        funcfullsig ? funcfullsig :
                        funcsig);

!     if (is_procedure)
!         ;
      else if (funcresult)
          appendPQExpBuffer(q, " RETURNS %s", funcresult);
      else
--- 11774,11781 ----
                        funcfullsig ? funcfullsig :
                        funcsig);

!     if (prokind[0] == PROKIND_PROCEDURE)
!          /* no result type to output */ ;
      else if (funcresult)
          appendPQExpBuffer(q, " RETURNS %s", funcresult);
      else
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11776,11782 ****
          }
      }

!     if (proiswindow[0] == 't')
          appendPQExpBufferStr(q, " WINDOW");

      if (provolatile[0] != PROVOLATILE_VOLATILE)
--- 11806,11812 ----
          }
      }

!     if (prokind[0] == PROKIND_WINDOW)
          appendPQExpBufferStr(q, " WINDOW");

      if (provolatile[0] != PROVOLATILE_VOLATILE)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 6f74e15..1bea6ae 100644
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** qr/^GRANT SELECT ON TABLE dump_test_seco
*** 6257,6264 ****
                             prorows,
                             provariadic,
                             protransform,
!                            proisagg,
!                            proiswindow,
                             prosecdef,
                             proleakproof,
                             proisstrict,
--- 6257,6263 ----
                             prorows,
                             provariadic,
                             protransform,
!                            prokind,
                             prosecdef,
                             proleakproof,
                             proisstrict,
*************** qr/^GRANT SELECT ON TABLE dump_test_seco
*** 6290,6297 ****
          \QGRANT SELECT(prorows) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(provariadic) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(protransform) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
!         \QGRANT SELECT(proisagg) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
!         \QGRANT SELECT(proiswindow) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(prosecdef) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(proleakproof) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(proisstrict) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
--- 6289,6295 ----
          \QGRANT SELECT(prorows) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(provariadic) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(protransform) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
!         \QGRANT SELECT(prokind) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(prosecdef) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(proleakproof) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
          \QGRANT SELECT(proisstrict) ON TABLE pg_catalog.pg_proc TO PUBLIC;\E\n.*
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 466a780..0c3be1f 100644
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
*************** describeAggregates(const char *pattern,
*** 100,111 ****
                            "  pg_catalog.format_type(p.proargtypes[0], NULL) AS \"%s\",\n",
                            gettext_noop("Argument data types"));

!     appendPQExpBuffer(&buf,
!                       "  pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"\n"
!                       "FROM pg_catalog.pg_proc p\n"
!                       "     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace\n"
!                       "WHERE p.proisagg\n",
!                       gettext_noop("Description"));

      if (!showSystem && !pattern)
          appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"
--- 100,119 ----
                            "  pg_catalog.format_type(p.proargtypes[0], NULL) AS \"%s\",\n",
                            gettext_noop("Argument data types"));

!     if (pset.sversion >= 110000)
!         appendPQExpBuffer(&buf,
!                           "  pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"\n"
!                           "FROM pg_catalog.pg_proc p\n"
!                           "     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace\n"
!                           "WHERE p.prokind = 'a'\n",
!                           gettext_noop("Description"));
!     else
!         appendPQExpBuffer(&buf,
!                           "  pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"\n"
!                           "FROM pg_catalog.pg_proc p\n"
!                           "     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace\n"
!                           "WHERE p.proisagg\n",
!                           gettext_noop("Description"));

      if (!showSystem && !pattern)
          appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"
*************** describeFunctions(const char *functypes,
*** 346,359 ****
                        gettext_noop("Schema"),
                        gettext_noop("Name"));

!     if (pset.sversion >= 80400)
          appendPQExpBuffer(&buf,
                            "  pg_catalog.pg_get_function_result(p.oid) as \"%s\",\n"
                            "  pg_catalog.pg_get_function_arguments(p.oid) as \"%s\",\n"
                            " CASE\n"
                            "  WHEN p.proisagg THEN '%s'\n"
                            "  WHEN p.proiswindow THEN '%s'\n"
-                           "  WHEN p.prorettype = 0 THEN '%s'\n"
                            "  WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN '%s'\n"
                            "  ELSE '%s'\n"
                            " END as \"%s\"",
--- 354,384 ----
                        gettext_noop("Schema"),
                        gettext_noop("Name"));

!     if (pset.sversion >= 110000)
!         appendPQExpBuffer(&buf,
!                           "  pg_catalog.pg_get_function_result(p.oid) as \"%s\",\n"
!                           "  pg_catalog.pg_get_function_arguments(p.oid) as \"%s\",\n"
!                           " CASE p.prokind\n"
!                           "  WHEN 'a' THEN '%s'\n"
!                           "  WHEN 'w' THEN '%s'\n"
!                           "  WHEN 'p' THEN '%s'\n"
!                           "  ELSE '%s'\n"
!                           " END as \"%s\"",
!                           gettext_noop("Result data type"),
!                           gettext_noop("Argument data types"),
!         /* translator: "agg" is short for "aggregate" */
!                           gettext_noop("agg"),
!                           gettext_noop("window"),
!                           gettext_noop("proc"),
!                           gettext_noop("func"),
!                           gettext_noop("Type"));
!     else if (pset.sversion >= 80400)
          appendPQExpBuffer(&buf,
                            "  pg_catalog.pg_get_function_result(p.oid) as \"%s\",\n"
                            "  pg_catalog.pg_get_function_arguments(p.oid) as \"%s\",\n"
                            " CASE\n"
                            "  WHEN p.proisagg THEN '%s'\n"
                            "  WHEN p.proiswindow THEN '%s'\n"
                            "  WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN '%s'\n"
                            "  ELSE '%s'\n"
                            " END as \"%s\"",
*************** describeFunctions(const char *functypes,
*** 362,368 ****
          /* translator: "agg" is short for "aggregate" */
                            gettext_noop("agg"),
                            gettext_noop("window"),
-                           gettext_noop("proc"),
                            gettext_noop("trigger"),
                            gettext_noop("func"),
                            gettext_noop("Type"));
--- 387,392 ----
*************** describeFunctions(const char *functypes,
*** 494,500 ****
                  appendPQExpBufferStr(&buf, "WHERE ");
                  have_where = true;
              }
!             appendPQExpBufferStr(&buf, "NOT p.proisagg\n");
          }
          if (!showTrigger)
          {
--- 518,527 ----
                  appendPQExpBufferStr(&buf, "WHERE ");
                  have_where = true;
              }
!             if (pset.sversion >= 110000)
!                 appendPQExpBufferStr(&buf, "p.prokind <> 'a'\n");
!             else
!                 appendPQExpBufferStr(&buf, "NOT p.proisagg\n");
          }
          if (!showTrigger)
          {
*************** describeFunctions(const char *functypes,
*** 516,522 ****
                  appendPQExpBufferStr(&buf, "WHERE ");
                  have_where = true;
              }
!             appendPQExpBufferStr(&buf, "NOT p.proiswindow\n");
          }
      }
      else
--- 543,552 ----
                  appendPQExpBufferStr(&buf, "WHERE ");
                  have_where = true;
              }
!             if (pset.sversion >= 110000)
!                 appendPQExpBufferStr(&buf, "p.prokind <> 'w'\n");
!             else
!                 appendPQExpBufferStr(&buf, "NOT p.proiswindow\n");
          }
      }
      else
*************** describeFunctions(const char *functypes,
*** 528,534 ****
          /* Note: at least one of these must be true ... */
          if (showAggregate)
          {
!             appendPQExpBufferStr(&buf, "p.proisagg\n");
              needs_or = true;
          }
          if (showTrigger)
--- 558,567 ----
          /* Note: at least one of these must be true ... */
          if (showAggregate)
          {
!             if (pset.sversion >= 110000)
!                 appendPQExpBufferStr(&buf, "p.prokind = 'a'\n");
!             else
!                 appendPQExpBufferStr(&buf, "p.proisagg\n");
              needs_or = true;
          }
          if (showTrigger)
*************** describeFunctions(const char *functypes,
*** 543,549 ****
          {
              if (needs_or)
                  appendPQExpBufferStr(&buf, "       OR ");
!             appendPQExpBufferStr(&buf, "p.proiswindow\n");
              needs_or = true;
          }
          appendPQExpBufferStr(&buf, "      )\n");
--- 576,585 ----
          {
              if (needs_or)
                  appendPQExpBufferStr(&buf, "       OR ");
!             if (pset.sversion >= 110000)
!                 appendPQExpBufferStr(&buf, "p.prokind = 'w'\n");
!             else
!                 appendPQExpBufferStr(&buf, "p.proiswindow\n");
              needs_or = true;
          }
          appendPQExpBufferStr(&buf, "      )\n");
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 8bc4a19..cf23d13 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** static const SchemaQuery Query_for_list_
*** 349,355 ****
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.proisagg",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
--- 349,355 ----
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.prokind = 'a'",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
*************** static const SchemaQuery Query_for_list_
*** 397,403 ****
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.prorettype <> 0",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
--- 397,403 ----
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.prokind IN ('f', 'w')",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
*************** static const SchemaQuery Query_for_list_
*** 428,434 ****
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.prorettype = 0",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
--- 428,434 ----
      /* catname */
      "pg_catalog.pg_proc p",
      /* selcondition */
!     "p.prokind = 'p'",
      /* viscondition */
      "pg_catalog.pg_function_is_visible(p.oid)",
      /* namespace */
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 26b1866..01cf59e 100644
*** a/src/include/catalog/pg_class.h
--- b/src/include/catalog/pg_class.h
*************** DATA(insert OID = 1247 (  pg_type        PGNSP
*** 151,157 ****
  DESCR("");
  DATA(insert OID = 1249 (  pg_attribute    PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 22 0 f f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
! DATA(insert OID = 1255 (  pg_proc        PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 29 0 t f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
  DATA(insert OID = 1259 (  pg_class        PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 33 0 t f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
--- 151,157 ----
  DESCR("");
  DATA(insert OID = 1249 (  pg_attribute    PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 22 0 f f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
! DATA(insert OID = 1255 (  pg_proc        PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 28 0 t f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
  DATA(insert OID = 1259 (  pg_class        PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 33 0 t f f f f f f t n f 3 1 _null_
_null__null_)); 
  DESCR("");
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index c00d055..b25c918 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** CATALOG(pg_proc,1255) BKI_BOOTSTRAP BKI_
*** 43,50 ****
      float4        prorows;        /* estimated # of rows out (if proretset) */
      Oid            provariadic;    /* element type of variadic array, or 0 */
      regproc        protransform;    /* transforms calls to it during planning */
!     bool        proisagg;        /* is it an aggregate? */
!     bool        proiswindow;    /* is it a window function? */
      bool        prosecdef;        /* security definer */
      bool        proleakproof;    /* is it a leak-proof function? */
      bool        proisstrict;    /* strict with respect to NULLs? */
--- 43,49 ----
      float4        prorows;        /* estimated # of rows out (if proretset) */
      Oid            provariadic;    /* element type of variadic array, or 0 */
      regproc        protransform;    /* transforms calls to it during planning */
!     char        prokind;        /* see PROKIND_ categories below */
      bool        prosecdef;        /* security definer */
      bool        proleakproof;    /* is it a leak-proof function? */
      bool        proisstrict;    /* strict with respect to NULLs? */
*************** typedef FormData_pg_proc *Form_pg_proc;
*** 86,92 ****
   *        compiler constants for pg_proc
   * ----------------
   */
! #define Natts_pg_proc                    29
  #define Anum_pg_proc_proname            1
  #define Anum_pg_proc_pronamespace        2
  #define Anum_pg_proc_proowner            3
--- 85,91 ----
   *        compiler constants for pg_proc
   * ----------------
   */
! #define Natts_pg_proc                    28
  #define Anum_pg_proc_proname            1
  #define Anum_pg_proc_pronamespace        2
  #define Anum_pg_proc_proowner            3
*************** typedef FormData_pg_proc *Form_pg_proc;
*** 95,121 ****
  #define Anum_pg_proc_prorows            6
  #define Anum_pg_proc_provariadic        7
  #define Anum_pg_proc_protransform        8
! #define Anum_pg_proc_proisagg            9
! #define Anum_pg_proc_proiswindow        10
! #define Anum_pg_proc_prosecdef            11
! #define Anum_pg_proc_proleakproof        12
! #define Anum_pg_proc_proisstrict        13
! #define Anum_pg_proc_proretset            14
! #define Anum_pg_proc_provolatile        15
! #define Anum_pg_proc_proparallel        16
! #define Anum_pg_proc_pronargs            17
! #define Anum_pg_proc_pronargdefaults    18
! #define Anum_pg_proc_prorettype            19
! #define Anum_pg_proc_proargtypes        20
! #define Anum_pg_proc_proallargtypes        21
! #define Anum_pg_proc_proargmodes        22
! #define Anum_pg_proc_proargnames        23
! #define Anum_pg_proc_proargdefaults        24
! #define Anum_pg_proc_protrftypes        25
! #define Anum_pg_proc_prosrc                26
! #define Anum_pg_proc_probin                27
! #define Anum_pg_proc_proconfig            28
! #define Anum_pg_proc_proacl                29

  /* ----------------
   *        initial contents of pg_proc
--- 94,119 ----
  #define Anum_pg_proc_prorows            6
  #define Anum_pg_proc_provariadic        7
  #define Anum_pg_proc_protransform        8
! #define Anum_pg_proc_prokind            9
! #define Anum_pg_proc_prosecdef            10
! #define Anum_pg_proc_proleakproof        11
! #define Anum_pg_proc_proisstrict        12
! #define Anum_pg_proc_proretset            13
! #define Anum_pg_proc_provolatile        14
! #define Anum_pg_proc_proparallel        15
! #define Anum_pg_proc_pronargs            16
! #define Anum_pg_proc_pronargdefaults    17
! #define Anum_pg_proc_prorettype            18
! #define Anum_pg_proc_proargtypes        19
! #define Anum_pg_proc_proallargtypes        20
! #define Anum_pg_proc_proargmodes        21
! #define Anum_pg_proc_proargnames        22
! #define Anum_pg_proc_proargdefaults        23
! #define Anum_pg_proc_protrftypes        24
! #define Anum_pg_proc_prosrc                25
! #define Anum_pg_proc_probin                26
! #define Anum_pg_proc_proconfig            27
! #define Anum_pg_proc_proacl                28

  /* ----------------
   *        initial contents of pg_proc
*************** DATA(insert OID = 5028 ( satisfies_hash_
*** 5576,5581 ****
--- 5574,5587 ----
  DESCR("hash partition CHECK constraint");

  /*
+  * Symbolic values for prokind column
+  */
+ #define PROKIND_FUNCTION 'f'
+ #define PROKIND_AGGREGATE 'a'
+ #define PROKIND_WINDOW 'w'
+ #define PROKIND_PROCEDURE 'p'
+
+ /*
   * Symbolic values for provolatile column: these indicate whether the result
   * of a function is dependent *only* on the values of its explicit arguments,
   * or can change due to outside factors (such as parameter variables or
diff --git a/src/include/catalog/pg_proc_fn.h b/src/include/catalog/pg_proc_fn.h
index 098e2e6..b66871b 100644
*** a/src/include/catalog/pg_proc_fn.h
--- b/src/include/catalog/pg_proc_fn.h
*************** extern ObjectAddress ProcedureCreate(con
*** 27,34 ****
                  Oid languageValidator,
                  const char *prosrc,
                  const char *probin,
!                 bool isAgg,
!                 bool isWindowFunc,
                  bool security_definer,
                  bool isLeakProof,
                  bool isStrict,
--- 27,33 ----
                  Oid languageValidator,
                  const char *prosrc,
                  const char *probin,
!                 char prokind,
                  bool security_definer,
                  bool isLeakProof,
                  bool isStrict,
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 1f6c04a..e55ea40 100644
*** a/src/include/utils/lsyscache.h
--- b/src/include/utils/lsyscache.h
*************** extern bool get_func_retset(Oid funcid);
*** 117,123 ****
  extern bool func_strict(Oid funcid);
  extern char func_volatile(Oid funcid);
  extern char func_parallel(Oid funcid);
! extern bool get_func_isagg(Oid funcid);
  extern bool get_func_leakproof(Oid funcid);
  extern float4 get_func_cost(Oid funcid);
  extern float4 get_func_rows(Oid funcid);
--- 117,123 ----
  extern bool func_strict(Oid funcid);
  extern char func_volatile(Oid funcid);
  extern char func_parallel(Oid funcid);
! extern char get_func_prokind(Oid funcid);
  extern bool get_func_leakproof(Oid funcid);
  extern float4 get_func_cost(Oid funcid);
  extern float4 get_func_rows(Oid funcid);
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 77c41b2..fa8e2fd 100644
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
*************** compile_plperl_function(Oid fn_oid, bool
*** 2832,2838 ****
           * Get the required information for input conversion of the
           * return value.
           ************************************************************/
!         if (!is_trigger && !is_event_trigger && procStruct->prorettype)
          {
              Oid            rettype = procStruct->prorettype;

--- 2832,2839 ----
           * Get the required information for input conversion of the
           * return value.
           ************************************************************/
!         if (!is_trigger && !is_event_trigger &&
!             procStruct->prokind != PROKIND_PROCEDURE)
          {
              Oid            rettype = procStruct->prorettype;

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index d07a16a..6fbbc4a 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** do_compile(FunctionCallInfo fcinfo,
*** 275,280 ****
--- 275,281 ----
      bool        isnull;
      char       *proc_source;
      HeapTuple    typeTup;
+     Form_pg_type typeStruct;
      PLpgSQL_variable *var;
      PLpgSQL_rec *rec;
      int            i;
*************** do_compile(FunctionCallInfo fcinfo,
*** 365,370 ****
--- 366,373 ----
      else
          function->fn_is_trigger = PLPGSQL_NOT_TRIGGER;

+     function->fn_prokind = procStruct->prokind;
+
      /*
       * Initialize the compiler, particularly the namespace stack.  The
       * outermost namespace contains function parameters and other special
*************** do_compile(FunctionCallInfo fcinfo,
*** 529,538 ****
              /*
               * Lookup the function's return type
               */
-             if (rettypeid)
-             {
-                 Form_pg_type typeStruct;
-
                  typeTup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(rettypeid));
                  if (!HeapTupleIsValid(typeTup))
                      elog(ERROR, "cache lookup failed for type %u", rettypeid);
--- 532,537 ----
*************** do_compile(FunctionCallInfo fcinfo,
*** 577,583 ****
                  }

                  ReleaseSysCache(typeTup);
-             }
              break;

          case PLPGSQL_DML_TRIGGER:
--- 576,581 ----
*************** plpgsql_compile_inline(char *proc_source
*** 890,895 ****
--- 888,894 ----
      function->fn_retset = false;
      function->fn_retistuple = false;
      function->fn_retisdomain = false;
+     function->fn_prokind = PROKIND_FUNCTION;
      /* a bit of hardwired knowledge about type VOID here */
      function->fn_retbyval = true;
      function->fn_rettyplen = sizeof(int32);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4ff87e0..297aa3e 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** plpgsql_exec_function(PLpgSQL_function *
*** 573,579 ****
      estate.err_text = NULL;
      estate.err_stmt = (PLpgSQL_stmt *) (func->action);
      rc = exec_stmt_block(&estate, func->action);
!     if (rc != PLPGSQL_RC_RETURN && func->fn_rettype)
      {
          estate.err_stmt = NULL;
          estate.err_text = NULL;
--- 573,579 ----
      estate.err_text = NULL;
      estate.err_stmt = (PLpgSQL_stmt *) (func->action);
      rc = exec_stmt_block(&estate, func->action);
!     if (rc != PLPGSQL_RC_RETURN)
      {
          estate.err_stmt = NULL;
          estate.err_text = NULL;
*************** plpgsql_exec_function(PLpgSQL_function *
*** 617,625 ****
      }
      else if (!estate.retisnull)
      {
!         if (!func->fn_rettype)
              ereport(ERROR,
!                     (errmsg("cannot return a value from a procedure")));

          /*
           * Cast result value to function's declared result type, and copy it
--- 617,626 ----
      }
      else if (!estate.retisnull)
      {
!         if (func->fn_prokind == PROKIND_PROCEDURE)
              ereport(ERROR,
!                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                      errmsg("cannot return a value from a procedure")));

          /*
           * Cast result value to function's declared result type, and copy it
*************** exec_stmt_return(PLpgSQL_execstate *esta
*** 2956,2964 ****
      /*
       * Special hack for function returning VOID: instead of NULL, return a
       * non-null VOID value.  This is of dubious importance but is kept for
!      * backwards compatibility.
       */
!     if (estate->fn_rettype == VOIDOID)
      {
          estate->retval = (Datum) 0;
          estate->retisnull = false;
--- 2957,2966 ----
      /*
       * Special hack for function returning VOID: instead of NULL, return a
       * non-null VOID value.  This is of dubious importance but is kept for
!      * backwards compatibility.  We don't do it for procedures, though.
       */
!     if (estate->fn_rettype == VOIDOID &&
!         estate->func->fn_prokind != PROKIND_PROCEDURE)
      {
          estate->retval = (Datum) 0;
          estate->retisnull = false;
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 688fbd6..697ead0 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***************
*** 16,21 ****
--- 16,22 ----
  #include "postgres.h"

  #include "catalog/namespace.h"
+ #include "catalog/pg_proc.h"
  #include "catalog/pg_type.h"
  #include "parser/parser.h"
  #include "parser/parse_type.h"
*************** make_return_stmt(int location)
*** 3137,3143 ****
                       parser_errposition(yylloc)));
          new->retvarno = plpgsql_curr_compile->out_param_varno;
      }
!     else if (plpgsql_curr_compile->fn_rettype == VOIDOID)
      {
          if (yylex() != ';')
              ereport(ERROR,
--- 3138,3145 ----
                       parser_errposition(yylloc)));
          new->retvarno = plpgsql_curr_compile->out_param_varno;
      }
!     else if (plpgsql_curr_compile->fn_rettype == VOIDOID &&
!              plpgsql_curr_compile->fn_prokind != PROKIND_PROCEDURE)
      {
          if (yylex() != ';')
              ereport(ERROR,
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 26a7344..dd59036 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_function
*** 918,923 ****
--- 918,924 ----
      bool        fn_retisdomain;
      bool        fn_retset;
      bool        fn_readonly;
+     char        fn_prokind;

      int            fn_nargs;
      int            fn_argvarnos[FUNC_MAX_ARGS];
diff --git a/src/pl/plpython/plpy_procedure.c b/src/pl/plpython/plpy_procedure.c
index 4e06413..82cc3f2 100644
*** a/src/pl/plpython/plpy_procedure.c
--- b/src/pl/plpython/plpy_procedure.c
*************** PLy_procedure_create(HeapTuple procTup,
*** 188,194 ****
          proc->fn_tid = procTup->t_self;
          proc->fn_readonly = (procStruct->provolatile != PROVOLATILE_VOLATILE);
          proc->is_setof = procStruct->proretset;
!         proc->is_procedure = (procStruct->prorettype == InvalidOid);
          proc->src = NULL;
          proc->argnames = NULL;
          proc->args = NULL;
--- 188,194 ----
          proc->fn_tid = procTup->t_self;
          proc->fn_readonly = (procStruct->provolatile != PROVOLATILE_VOLATILE);
          proc->is_setof = procStruct->proretset;
!         proc->is_procedure = (procStruct->prokind == PROKIND_PROCEDURE);
          proc->src = NULL;
          proc->argnames = NULL;
          proc->args = NULL;
*************** PLy_procedure_create(HeapTuple procTup,
*** 208,214 ****
           * get information required for output conversion of the return value,
           * but only if this isn't a trigger or procedure.
           */
!         if (!is_trigger && procStruct->prorettype)
          {
              Oid            rettype = procStruct->prorettype;
              HeapTuple    rvTypeTup;
--- 208,214 ----
           * get information required for output conversion of the return value,
           * but only if this isn't a trigger or procedure.
           */
!         if (!is_trigger && procStruct->prokind != PROKIND_PROCEDURE)
          {
              Oid            rettype = procStruct->prorettype;
              HeapTuple    rvTypeTup;
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 5df4dfd..2eb6c33 100644
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
*************** compile_pltcl_function(Oid fn_oid, Oid t
*** 1523,1531 ****
           * Get the required information for input conversion of the
           * return value.
           ************************************************************/
!         prodesc->fn_is_procedure = (procStruct->prorettype == InvalidOid);

!         if (!is_trigger && !is_event_trigger && procStruct->prorettype)
          {
              Oid            rettype = procStruct->prorettype;

--- 1523,1531 ----
           * Get the required information for input conversion of the
           * return value.
           ************************************************************/
!         prodesc->fn_is_procedure = (procStruct->prokind == PROKIND_PROCEDURE);

!         if (!is_trigger && !is_event_trigger && !prodesc->fn_is_procedure)
          {
              Oid            rettype = procStruct->prorettype;

diff --git a/src/test/regress/expected/alter_generic.out b/src/test/regress/expected/alter_generic.out
index 44356de..3e40df1 100644
*** a/src/test/regress/expected/alter_generic.out
--- b/src/test/regress/expected/alter_generic.out
*************** ERROR:  must be owner of function alt_ag
*** 83,103 ****
  ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2;  -- failed (name conflict)
  ERROR:  function alt_agg2(integer) already exists in schema "alt_nsp2"
  RESET SESSION AUTHORIZATION;
! SELECT n.nspname, proname, prorettype::regtype, proisagg, a.rolname
    FROM pg_proc p, pg_namespace n, pg_authid a
    WHERE p.pronamespace = n.oid AND p.proowner = a.oid
      AND n.nspname IN ('alt_nsp1', 'alt_nsp2')
    ORDER BY nspname, proname;
!  nspname  |  proname  | prorettype | proisagg |       rolname
! ----------+-----------+------------+----------+---------------------
!  alt_nsp1 | alt_agg2  | integer    | t        | regress_alter_user2
!  alt_nsp1 | alt_agg3  | integer    | t        | regress_alter_user1
!  alt_nsp1 | alt_agg4  | integer    | t        | regress_alter_user2
!  alt_nsp1 | alt_func2 | integer    | f        | regress_alter_user2
!  alt_nsp1 | alt_func3 | integer    | f        | regress_alter_user1
!  alt_nsp1 | alt_func4 | integer    | f        | regress_alter_user2
!  alt_nsp2 | alt_agg2  | integer    | t        | regress_alter_user3
!  alt_nsp2 | alt_func2 | integer    | f        | regress_alter_user3
  (8 rows)

  --
--- 83,103 ----
  ALTER AGGREGATE alt_agg2(int) SET SCHEMA alt_nsp2;  -- failed (name conflict)
  ERROR:  function alt_agg2(integer) already exists in schema "alt_nsp2"
  RESET SESSION AUTHORIZATION;
! SELECT n.nspname, proname, prorettype::regtype, prokind, a.rolname
    FROM pg_proc p, pg_namespace n, pg_authid a
    WHERE p.pronamespace = n.oid AND p.proowner = a.oid
      AND n.nspname IN ('alt_nsp1', 'alt_nsp2')
    ORDER BY nspname, proname;
!  nspname  |  proname  | prorettype | prokind |       rolname
! ----------+-----------+------------+---------+---------------------
!  alt_nsp1 | alt_agg2  | integer    | a       | regress_alter_user2
!  alt_nsp1 | alt_agg3  | integer    | a       | regress_alter_user1
!  alt_nsp1 | alt_agg4  | integer    | a       | regress_alter_user2
!  alt_nsp1 | alt_func2 | integer    | f       | regress_alter_user2
!  alt_nsp1 | alt_func3 | integer    | f       | regress_alter_user1
!  alt_nsp1 | alt_func4 | integer    | f       | regress_alter_user2
!  alt_nsp2 | alt_agg2  | integer    | a       | regress_alter_user3
!  alt_nsp2 | alt_func2 | integer    | f       | regress_alter_user3
  (8 rows)

  --
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index 5ff1e0d..3cdd92c 100644
*** a/src/test/regress/expected/create_function_3.out
--- b/src/test/regress/expected/create_function_3.out
*************** ERROR:  could not find a function named
*** 271,276 ****
--- 271,285 ----
  DROP FUNCTION functest_b_2;  -- error, ambiguous
  ERROR:  function name "functest_b_2" is not unique
  HINT:  Specify the argument list to select the function unambiguously.
+ -- CREATE OR REPLACE tests
+ CREATE FUNCTION functest1(a int) RETURNS int LANGUAGE SQL AS 'SELECT $1';
+ CREATE OR REPLACE FUNCTION functest1(a int) RETURNS int LANGUAGE SQL WINDOW AS 'SELECT $1';
+ ERROR:  cannot change routine type
+ DETAIL:  "functest1" is a function.
+ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1';
+ ERROR:  cannot change routine type
+ DETAIL:  "functest1" is a function.
+ DROP FUNCTION functest1(a int);
  -- Cleanups
  DROP SCHEMA temp_func_test CASCADE;
  NOTICE:  drop cascades to 16 other objects
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 6616cc1..01608d2 100644
*** a/src/test/regress/expected/opr_sanity.out
--- b/src/test/regress/expected/opr_sanity.out
*************** WHERE p1.prolang = 0 OR p1.prorettype =
*** 74,79 ****
--- 74,80 ----
         0::oid = ANY (p1.proargtypes) OR
         procost <= 0 OR
         CASE WHEN proretset THEN prorows <= 0 ELSE prorows != 0 END OR
+        prokind NOT IN ('f', 'a', 'w', 'p') OR
         provolatile NOT IN ('i', 's', 'v') OR
         proparallel NOT IN ('s', 'r', 'u');
   oid | proname
*************** WHERE prosrc IS NULL OR prosrc = '' OR p
*** 88,97 ****
  -----+---------
  (0 rows)

! -- proiswindow shouldn't be set together with proisagg or proretset
  SELECT p1.oid, p1.proname
  FROM pg_proc AS p1
! WHERE proiswindow AND (proisagg OR proretset);
   oid | proname
  -----+---------
  (0 rows)
--- 89,98 ----
  -----+---------
  (0 rows)

! -- proretset should only be set for normal functions
  SELECT p1.oid, p1.proname
  FROM pg_proc AS p1
! WHERE proretset AND prokind != 'f';
   oid | proname
  -----+---------
  (0 rows)
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 154,162 ****
  WHERE p1.oid < p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     (p1.proisagg = false OR p2.proisagg = false) AND
      (p1.prolang != p2.prolang OR
!      p1.proisagg != p2.proisagg OR
       p1.prosecdef != p2.prosecdef OR
       p1.proleakproof != p2.proleakproof OR
       p1.proisstrict != p2.proisstrict OR
--- 155,163 ----
  WHERE p1.oid < p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     (p1.prokind != 'a' OR p2.prokind != 'a') AND
      (p1.prolang != p2.prolang OR
!      p1.prokind != p2.prokind OR
       p1.prosecdef != p2.prosecdef OR
       p1.proleakproof != p2.proleakproof OR
       p1.proisstrict != p2.proisstrict OR
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 182,188 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.prorettype < p2.prorettype)
--- 183,189 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.prorettype < p2.prorettype)
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 198,204 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[0] < p2.proargtypes[0])
--- 199,205 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[0] < p2.proargtypes[0])
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 216,222 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[1] < p2.proargtypes[1])
--- 217,223 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[1] < p2.proargtypes[1])
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 233,239 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[2] < p2.proargtypes[2])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 234,240 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[2] < p2.proargtypes[2])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 246,252 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[3] < p2.proargtypes[3])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 247,253 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[3] < p2.proargtypes[3])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 259,265 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[4] < p2.proargtypes[4])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 260,266 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[4] < p2.proargtypes[4])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 271,277 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[5] < p2.proargtypes[5])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 272,278 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[5] < p2.proargtypes[5])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 283,289 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[6] < p2.proargtypes[6])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 284,290 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[6] < p2.proargtypes[6])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 295,301 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[7] < p2.proargtypes[7])
  ORDER BY 1, 2;
   proargtypes | proargtypes
--- 296,302 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[7] < p2.proargtypes[7])
  ORDER BY 1, 2;
   proargtypes | proargtypes
*************** WHERE aggfnoid = 0 OR aggtransfn = 0 OR
*** 1292,1306 ****
  SELECT a.aggfnoid::oid, p.proname
  FROM pg_aggregate as a, pg_proc as p
  WHERE a.aggfnoid = p.oid AND
!     (NOT p.proisagg OR p.proretset OR p.pronargs < a.aggnumdirectargs);
   aggfnoid | proname
  ----------+---------
  (0 rows)

! -- Make sure there are no proisagg pg_proc entries without matches.
  SELECT oid, proname
  FROM pg_proc as p
! WHERE p.proisagg AND
      NOT EXISTS (SELECT 1 FROM pg_aggregate a WHERE a.aggfnoid = p.oid);
   oid | proname
  -----+---------
--- 1293,1307 ----
  SELECT a.aggfnoid::oid, p.proname
  FROM pg_aggregate as a, pg_proc as p
  WHERE a.aggfnoid = p.oid AND
!     (p.prokind != 'a' OR p.proretset OR p.pronargs < a.aggnumdirectargs);
   aggfnoid | proname
  ----------+---------
  (0 rows)

! -- Make sure there are no prokind = PROKIND_AGGREGATE pg_proc entries without matches.
  SELECT oid, proname
  FROM pg_proc as p
! WHERE p.prokind = 'a' AND
      NOT EXISTS (SELECT 1 FROM pg_aggregate a WHERE a.aggfnoid = p.oid);
   oid | proname
  -----+---------
*************** ORDER BY 1, 2;
*** 1639,1645 ****
  SELECT p1.oid::regprocedure, p2.oid::regprocedure
  FROM pg_proc AS p1, pg_proc AS p2
  WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
!     p1.proisagg AND p2.proisagg AND
      array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
  ORDER BY 1;
       oid      |   oid
--- 1640,1646 ----
  SELECT p1.oid::regprocedure, p2.oid::regprocedure
  FROM pg_proc AS p1, pg_proc AS p2
  WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
!     p1.prokind = 'a' AND p2.prokind = 'a' AND
      array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
  ORDER BY 1;
       oid      |   oid
*************** ORDER BY 1;
*** 1650,1656 ****
  -- For the same reason, built-in aggregates with default arguments are no good.
  SELECT oid, proname
  FROM pg_proc AS p
! WHERE proisagg AND proargdefaults IS NOT NULL;
   oid | proname
  -----+---------
  (0 rows)
--- 1651,1657 ----
  -- For the same reason, built-in aggregates with default arguments are no good.
  SELECT oid, proname
  FROM pg_proc AS p
! WHERE prokind = 'a' AND proargdefaults IS NOT NULL;
   oid | proname
  -----+---------
  (0 rows)
*************** WHERE proisagg AND proargdefaults IS NOT
*** 1660,1666 ****
  -- that is not subject to the misplaced ORDER BY issue).
  SELECT p.oid, proname
  FROM pg_proc AS p JOIN pg_aggregate AS a ON a.aggfnoid = p.oid
! WHERE proisagg AND provariadic != 0 AND a.aggkind = 'n';
   oid | proname
  -----+---------
  (0 rows)
--- 1661,1667 ----
  -- that is not subject to the misplaced ORDER BY issue).
  SELECT p.oid, proname
  FROM pg_proc AS p JOIN pg_aggregate AS a ON a.aggfnoid = p.oid
! WHERE prokind = 'a' AND provariadic != 0 AND a.aggkind = 'n';
   oid | proname
  -----+---------
  (0 rows)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5acb92f..d7eff6c 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** UNION ALL
*** 1521,1529 ****
   SELECT l.objoid,
      l.classoid,
      l.objsubid,
!         CASE
!             WHEN (pro.proisagg = true) THEN 'aggregate'::text
!             WHEN (pro.proisagg = false) THEN 'function'::text
              ELSE NULL::text
          END AS objtype,
      pro.pronamespace AS objnamespace,
--- 1521,1531 ----
   SELECT l.objoid,
      l.classoid,
      l.objsubid,
!         CASE pro.prokind
!             WHEN 'a'::"char" THEN 'aggregate'::text
!             WHEN 'f'::"char" THEN 'function'::text
!             WHEN 'p'::"char" THEN 'procedure'::text
!             WHEN 'w'::"char" THEN 'window'::text
              ELSE NULL::text
          END AS objtype,
      pro.pronamespace AS objnamespace,
diff --git a/src/test/regress/sql/alter_generic.sql b/src/test/regress/sql/alter_generic.sql
index 96be6e7..fd43f23 100644
*** a/src/test/regress/sql/alter_generic.sql
--- b/src/test/regress/sql/alter_generic.sql
*************** ALTER AGGREGATE alt_agg2(int) SET SCHEMA
*** 81,87 ****

  RESET SESSION AUTHORIZATION;

! SELECT n.nspname, proname, prorettype::regtype, proisagg, a.rolname
    FROM pg_proc p, pg_namespace n, pg_authid a
    WHERE p.pronamespace = n.oid AND p.proowner = a.oid
      AND n.nspname IN ('alt_nsp1', 'alt_nsp2')
--- 81,87 ----

  RESET SESSION AUTHORIZATION;

! SELECT n.nspname, proname, prorettype::regtype, prokind, a.rolname
    FROM pg_proc p, pg_namespace n, pg_authid a
    WHERE p.pronamespace = n.oid AND p.proowner = a.oid
      AND n.nspname IN ('alt_nsp1', 'alt_nsp2')
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index fbdf831..8f209d5 100644
*** a/src/test/regress/sql/create_function_3.sql
--- b/src/test/regress/sql/create_function_3.sql
*************** DROP FUNCTION functest_b_1;  -- error, n
*** 175,180 ****
--- 175,188 ----
  DROP FUNCTION functest_b_2;  -- error, ambiguous


+ -- CREATE OR REPLACE tests
+
+ CREATE FUNCTION functest1(a int) RETURNS int LANGUAGE SQL AS 'SELECT $1';
+ CREATE OR REPLACE FUNCTION functest1(a int) RETURNS int LANGUAGE SQL WINDOW AS 'SELECT $1';
+ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1';
+ DROP FUNCTION functest1(a int);
+
+
  -- Cleanups
  DROP SCHEMA temp_func_test CASCADE;
  DROP USER regress_unpriv_user;
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index e8fdf84..a593d37 100644
*** a/src/test/regress/sql/opr_sanity.sql
--- b/src/test/regress/sql/opr_sanity.sql
*************** WHERE p1.prolang = 0 OR p1.prorettype =
*** 82,87 ****
--- 82,88 ----
         0::oid = ANY (p1.proargtypes) OR
         procost <= 0 OR
         CASE WHEN proretset THEN prorows <= 0 ELSE prorows != 0 END OR
+        prokind NOT IN ('f', 'a', 'w', 'p') OR
         provolatile NOT IN ('i', 's', 'v') OR
         proparallel NOT IN ('s', 'r', 'u');

*************** SELECT p1.oid, p1.proname
*** 90,99 ****
  FROM pg_proc as p1
  WHERE prosrc IS NULL OR prosrc = '' OR prosrc = '-';

! -- proiswindow shouldn't be set together with proisagg or proretset
  SELECT p1.oid, p1.proname
  FROM pg_proc AS p1
! WHERE proiswindow AND (proisagg OR proretset);

  -- currently, no built-in functions should be SECURITY DEFINER;
  -- this might change in future, but there will probably never be many.
--- 91,100 ----
  FROM pg_proc as p1
  WHERE prosrc IS NULL OR prosrc = '' OR prosrc = '-';

! -- proretset should only be set for normal functions
  SELECT p1.oid, p1.proname
  FROM pg_proc AS p1
! WHERE proretset AND prokind != 'f';

  -- currently, no built-in functions should be SECURITY DEFINER;
  -- this might change in future, but there will probably never be many.
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 140,148 ****
  WHERE p1.oid < p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     (p1.proisagg = false OR p2.proisagg = false) AND
      (p1.prolang != p2.prolang OR
!      p1.proisagg != p2.proisagg OR
       p1.prosecdef != p2.prosecdef OR
       p1.proleakproof != p2.proleakproof OR
       p1.proisstrict != p2.proisstrict OR
--- 141,149 ----
  WHERE p1.oid < p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     (p1.prokind != 'a' OR p2.prokind != 'a') AND
      (p1.prolang != p2.prolang OR
!      p1.prokind != p2.prokind OR
       p1.prosecdef != p2.prosecdef OR
       p1.proleakproof != p2.proleakproof OR
       p1.proisstrict != p2.proisstrict OR
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 166,172 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.prorettype < p2.prorettype)
--- 167,173 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.prorettype < p2.prorettype)
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 177,183 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[0] < p2.proargtypes[0])
--- 178,184 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[0] < p2.proargtypes[0])
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 188,194 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[1] < p2.proargtypes[1])
--- 189,195 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      p1.prosrc NOT LIKE E'range\\_constructor_' AND
      p2.prosrc NOT LIKE E'range\\_constructor_' AND
      (p1.proargtypes[1] < p2.proargtypes[1])
*************** FROM pg_proc AS p1, pg_proc AS p2
*** 199,205 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[2] < p2.proargtypes[2])
  ORDER BY 1, 2;

--- 200,206 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[2] < p2.proargtypes[2])
  ORDER BY 1, 2;

*************** FROM pg_proc AS p1, pg_proc AS p2
*** 208,214 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[3] < p2.proargtypes[3])
  ORDER BY 1, 2;

--- 209,215 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[3] < p2.proargtypes[3])
  ORDER BY 1, 2;

*************** FROM pg_proc AS p1, pg_proc AS p2
*** 217,223 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[4] < p2.proargtypes[4])
  ORDER BY 1, 2;

--- 218,224 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[4] < p2.proargtypes[4])
  ORDER BY 1, 2;

*************** FROM pg_proc AS p1, pg_proc AS p2
*** 226,232 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[5] < p2.proargtypes[5])
  ORDER BY 1, 2;

--- 227,233 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[5] < p2.proargtypes[5])
  ORDER BY 1, 2;

*************** FROM pg_proc AS p1, pg_proc AS p2
*** 235,241 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[6] < p2.proargtypes[6])
  ORDER BY 1, 2;

--- 236,242 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[6] < p2.proargtypes[6])
  ORDER BY 1, 2;

*************** FROM pg_proc AS p1, pg_proc AS p2
*** 244,250 ****
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     NOT p1.proisagg AND NOT p2.proisagg AND
      (p1.proargtypes[7] < p2.proargtypes[7])
  ORDER BY 1, 2;

--- 245,251 ----
  WHERE p1.oid != p2.oid AND
      p1.prosrc = p2.prosrc AND
      p1.prolang = 12 AND p2.prolang = 12 AND
!     p1.prokind != 'a' AND p2.prokind != 'a' AND
      (p1.proargtypes[7] < p2.proargtypes[7])
  ORDER BY 1, 2;

*************** WHERE aggfnoid = 0 OR aggtransfn = 0 OR
*** 804,816 ****
  SELECT a.aggfnoid::oid, p.proname
  FROM pg_aggregate as a, pg_proc as p
  WHERE a.aggfnoid = p.oid AND
!     (NOT p.proisagg OR p.proretset OR p.pronargs < a.aggnumdirectargs);

! -- Make sure there are no proisagg pg_proc entries without matches.

  SELECT oid, proname
  FROM pg_proc as p
! WHERE p.proisagg AND
      NOT EXISTS (SELECT 1 FROM pg_aggregate a WHERE a.aggfnoid = p.oid);

  -- If there is no finalfn then the output type must be the transtype.
--- 805,817 ----
  SELECT a.aggfnoid::oid, p.proname
  FROM pg_aggregate as a, pg_proc as p
  WHERE a.aggfnoid = p.oid AND
!     (p.prokind != 'a' OR p.proretset OR p.pronargs < a.aggnumdirectargs);

! -- Make sure there are no prokind = PROKIND_AGGREGATE pg_proc entries without matches.

  SELECT oid, proname
  FROM pg_proc as p
! WHERE p.prokind = 'a' AND
      NOT EXISTS (SELECT 1 FROM pg_aggregate a WHERE a.aggfnoid = p.oid);

  -- If there is no finalfn then the output type must be the transtype.
*************** ORDER BY 1, 2;
*** 1089,1095 ****
  SELECT p1.oid::regprocedure, p2.oid::regprocedure
  FROM pg_proc AS p1, pg_proc AS p2
  WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
!     p1.proisagg AND p2.proisagg AND
      array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
  ORDER BY 1;

--- 1090,1096 ----
  SELECT p1.oid::regprocedure, p2.oid::regprocedure
  FROM pg_proc AS p1, pg_proc AS p2
  WHERE p1.oid < p2.oid AND p1.proname = p2.proname AND
!     p1.prokind = 'a' AND p2.prokind = 'a' AND
      array_dims(p1.proargtypes) != array_dims(p2.proargtypes)
  ORDER BY 1;

*************** ORDER BY 1;
*** 1097,1103 ****

  SELECT oid, proname
  FROM pg_proc AS p
! WHERE proisagg AND proargdefaults IS NOT NULL;

  -- For the same reason, we avoid creating built-in variadic aggregates, except
  -- that variadic ordered-set aggregates are OK (since they have special syntax
--- 1098,1104 ----

  SELECT oid, proname
  FROM pg_proc AS p
! WHERE prokind = 'a' AND proargdefaults IS NOT NULL;

  -- For the same reason, we avoid creating built-in variadic aggregates, except
  -- that variadic ordered-set aggregates are OK (since they have special syntax
*************** WHERE proisagg AND proargdefaults IS NOT
*** 1105,1111 ****

  SELECT p.oid, proname
  FROM pg_proc AS p JOIN pg_aggregate AS a ON a.aggfnoid = p.oid
! WHERE proisagg AND provariadic != 0 AND a.aggkind = 'n';


  -- **************** pg_opfamily ****************
--- 1106,1112 ----

  SELECT p.oid, proname
  FROM pg_proc AS p JOIN pg_aggregate AS a ON a.aggfnoid = p.oid
! WHERE prokind = 'a' AND provariadic != 0 AND a.aggkind = 'n';


  -- **************** pg_opfamily ****************
#! /usr/bin/perl

# Usage: fix_proc_data.pl <pg_proc.h >new_pg_proc.h

use strict;
use warnings;

while (<>)
{
    if (m/^DATA/) {
    my @fields = split;
    my $proisagg = $fields[13];
    my $proiswindow = $fields[14];
    my $prokind;
    if ($proisagg eq 'f' && $proiswindow eq 'f') {
        $prokind = 'f';
    } elsif ($proisagg eq 't' && $proiswindow eq 'f') {
        $prokind = 'a';
    } elsif ($proisagg eq 'f' && $proiswindow eq 't') {
        $prokind = 'w';
    } else {
        die "bad proisagg/proiswindow";
    }
    s/ $proisagg $proiswindow / $prokind / || die "substitution failed";
    }
    print;
}

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

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

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