pgsql: SQL-standard function body

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: SQL-standard function body

Peter Eisentraut-3
SQL-standard function body

This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.

Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
this allows writing out the SQL statements making up the body
unquoted, either as a single statement:

    CREATE FUNCTION add(a integer, b integer) RETURNS integer
        LANGUAGE SQL
        RETURN a + b;

or as a block

    CREATE PROCEDURE insert_data(a integer, b integer)
    LANGUAGE SQL
    BEGIN ATOMIC
      INSERT INTO tbl VALUES (a);
      INSERT INTO tbl VALUES (b);
    END;

The function body is parsed at function definition time and stored as
expression nodes in a new pg_proc column prosqlbody.  So at run time,
no further parsing is required.

However, this form does not support polymorphic arguments, because
there is no more parse analysis done at call time.

Dependencies between the function and the objects it uses are fully
tracked.

A new RETURN statement is introduced.  This can only be used inside
function bodies.  Internally, it is treated much like a SELECT
statement.

psql needs some new intelligence to keep track of function body
boundaries so that it doesn't send off statements when it sees
semicolons that are inside a function body.

Tested-by: Jaime Casanova <[hidden email]>
Reviewed-by: Julien Rouhaud <[hidden email]>
Discussion: https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c02d7@...

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/e717a9a18b2e34c9c40e5259ad4d31cd7e420750

Modified Files
--------------
doc/src/sgml/catalogs.sgml                      |  10 +
doc/src/sgml/ref/create_function.sgml           | 125 ++++++++++--
doc/src/sgml/ref/create_procedure.sgml          |  61 +++++-
src/backend/catalog/pg_aggregate.c              |   1 +
src/backend/catalog/pg_proc.c                   | 116 +++++++----
src/backend/commands/aggregatecmds.c            |   2 +
src/backend/commands/functioncmds.c             | 145 ++++++++++---
src/backend/commands/typecmds.c                 |   4 +
src/backend/executor/functions.c                |  79 +++++---
src/backend/nodes/copyfuncs.c                   |  15 ++
src/backend/nodes/equalfuncs.c                  |  13 ++
src/backend/nodes/outfuncs.c                    |  12 ++
src/backend/nodes/readfuncs.c                   |   1 +
src/backend/optimizer/util/clauses.c            | 126 ++++++++----
src/backend/parser/analyze.c                    |  35 ++++
src/backend/parser/gram.y                       | 129 +++++++++---
src/backend/tcop/postgres.c                     |   3 +-
src/backend/utils/adt/ruleutils.c               | 139 ++++++++++++-
src/bin/pg_dump/pg_dump.c                       |  45 ++++-
src/bin/psql/describe.c                         |  15 +-
src/fe_utils/psqlscan.l                         |  24 ++-
src/include/catalog/catversion.h                |   2 +-
src/include/catalog/pg_proc.dat                 |   4 +
src/include/catalog/pg_proc.h                   |   6 +-
src/include/commands/defrem.h                   |   2 +
src/include/executor/functions.h                |  15 ++
src/include/fe_utils/psqlscan_int.h             |   2 +
src/include/nodes/nodes.h                       |   1 +
src/include/nodes/parsenodes.h                  |  13 ++
src/include/parser/kwlist.h                     |   2 +
src/include/tcop/tcopprot.h                     |   1 +
src/interfaces/ecpg/preproc/ecpg.addons         |   6 +
src/interfaces/ecpg/preproc/ecpg.trailer        |   4 +-
src/test/regress/expected/create_function_3.out | 257 +++++++++++++++++++++++-
src/test/regress/expected/create_procedure.out  |  65 ++++++
src/test/regress/sql/create_function_3.sql      | 110 +++++++++-
src/test/regress/sql/create_procedure.sql       |  33 +++
37 files changed, 1411 insertions(+), 212 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Andres Freund
Hi,

On 2021-04-07 19:53:35 +0000, Peter Eisentraut wrote:

