Removing const-false IS NULL quals and redundant IS NOT NULL quals

(Moving discussion from -bugs [1] to -hackers for more visibility.)

Background:
This started out as a performance fix for bug #17540 but has now
extended beyond that as fixing that only requires we don't add
redundant IS NOT NULL quals to Min/Max aggregate rewrites. The
attached gets rid of all IS NOT NULL quals on columns that are
provably not null and replaces any IS NULL quals on NOT NULL columns
with a const-false gating qual which could result in not having to
scan the relation at all.

explain (costs off) select * from pg_class where oid is null;
QUERY PLAN
--------------------------
Result
One-Time Filter: false

The need for this is slightly higher than it once was as the self-join
removal code must add IS NOT NULL quals when removing self-joins when
the join condition is strict.

explain select c1.* from pg_class c1 inner join pg_class c2 on c1.oid=c2.oid;
QUERY PLAN
----------------------------------------------------------------
Seq Scan on pg_class c2 (cost=0.00..18.15 rows=415 width=273)

master would contain an oid IS NOT NULL filter condition.

On Fri, 1 Dec 2023 at 23:07, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>
> On Wed, Nov 29, 2023 at 8:48 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>> 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.
>
>
> Right, that's what we need to do.
>
>>
>> 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.
>
>
> Thanks for the suggestions on the tests. I had a go at improving the
> test queries and their comments.

Thanks. I made a pass over this patch which resulted in just adding
and tweaking some comments.

The other thing that bothers me about this patch now is the lack of
simplification of OR clauses with a redundant condition. For example:

postgres=# explain (costs off) select * from pg_class where oid is
null or relname = 'non-existent';
QUERY PLAN
---------------------------------------------------------------------
Bitmap Heap Scan on pg_class
Recheck Cond: ((oid IS NULL) OR (relname = 'non-existant'::name))
-> BitmapOr
-> Bitmap Index Scan on pg_class_oid_index
Index Cond: (oid IS NULL)
-> Bitmap Index Scan on pg_class_relname_nsp_index
Index Cond: (relname = 'non-existant'::name)
(7 rows)

oid is null is const-false and if we simplified that to remove the
redundant OR branch and run it through the constant folding code, we'd
end up with just the relname = 'non-existent' and we'd end up with a
more simple plan as a result.

I don't think that's a blocker. I think the patch is ready to go even
without doing anything to improve that.

Happy to hear other people's thoughts on this patch. Otherwise, I
currently don't think the missed optimisation is a reason to block
what we've ended up with so far.

David

[1] https://postgr.es/m/flat/17540-7aa1855ad5ec18b4%40postgresql.org