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