> SQL-standard function body
>
> This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
> statements for language SQL with a function body that conforms to the
> SQL standard and is portable to other implementations.
>
> Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
> this allows writing out the SQL statements making up the body
> unquoted, either as a single statement:
>
>     CREATE FUNCTION add(a integer, b integer) RETURNS integer
>         LANGUAGE SQL
>         RETURN a + b;
>
> or as a block
>
>     CREATE PROCEDURE insert_data(a integer, b integer)
>     LANGUAGE SQL
>     BEGIN ATOMIC
>       INSERT INTO tbl VALUES (a);
>       INSERT INTO tbl VALUES (b);
>     END;
>
> The function body is parsed at function definition time and stored as
> expression nodes in a new pg_proc column prosqlbody.  So at run time,
> no further parsing is required.
>
> However, this form does not support polymorphic arguments, because
> there is no more parse analysis done at call time.
>
> Dependencies between the function and the objects it uses are fully
> tracked.
>
> A new RETURN statement is introduced.  This can only be used inside
> function bodies.  Internally, it is treated much like a SELECT
> statement.
>
> psql needs some new intelligence to keep track of function body
> boundaries so that it doesn't send off statements when it sees
> semicolons that are inside a function body.
>
> Tested-by: Jaime Casanova <[hidden email]>
> Reviewed-by: Julien Rouhaud <[hidden email]>
> Discussion: https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c02d7@...
>
> Branch
> ------
> master
>
> Details
> -------
> https://git.postgresql.org/pg/commitdiff/e717a9a18b2e34c9c40e5259ad4d31cd7e420750

This is turning the BF red:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2021-04-07%2022%3A52%3A19

Might be force_parallel_mode=regress related.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2021-04-07 19:53:35 +0000, Peter Eisentraut wrote:
>> SQL-standard function body

> This is turning the BF red:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2021-04-07%2022%3A52%3A19
> Might be force_parallel_mode=regress related.

Yeah.  On my machine, it's fine without force_parallel_mode and
crashes with that.  Looks like query text is not getting passed
to the parallel worker in some cases.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

David Rowley
On Thu, 8 Apr 2021 at 11:06, Tom Lane <[hidden email]> wrote:
>
> Andres Freund <[hidden email]> writes:
>
> > Might be force_parallel_mode=regress related.
>
> Yeah.  On my machine, it's fine without force_parallel_mode and
> crashes with that.  Looks like query text is not getting passed
> to the parallel worker in some cases.

I wonder if it would be worth adding a regression test run of the core
tests during make check-world with force_parallel_mode set to regress.

But maybe people running the tests on slow machines might not be a fan
of that idea.

David


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Andres Freund
Hi,

On 2021-04-08 13:33:18 +1200, David Rowley wrote:

> On Thu, 8 Apr 2021 at 11:06, Tom Lane <[hidden email]> wrote:
> >
> > Andres Freund <[hidden email]> writes:
> >
> > > Might be force_parallel_mode=regress related.
> >
> > Yeah.  On my machine, it's fine without force_parallel_mode and
> > crashes with that.  Looks like query text is not getting passed
> > to the parallel worker in some cases.
>
> I wonder if it would be worth adding a regression test run of the core
> tests during make check-world with force_parallel_mode set to regress.
>
> But maybe people running the tests on slow machines might not be a fan
> of that idea.

I've wondered about that too. Perhaps we could reuse the pg_upgrade run?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Michael Paquier-2
On Wed, Apr 07, 2021 at 06:50:17PM -0700, Andres Freund wrote:
> I've wondered about that too. Perhaps we could reuse the pg_upgrade run?

Yes, it would be a good idea to reuse that.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Peter Eisentraut-3
In reply to this post by Tom Lane-2
On 08.04.21 01:06, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
>> On 2021-04-07 19:53:35 +0000, Peter Eisentraut wrote:
>>> SQL-standard function body
>
>> This is turning the BF red:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2021-04-07%2022%3A52%3A19
>> Might be force_parallel_mode=regress related.
>
> Yeah.  On my machine, it's fine without force_parallel_mode and
> crashes with that.  Looks like query text is not getting passed
> to the parallel worker in some cases.

There does not appear to be any indication in CreateQueryDesc() or for
es_sourceText that the query source text needs to be not NULL (unlike
PortalDefineQuery() for example).  If we want that to be the case, an
assert in CreateQueryDesc() would be a straightforward way to catch
this.  But for example code in CreateExecutorState() and
executor_errposition() is prepared to have es_sourceText be NULL, so
relative to that, the fix that Andres put into ExecInitParallelPlan() to
handle that as well seems appropriate.


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Robert Haas
On Thu, Apr 8, 2021 at 2:25 AM Peter Eisentraut <[hidden email]> wrote:

