2017-03-18 17:50 GMT+01:00 Tom Lane <t...@sss.pgh.pa.us>: > Pavel Stehule <pavel.steh...@gmail.com> writes: > > I have not any objection - I'll mark this patch as ready for commiter > > I took a quick look through this and noted that it fails to touch > ruleutils.c, which means that dumping of views containing CORRESPONDING > certainly doesn't work. >
it my mistake - this bug I should to see :( > > Also, the changes in parser/analyze.c seem rather massive and > correspondingly hard to review. Is it possible to rearrange the > patch to reduce the size of that diff? If you can avoid moving > or reindenting existing code, that'd help. > > The code in that area seems rather confused, too. For instance, > I'm not sure exactly what orderCorrespondingList() is good for, > but it certainly doesn't look to be doing anything that either its > name or its header comment (or the comments at the call sites) would > suggest. Its input and output tlists are always in the same order. > > I'm a little disturbed by the fact that determineMatchingColumns() > is called twice, and more disturbed by the fact that it looks to be > O(N^2). This could be really slow with a lot of columns, couldn't it? > > I also think there should be some comments about exactly what matching > semantics we're enforcing. The SQL standard says > > a) If CORRESPONDING is specified, then: > i) Within the columns of T1, equivalent <column name>s shall > not be specified more than once and within the columns of > T2, equivalent <column name>s shall not be specified more > than once. > > That seems unreasonably stringent to me; it ought to be sufficient to > forbid duplicates of the names listed in CORRESPONDING, or the common > column names if there's no BY list. But whichever restriction you prefer, > this code seems to be failing to check it --- I certainly don't see any > new error message about "column name "foo" appears more than once". > > I don't think you want to be leaving behind debugging cruft like this: > + elog(DEBUG4, "%s", ltle->resname); > > I'm not impressed by using A_Const for the members of the CORRESPONDING > name list. That's not a clever solution, that's a confusing kluge, > because it's a complete violation of the meaning of A_Const. Elsewhere > we just use lists of String for name lists, and that seems sufficient > here. Personally I'd just use the existing columnList production rather > than rolling your own. > The reason was attach a location to name for more descriptive error message. > The comments in parsenodes.h about the new fields seem rather confused, > in fact I think they're probably backwards. > > Also, I thought the test cases were excessively uninspired and repetitive. > This, for example, seems like about two tests too many: > > +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(a) SELECT 4 a, 5 b, 6 c; > + a > +--- > + 1 > + 4 > +(2 rows) > + > +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(b) SELECT 4 a, 5 b, 6 c; > + b > +--- > + 2 > + 5 > +(2 rows) > + > +SELECT 1 a, 2 b, 3 c UNION CORRESPONDING BY(c) SELECT 4 a, 5 b, 6 c; > + c > +--- > + 3 > + 6 > +(2 rows) > > without even considering the fact that these are pretty duplicative of > tests around them. And some of the added tests seem to be just testing > basic UNION functionality, which we already have tests for. I fail to > see the point of adding any of these: > > +SELECT 1 AS two UNION CORRESPONDING SELECT 2 two; > + two > +----- > + 1 > + 2 > +(2 rows) > + > +SELECT 1 AS one UNION CORRESPONDING SELECT 1 one; > + one > +----- > + 1 > +(1 row) > + > +SELECT 1 AS two UNION ALL CORRESPONDING SELECT 2 two; > + two > +----- > + 1 > + 2 > +(2 rows) > + > +SELECT 1 AS two UNION ALL CORRESPONDING SELECT 1 two; > + two > +----- > + 1 > + 1 > +(2 rows) > > What I think actually is needed is some targeted testing showing > non-interference with nearby features. As an example, CORRESPONDING > can't safely be implemented by just replacing the tlists of the > input sub-selects with shortened versions, because that would break > situations such as DISTINCT in the sub-selects. I think it'd be a > good idea to have a test along the lines of > > SELECT DISTINCT x, y FROM ... > UNION ALL CORRESPONDING > SELECT DISTINCT x, z FROM ... > > with values chosen to show that the DISTINCT operators did operate > on x/y and x/z, not just x alone. > > regards, tom lane >
