On Mon, Dec 8, 2014 at 6:06 AM, Michael Paquier <michael.paqu...@gmail.com>
> 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.





Reply via email to