Re: [HACKERS] DROP FUNCTION of multiple functions

On Sun, Jan 1, 2017 at 1:17 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/1/16 9:32 PM, Peter Eisentraut wrote:
>> I think it would be better to get rid of objargs and have objname be a
>> general Node that can contain more specific node types so that there is
>> some amount of type tracking.  FuncWithArgs would be one such type,
>> Typename would be another, Value would be used for simple strings, and
>> we could create some other ones, or stick with lcons for some simple
>> cases.  But then we don't have to make stuff into one-item lists to just
>> to satisfy the currently required List.
>>
>> That's the general idea.  But that's a rather big change that I would
>> rather break down into smaller pieces.
>
> People wanted to the whole thing at once, so here it is.
>
> Patches 0001 and 0002 are some prep work.  Patch 0003 is the main patch
> that removes the objargs fields and changes the objname to a generic
> Node*.  0004 is an API simplification that could be committed together
> with 0003 but I kept it separate for easier review.  0005 accomplishes
> the DROP FUNCTION changes.  0006 is some other cleanup that fell out of
> this.

I don't see any problems with 0001. In 0002, the comment of
class_args/CreateOpClassItem in parsenodes.h needs to be updated,
because this becomes used by operators as well.

Regarding 0003, which is large, but actually quite simple... The
approach makes sense. All the checks on the object formats are moved
out of the static routines in objectaddress.c and moved into a single
place.

+           if (list_length(name) != 1)
+               ereport(ERROR,
+                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                     errmsg("name list length must be exactly %d", 1)));
+           objnode = linitial(name);
Those are the sanity checks for each object are moved to
pg_get_object_address(). This has as result some loss of verbosity in
the error messages. As the object type is passed by the user when
calling the SQL-callable interface, this new way looks fine to me.

One comment though: there are still many list_make2() or even
list_make3 calls for some object types. Would it make sense to replace
those lists with a decided number of items by a Node and simplify the
interface? Using a Node instead of a list would save some cycles. OK
that's really few though as that's basically comparing a list lookup
with a direct pointer lookup.

+           | DROP drop_type1 any_name_list opt_drop_behavior
+               {
+                   DropStmt *n = makeNode(DropStmt);
+                   n->removeType = $2;
+                   n->missing_ok = FALSE;
+                   n->objects = $3;
+                   n->behavior = $4;
+                   n->concurrent = false;
+                   $$ = (Node *)n;
+               }
+           | DROP drop_type2 IF_P EXISTS name_list opt_drop_behavior
+               {
drop_type1 and drop_type2 are not really explicit, especially after
reading that one allows schema-qualified names, not the other. Could
you use something like drop_type_qualified? The same applies to
comment_type and security_label.

0004 could be merged with 0002, no? That looks good to me.

In 0005, a nit:
+DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int),
functest_IS_3(int);-- Cleanups
The DROP query could be moved below the cleanup comment.

It may be a good idea to add an example of query dropping two
functions at once in the docs.

Could you add some regression tests for the drop of aggregates and
operators to stress the parsing code path?

While looking at 0006... DROP POLICY and DROP RULE could be unified. I
just noticed that while reading the code.

This patch series look in rather good shape to me at the end.
-- 
Michael