On Wed, 1 Nov 2023 at 15:21, Richard Guo <guofenglinux@gmail.com> wrote:
> I rebased this patch over the SJE commit
I rebased your v7 patch on top of 930d2b442 and updated the expected
results of some new regression tests which now have their NullTest
clauses removed.
I also renamed add_baserestrictinfo_to_rel() to
add_base_clause_to_rel() so that it's more aligned to
add_join_clause_to_rels().
On looking deeper, I see you're overwriting the rinfo_serial of the
const-false RestrictInfo with the one from the original RestrictInfo.
If that's the correct thing to do then the following comment would
need to be updated to mention this exception of why the rinfo_serial
isn't unique.
/*----------
* Serial number of this RestrictInfo. This is unique within the current
* PlannerInfo context, with a few critical exceptions:
* 1. When we generate multiple clones of the same qual condition to
* cope with outer join identity 3, all the clones get the same serial
* number. This reflects that we only want to apply one of them in any
* given plan.
* 2. If we manufacture a commuted version of a qual to use as an index
* condition, it copies the original's rinfo_serial, since it is in
* practice the same condition.
* 3. RestrictInfos made for a child relation copy their parent's
* rinfo_serial. Likewise, when an EquivalenceClass makes a derived
* equality clause for a child relation, it copies the rinfo_serial of
* the matching equality clause for the parent. This allows detection
* of redundant pushed-down equality clauses.
*----------
*/
Looking at the tests, I see:
select * from pred_tab t1 left join pred_tab t2 on true left join
pred_tab t3 on t2.a is null;
I'm wondering if you can come up with a better test for this? I don't
quite see any reason why varnullingrels can't be empty for t2.a in the
join qual as the "ON true" join condition between t1 and t2 means that
there shouldn't ever be any NULL t2.a rows. My thoughts are that if
we improve how varnullingrels are set in the future then this test
will be broken.
Also, I also like to write exactly what each test is testing so that
it's easier in the future to maintain the expected results. It's
often tricky when making planner changes to know if some planner
changes makes a test completely useless or if the expected results
just need to be updated. If someone changes varnullingrels to be
empty for this case, then if they accept the actual results as
expected results then the test becomes useless. I tend to do this
with comments in the .sql file along the lines of "-- Ensure ..."
I also would rather see the SQLs in the test wrap their lines before
each join and the keywords to be upper case.
David