> On 08.04.21 01:06, Tom Lane wrote:
> > Andres Freund <[hidden email]> writes:
> >> On 2021-04-07 19:53:35 +0000, Peter Eisentraut wrote:
> >>> SQL-standard function body
> >
> >> This is turning the BF red:
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2021-04-07%2022%3A52%3A19
> >> Might be force_parallel_mode=regress related.
> >
> > Yeah.  On my machine, it's fine without force_parallel_mode and
> > crashes with that.  Looks like query text is not getting passed
> > to the parallel worker in some cases.
>
> There does not appear to be any indication in CreateQueryDesc() or for
> es_sourceText that the query source text needs to be not NULL (unlike
> PortalDefineQuery() for example).  If we want that to be the case, an
> assert in CreateQueryDesc() would be a straightforward way to catch
> this.  But for example code in CreateExecutorState() and
> executor_errposition() is prepared to have es_sourceText be NULL, so
> relative to that, the fix that Andres put into ExecInitParallelPlan() to
> handle that as well seems appropriate.

I think we should insist on having a query string all the time, if at
all possible, because having the query string is awfully nice for
debugging. For example, if a parallel worker dumps core, you'd like to
be able to find out what query it was trying to run when things went
bad. But if we're not going to do that, then I think it would be
better to do something more like what Noah did in
f90e80b9138355a51d2d5b5b63e1f89c4ba53325 rather than what Andres did
here.

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


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Andrew Dunstan
In reply to this post by Andres Freund

On 4/7/21 9:50 PM, Andres Freund wrote:

> Hi,
>
> On 2021-04-08 13:33:18 +1200, David Rowley wrote:
>> On Thu, 8 Apr 2021 at 11:06, Tom Lane <[hidden email]> wrote:
>>> Andres Freund <[hidden email]> writes:
>>>
>>>> Might be force_parallel_mode=regress related.
>>> Yeah.  On my machine, it's fine without force_parallel_mode and
>>> crashes with that.  Looks like query text is not getting passed
>>> to the parallel worker in some cases.
>> I wonder if it would be worth adding a regression test run of the core
>> tests during make check-world with force_parallel_mode set to regress.
>>
>> But maybe people running the tests on slow machines might not be a fan
>> of that idea.
> I've wondered about that too. Perhaps we could reuse the pg_upgrade run?
>


Honestly I'd prefer it if we could get rid of the rerun of 'make check'
by pg_upgrade's test.sh and instead upgrade the data directory made by
the earlier 'make check' run if it's still there (which would mean we'd
need to stop it being deleted).


cheers


andrew


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



Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Tom Lane-2
Andrew Dunstan <[hidden email]> writes:
> On 4/7/21 9:50 PM, Andres Freund wrote:
>> I've wondered about that too. Perhaps we could reuse the pg_upgrade run?

> Honestly I'd prefer it if we could get rid of the rerun of 'make check'
> by pg_upgrade's test.sh and instead upgrade the data directory made by
> the earlier 'make check' run if it's still there (which would mean we'd
> need to stop it being deleted).

