On Wed, May 24, 2023 at 2:48 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Richard Guo <guofengli...@gmail.com> writes: > > Considering that clone clauses should always be outer-join clauses not > > filter clauses, I'm wondering if we can add an additional check for that > > in RINFO_IS_PUSHED_DOWN, something like > > > #define RINFO_IS_PUSHED_DOWN(rinfo, joinrelids) \ > > - ((rinfo)->is_pushed_down || \ > > - !bms_is_subset((rinfo)->required_relids, joinrelids)) > > + (!((rinfo)->has_clone || ((rinfo)->is_clone)) && \ > > + ((rinfo)->is_pushed_down || \ > > + !bms_is_subset((rinfo)->required_relids, joinrelids))) > > I don't like that one bit; it seems way too likely to introduce > bad side-effects elsewhere. Agreed. I also do not have too much confidence in it. > I think the real issue is that "is pushed down" has never been a > conceptually accurate way of thinking about what > remove_rel_from_query needs to do here. Using RINFO_IS_PUSHED_DOWN > happened to work up to now, but it's an abuse of that macro, and > changing the macro's behavior isn't the right way to fix it. > > Having said that, I'm not sure what is a better way to think about it. > It seems like our data structure doesn't have a clear tie between > restrictinfos and their original join level, or at least, to the extent > that it did the "clone clause" mechanism has broken it. > > I wonder if we could do something involving recording the > rinfo_serial numbers of all the clauses extracted from a particular > syntactic join level (we could keep this in a bitmapset attached > to each SpecialJoinInfo, perhaps) and then filtering the joinclauses > on the basis of serial numbers instead of required_relids. I think this is a better way to fix the issue. I went ahead and drafted a patch as attached. But I doubt that the collecting of rinfo_serial numbers is thorough in the patch. Should we also collect the rinfo_serial of quals generated in reconsider_outer_join_clauses()? I believe that we do not need to consider the quals from generate_base_implied_equalities(), since they are all supposed to be restriction clauses not join clauses. Thanks Richard
v1-0001-WIP-Fix-the-outer-join-removal-problem.patch
Description: Binary data
