David Rowley <david.row...@2ndquadrant.com> wrote: > I've attached an updated version of this patch.
Following are the findings from my review. On the LATERAL references: This query (proposed upthread by Tom and adjusted by me so it can be executed on the your test tables) select distinct t1.id from t1 left join t2 on t1.id=t2.b, lateral nextval('foo'); is subject to join removal because in rel_distinctified_after_join() you check brel->lateral_vars, which is NIL in this case. On the other hand I'm not sure that user should expect any number of executions of the function in this particular case because the actual join order can change: it can happen that the function scan is first joined to t1 and t2 is joined to the result. The case that requires more caution is that the function references both t1 and t2, i.e. *all* tables of the left join from which you want to remove the inner relation. Maybe that's the reason you introduced the test for brel->lateral_vars, but you forgot to check the actual variables in the list. On the volatile function in the targetlist: Volatile function in the DISTINCT ON expression isn't necessarily noticed by your checks: select distinct on (nextval('foo')) t1.id from t1 left join t2 on t1.id=t2.b; Perhaps you should simply check the whole targetlist if there's no GROUP BY clause. On coding: * join_is_removable(): Just like rel_distinctified_after_join() is called before rel_is_distinct_for() - obviously because it's cheaper - I suggest to call query_distinctifies_rows() before rel_supports_distinctness(). * header comment of query_distinctifies_rows() mentions rel_tuples_needed, but I can't find such a function. * rel_distinctified_after_join() does not really use the "rel" argument. Besides removing it you probably should rename the function. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at