On Wed, Nov 26, 2014 at 4:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think what's likely missing here is a clear design for the raw parse > tree representation (what's returned by the bison grammar). The patch > seems to be trying to skate by without creating any new parse node types > or fields, but that may well be a bad idea. At the very least there > needs to be some commentary added to parsenodes.h explaining what the > representation actually is (cf commentary there for MultiAssignRef). > > Also, I think it's a mistake not to be following the MultiAssignRef > model for the case of "(*) = ctext_row". We optimize the ROW-source > case at the grammar stage when there's a fixed number of target columns, > because that's a very easy transformation --- but you can't do it like > that when there's not. It's possible that that optimization should be > delayed till later even in the existing case; in general, optimizing > in gram.y is a bad habit that's better avoided ... Marking as "returned with feedback" based on those comments.
Thank you everybody for your comments.
I am indeed trying to avoid a new node since my intuitive feeling says that we do not need a new node for a functionality which is present in other commands albeit in different form. However, I agree that documentation explaining parse representation is lacking and shall improve that. Also, in accordance to Tom's comments, I shall look for not touching target list directly and follow the path of MultiAssignRef.
I have moved patch to current CF and marked it as Waiting on Author since I plan to resubmit after addressing the concerns.
