I ended up moving the function that looks for the NullTest quals in the joinlist out so it's done after the quals have been distributed to the relations.
It seems that the RestrictInfos for the "same" qual condition still may get different serial numbers even if transform_join_clauses() is called after we've distributed all the quals. For example,
select * from t1 left join t2 on t1.a = t2.c left join t3 on t2.c = t3.e and t2.c is null;
There are two versions for qual 't2.c is null': with and without being marked nullable by t1/t2 join. Let's write them as 'c* is null' and 'c is null'. They are supposed to have identical serial number. But after we've transformed 'c is null' to 'false', they do not have identical serial number any more. This may cause problems where the logic of serial numbers is relied on?
I'm not really that happy with this as if we ever found some way to optimise quals that could be made part of an EquivalenceClass then those quals would have already have been processed to become EquivalenceClasses. I just don't see how to do it earlier as deconstruct_distribute_oj_quals() calls remove_nulling_relids() which changes the Var's varnullingrels causing them to be empty during the processing of the NullTest qual.
Hmm, I don't think it's a problem that deconstruct_distribute_oj_quals changes the nullingrels. It will compute the correct nullingrels at last for different clones of the same qual condition. We can just check the nullingrels whatever it computes.
It's also not so great that the RestrictInfo gets duplicated in:
Adjusting the code to build a new false clause and setting that in the existing RestrictInfo rather than building a new RestrictInfo seems to fix that. I wondered if the duplication was a result of the rinfo_serial number changing.
The RestrictInfo nodes in different joinlists are multiply-linked rather than copied, so when building restrictlist for a joinrel we use pointer equality to remove any duplication. In your patch new RestrictInfo nodes are created in transform_join_clauses(), so pointer equality no longer works and we see duplication in the plan.
Checking back to the original MinMaxAgg I'm not sure if this is all getting more complex than it's worth or not.
It seems that optimizing IS NULL quals is more complex than optimizing IS NOT NULL quals. I also wonder if it's worth the trouble to optimize IS NULL quals.
BTW, there is an Assert failure running regression tests with your patch. I haven't looked into it.
