Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Peter Eisentraut wrote:
>> Here is a patch series that implements several changes in the internal
>> grammar and node representation of function signatures. They are not
>> necessarily meant to be applied separately, but they explain the
>> progression of the changes nicely, so I left them like that for review.
> I think patches 1-4 and 6 are pretty clearly straight cleanup and it
> seems fine to me to push them ahead of the rest. I didn't review super
> carefully but it looks good on a quick glance. I'd just advise to pay
> special attention to the objectaddress.sql test and the UI involved
> there (which you appear to have done already).
I looked at this briefly. I agree that 0001-0003 are simple cleanup of
the grammar and could be pushed without further ado. 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. This seems seriously bug-prone.
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);
I've got no confidence that you've found all the places that will be
broken by that, nor that it won't introduce new bugs later.
Also, it seems like the end goal ought to involve getting rid of the
separate objarg/objargs fields in places like RenameStmt, as well
as the separate objargs argument to get_object_address and friends,
but the patch mostly doesn't do that --- 0006 does change
CreateOpClassItem, and 0007 changes AlterOperatorStmt, but that seems
like just the tip of the iceberg.
I wonder whether it would be useful to invent an ObjectName node type
typedef struct ObjectName{ NodeTag type; List *names; /* list of String, representing possibly-qualified
name*/ List *args; /* list of TypeName, if needed for object type */} ObjectName;
and replace all the places that currently have separate objname/objargs
fields or arguments with a uniform "ObjectName *" field/argument. (This
node type could replace FuncWithArgs, obviously.) Although that would
be more invasive, you could be certain that you'd looked at every place
that's affected by the change of representation.
regards, tom lane