Good idea as far as speeding check-world and buildfarm runs, but I wonder
if we wouldn't be losing test coverage.  Seeing the number of times that
buildfarm runs have gotten through "make check" only to fail at the
re-run in pg_upgrade, it seems clear to me that there is something
different about the execution environment in the latter case.  I've
never been able to pin down quite what :-(

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Andres Freund
Hi,

On Thu, Apr 8, 2021, at 07:52, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> writes:
> > On 4/7/21 9:50 PM, Andres Freund wrote:
> >> I've wondered about that too. Perhaps we could reuse the pg_upgrade run?
>
> > Honestly I'd prefer it if we could get rid of the rerun of 'make check'
> > by pg_upgrade's test.sh and instead upgrade the data directory made by
> > the earlier 'make check' run if it's still there (which would mean we'd
> > need to stop it being deleted).
>
> Good idea as far as speeding check-world and buildfarm runs, but I wonder
> if we wouldn't be losing test coverage.  Seeing the number of times that
> buildfarm runs have gotten through "make check" only to fail at the
> re-run in pg_upgrade, it seems clear to me that there is something
> different about the execution environment in the latter case.  I've
> never been able to pin down quite what :-(

IIRC we made it run with fsync explicitly enabled. Which obviously will change timing somewhat substantially...


Andres


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Michael Paquier-2
On Thu, Apr 08, 2021 at 07:56:10AM -0700, Andres Freund wrote:
> IIRC we made it run with fsync explicitly enabled. Which obviously
> will change timing somewhat substantially...

Yeah.  I think that this kind of things pretty much boils down into
making the tests of pg_upgrade into TAP.  We have already a set of
rather-sane defaults in PostgresNode.  This does not prevent just
sticking one or two extra configuration lines into test.sh, of
course.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Laurenz Albe
In reply to this post by Peter Eisentraut-3
On Wed, 2021-04-07 at 19:53 +0000, Peter Eisentraut wrote:

> SQL-standard function body
>
> This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
> statements for language SQL with a function body that conforms to the
> SQL standard and is portable to other implementations.
>
> [...]
>
> psql needs some new intelligence to keep track of function body
> boundaries so that it doesn't send off statements when it sees
> semicolons that are inside a function body.
This causes psql to fail to recognize the semicolon in the following
statement as the end of a statement:

  IMPORT FOREIGN SCHEMA x FROM SERVER y INTO z OPTIONS (case 'lower');

The cause is the "case", which is recognized as the start of a function
body.

Now you could argue that "case" is a reserved word, and I should
double quote it if I insist on using it as a FDW option, but it
is a regression.

Would it be an option to recognize BEGIN and CASE as starting a
function body only if they are *not* inside parentheses, like in
the attached?

Yours,
Laurenz Albe

0001-Improve-psql-s-parsing-of-SQL-standard-function-bodi.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Laurenz Albe
On Fri, 2021-04-09 at 19:44 +0200, I wrote:

> > SQL-standard function body
> >
> > psql needs some new intelligence to keep track of function body
> > boundaries so that it doesn't send off statements when it sees
> > semicolons that are inside a function body.
>
> This causes psql to fail to recognize the semicolon in the following
> statement as the end of a statement:
>
>   IMPORT FOREIGN SCHEMA x FROM SERVER y INTO z OPTIONS (case 'lower');
>
> The cause is the "case", which is recognized as the start of a function
> body.
>
> Would it be an option to recognize BEGIN and CASE as starting a
> function body only if they are *not* inside parentheses, like in
> the attached?
Here is an improved patch, which treats END in the same fashion
(not properly indented for readability).

Yours,
Laurenz Albe

0001-Improve-parsing-of-SQL-standard-functions-in-psql.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Peter Eisentraut-7
On 10.04.21 03:38, Laurenz Albe wrote:

> On Fri, 2021-04-09 at 19:44 +0200, I wrote:
>>> SQL-standard function body
>>>
>>> psql needs some new intelligence to keep track of function body
>>> boundaries so that it doesn't send off statements when it sees
>>> semicolons that are inside a function body.
>>
>> This causes psql to fail to recognize the semicolon in the following
>> statement as the end of a statement:
>>
>>    IMPORT FOREIGN SCHEMA x FROM SERVER y INTO z OPTIONS (case 'lower');
>>
>> The cause is the "case", which is recognized as the start of a function
>> body.
>>
>> Would it be an option to recognize BEGIN and CASE as starting a
>> function body only if they are *not* inside parentheses, like in
>> the attached?
>
> Here is an improved patch, which treats END in the same fashion
> (not properly indented for readability).
Thanks, I took another look at this and augmented your change with a
change that tracks whether the statement starts with CREATE [OR REPLACE]
{FUNCTION|PROCEDURE}.  That should make it pretty safe.  What do you think?

0001-psql-Refine-lexing-of-BEGIN.END-blocks-in-CREATE-FUN.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: SQL-standard function body

Laurenz Albe
On Mon, 2021-04-12 at 17:25 +0200, Peter Eisentraut wrote:

> On 10.04.21 03:38, Laurenz Albe wrote:
> > On Fri, 2021-04-09 at 19:44 +0200, I wrote:
> > > > SQL-standard function body
> > > >
> > > > psql needs some new intelligence to keep track of function body
> > > > boundaries so that it doesn't send off statements when it sees
> > > > semicolons that are inside a function body.
> > >
> > > This causes psql to fail to recognize the semicolon in the following
> > > statement as the end of a statement:
> > >
> > >    IMPORT FOREIGN SCHEMA x FROM SERVER y INTO z OPTIONS (case 'lower');
> > >
> > > The cause is the "case", which is recognized as the start of a function
> > > body.
> > >
> > > Would it be an option to recognize BEGIN and CASE as starting a
> > > function body only if they are *not* inside parentheses, like in
> > > the attached?
> >
> > Here is an improved patch, which treats END in the same fashion
> > (not properly indented for readability).
>
> Thanks, I took another look at this and augmented your change with a
> change that tracks whether the statement starts with CREATE [OR REPLACE]
> {FUNCTION|PROCEDURE}.  That should make it pretty safe.  What do you think?

Thanks, that is fine with me and more than I asked for.
That should definitely exclude all false positive matches.

Yours,
Laurenz Albe