segmentation fault in pg head with SQL function.

On Fri, Mar 16, 2018 at 11:35:13AM +0530, Prabhat Sahu wrote:

I found a segmentation fault on pg master Head with below steps and
stacktrace.

postgres=# CREATE OR REPLACE FUNCTION func1() RETURNS VOID
LANGUAGE SQL
AS $$
select 10;
$$;
CREATE FUNCTION

postgres=# select func1();
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

(Adding Peter and Tom in CC)

Problem reproducible here, and the bug has been introduced by fd1a421f.
It seems to me that the function should not be authorized to be created
to begin with, as it returns an integer in its last query, where I think
that check_sql_fn_retval is doing it wrong when called in
inline_function() as we know that it handles a function, and not a
procedure thanks to the first sanity checks at the top of the function.

Here is what happens before this commit:
=# CREATE OR REPLACE FUNCTION func1() RETURNS VOID
LANGUAGE SQL
AS $$
select 10;
$;
ERROR: 42P13: return type mismatch in function declared to return void
DETAIL: Actual return type is integer.
CONTEXT: SQL function "func1"
LOCATION: check_sql_fn_retval, functions.c:1655

As far as I can see, here is the faulty code:
-   if (!rettype)
+   /*
+    * 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;

While this can be bypassed for a procedure, we should fall down and fail
for a SQL function returning void depending on the tlist generated by
the last SQL query.

@@ -1624,8 +1622,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
    if (fn_typtype == TYPTYPE_BASE ||
        fn_typtype == TYPTYPE_DOMAIN ||
        fn_typtype == TYPTYPE_ENUM ||
-       fn_typtype == TYPTYPE_RANGE ||
-       rettype == VOIDOID)
+       fn_typtype == TYPTYPE_RANGE
This also causes the error message generated to change.

Peter, what's the intention here? Shouldn't void still be treated as a
scalar type?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Mar 16, 2018 at 11:35:13AM +0530, Prabhat Sahu wrote:

postgres=# CREATE OR REPLACE FUNCTION func1() RETURNS VOID
LANGUAGE SQL
AS $$
select 10;
$$;

Problem reproducible here, and the bug has been introduced by fd1a421f.
It seems to me that the function should not be authorized to be created
to begin with, as it returns an integer in its last query, where I think
that check_sql_fn_retval is doing it wrong when called in
inline_function() as we know that it handles a function, and not a
procedure thanks to the first sanity checks at the top of the function.

Hm. Actually, I think this is my fault. It is true that previous PG
versions would have rejected this function definition, but my intention
while revising Peter's prokind patch was that we'd start allowing a
VOID-returning SQL function to contain anything, and just ignore whatever
the last statement within it might be. The documentation doesn't say
much about VOID-returning SQL functions, but I certainly don't see
anything saying that they can't end with a SELECT, so arguably the old
behavior is a POLA violation. In any case, this is the behavior we
need for VOID-returning procedures, and there seems little reason not
to make functions act similarly.

So apparently I missed something with that. Will look more closely.

regards, tom lane

On 3/16/18 11:40, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Mar 16, 2018 at 11:35:13AM +0530, Prabhat Sahu wrote:

postgres=# CREATE OR REPLACE FUNCTION func1() RETURNS VOID
LANGUAGE SQL
AS $$
select 10;
$$;

Problem reproducible here, and the bug has been introduced by fd1a421f.
It seems to me that the function should not be authorized to be created
to begin with, as it returns an integer in its last query, where I think
that check_sql_fn_retval is doing it wrong when called in
inline_function() as we know that it handles a function, and not a
procedure thanks to the first sanity checks at the top of the function.

Hm. Actually, I think this is my fault. It is true that previous PG
versions would have rejected this function definition, but my intention
while revising Peter's prokind patch was that we'd start allowing a
VOID-returning SQL function to contain anything, and just ignore whatever
the last statement within it might be. The documentation doesn't say
much about VOID-returning SQL functions, but I certainly don't see
anything saying that they can't end with a SELECT, so arguably the old
behavior is a POLA violation. In any case, this is the behavior we
need for VOID-returning procedures, and there seems little reason not
to make functions act similarly.

So apparently I missed something with that. Will look more closely.

This was listed as an open item, but it was already fixed by
877cdf11eaa9cabcb9b1e3c1bef0760fe08efdc3, so I'll remove it.

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