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

Hari Babu
Fujitsu Australia

Reply via email to