Re: (PATCH) Adding CORRESPONDING to Set Operations

On Mon, Nov 14, 2011 at 6:09 AM, Kerem Kat <keremkat(at)gmail(dot)com> wrote:
> On Mon, Nov 14, 2011 at 15:32, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Kerem Kat <keremkat(at)gmail(dot)com> writes:
>>> Corresponding is currently implemented in the parse/analyze phase. If
>>> it were to be implemented in the planning phase, explain output would
>>> likely be as you expect it to be.
>>
>> It's already been pointed out to you that doing this at parse time is
>> unacceptable, because of the implications for reverse-listing of rules
>> (views).
>>
>>                        regards, tom lane
>>
>
> I am well aware of that thank you.

I took a quick look at the patch and found some miscellaneous points including:

- don't use // style comment
- keep consistent in terms of space around parenthesis like if and foreach
- ereport should have error position as long as possible, especially
in syntax error
- I'm not convinced that new correspoinding_union.sql test is added. I
prefer to include new tests in union.sql
- CORRESPONDING BY should have column name list, not expression list.
It's not legal to say CORRESPONDING BY (1 + 1)
- column list rule should be presented in document, too
- I don't see why you call checkWellFormedRecursionWalker on
corresponding clause

And more than above, Tom's point is the biggest blocker. I'd suggest
to rework it so that target list of Query of RangeTblEntry on the top
of tree have less columns that match the filter. By that way, I guess
you can keep original information as well as filtered top-most target
list. Eventually you need to work on the planner, too. Though I've not
read all of the patch, the design rework should be done first. I'll
mark this as Waiting on Author.

Regards,
--
Hitoshi Harada