Alvaro Herrera <alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers