On Thu, Oct 10, 2024 at 7:37 AM Richard Guo <guofengli...@gmail.com> wrote: > > > > > > So may > > > > be we should do this processing elsewhere and replace the original > > > > clause itself? > > > > > > I’m not sure about this. Different versions of the same qual clause > > > can lead to different conclusions about whether it can be reduced to > > > constant-FALSE. > > > > Can you please provide an example? > > I think the query I provided in my initial email serves as an example. > To quote what I said there: > > " > In the query above, the has_clone version of qual 't1.a is null' would > be reduced to constant-FALSE, while the is_clone version would not. > " > > > > I don't think it is possible to replace the original > > > clause; we need to do this processing on a version-by-version basis. > > > > > > > Either way, each version will be represented by only one RestrictInfo. > > Why can't we replace that version instead of creating a new > > RestrictInfo? > > Sure we can do that. However, I find that manually altering a > well-formed RestrictInfo's clause (along with its left_relids, > right_relids, clause_relids, num_base_rels, etc.) seems too > hacky to me.
That's fair. 1. What strikes me as odd in your patch is that the last_rinfo_serial is reset so far away from the new clause that will be created with the next last_rinfo_serial OR the clause whose clones are being created. It either indicates another site where we are missing (possibly future) to restore rinfo_serial in a clone OR reset of last_serial_rinfo needs to happen somewhere else. Why isn't resetting last_rinfo_serial in deconstruct_distribute_oj_quals() enough? 2. This change would also prevent add_base_clause_to_rel() and add_join_clause_to_rels() from being used anywhere outside the context of deconstruct_distribute_oj_quals() since these functions would reset last_rinfo_serial when it's expected to be incremented. Moreover another make_restrictinfo() call somewhere in the minions of distribute_qual_to_rels() or distribute_quals_to_rels() would cause a similar bug. 3. Do we need to reset last_serial_rinfo everywhere we reset rinfo_serial e.g. create_join_clause()? I suspect that b262ad440edecda0b1aba81d967ab560a83acb8a didn't do anything wrong. It's effectively copying rinfo_serial of original clause into the derived clause. I would have liked it better if it would have used the same method as create_join_clause(). Some other commti failed to notice the some minion of distribute_quals_to_rels()->distribute_qual_to_rels() may increment last_rinfo_serial while creating an alternate RestrictInfo for the qual passed to distribute_qual_to_rels(). I think a robust fix is to reset last_rinfo_serial in distribute_quals_to_rels() for every qual and add an Assert(root->last_rinfo_serial == saved_last_rinfo_serial + list_length(quals)) before resetting last_rinfo_serial in deconstruct_distribute_oj_quals() to catch any future digressions. -- Best Wishes, Ashutosh Bapat
