On Fri, Dec 2, 2016 at 1:32 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
> 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.
Moved to next CF with "needs review" status.
Regards,
Hari Babu
Fujitsu Australia
