Re: [HACKERS] SQL procedures - Mailing list pgsql-hackers
From | Andrew Dunstan |
---|---|
Subject | Re: [HACKERS] SQL procedures |
Date | |
Msg-id | fd3d1477-a386-5f59-7e8c-e88967300d81@2ndQuadrant.com Whole thread Raw |
In response to | Re: [HACKERS] SQL procedures (Peter Eisentraut <peter.eisentraut@2ndquadrant.com>) |
Responses |
Re: [HACKERS] SQL procedures
Re: [HACKERS] SQL procedures |
List | pgsql-hackers |
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
pgsql-hackers by date: