Re: DROP FUNCTION of multiple functions

On 11/23/16 5:04 PM, Tom Lane wrote:
> I looked at this briefly. I agree that 0001-0003 are simple cleanup of
> the grammar and could be pushed without further ado.

Done.

> However, starting
> with 0004 I begin to get queasy. The plan seems to be that instead of
> "objname" always being a List of strings, for functions (and then
> aggregates and operators) it will be a one-element List of some new struct
> that has then got a name list inside it.

I think the original idea of representing an object by a list of name
components plus optionally a list of arguments has accumulated too many
warts and should be replaced. For example: A Typename isn't a list, so
it has to be packed into a List to be able to pass it around. Some
objects only have a single-component string as a name. For a cast, we
arbitrarily put the source type into a the name list and the target type
into the argument list. For an operator class on the other hand we
create a cons of name and access method. The pending logical
replication patch has more such arbitrary examples. This pattern has to
be repeated consistently in gram.y for all cases where the object is
referenced (ALTER EXTENSION, DROP, COMMENT, RENAME, SET SCHEMA, OWNER
TO) and then consistently unpacked in objectaddress.c.

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. I have a separate patch in
progress for that, which I have attached here. It breaks some
regression tests in object_address.sql, which I haven't evaluated yet,
but that's the idea.

However, in these current patches I just wanted to take the first step
to normalize the representation of functions so that actual
functionality changes could be built in top of that.

> It leads to code with random changes of data representation at seemingly
> random places, like this bit from 0005:
>
> + if (stmt->removeType == OBJECT_AGGREGATE || stmt->removeType == OBJECT_FUNCTION)
> + objname = list_make1(objname);

Yeah, the reason for that is that when we want to store something as
objname, it needs to be a list, and the convention is to wrap non-lists
into a single-member list. So then objectaddress.c is coded to linitial
such lists when working on object types such as functions or types.
RemoveObjects() takes the list it gets from the grammar and passes each
element to get_object_address(). But a function_with_argtypes_list is a
list of FuncWithArgs, so we have to make those into single-member lists
first. The reason this works for types is that type_name_list looks
like this:

type_name_list:
Typename { $$ = list_make1(list_make1($1)); }
| type_name_list ',' Typename { $$ = lappend($1, list_make1($3)); }

I suppose we could make function_with_argtypes look like that as well
(and later change it back if we redesign it as discussed above). I
think if we did it that way, it would get rid of the warts in this patch
set.

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