On Mon, Dec 8, 2014 at 6:06 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > > On Wed, Nov 26, 2014 at 4:26 AM, Tom Lane <t...@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. Regards, Atri -- Regards, Atri *l'apprenant*
