Re: [HACKERS] SQL procedures

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

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

cheers

andrew

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