Обсуждение: 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@gmail.com> wrote:
>
>
> On Wed, Nov 29, 2023 at 8:48 AM David Rowley <dgrowleyml@gmail.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

Hi,

David Rowley <dgrowleyml@gmail.com> writes:

>
> 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
>
> [2. application/x-patch; v10-0001-Reduce-NullTest-quals-to-constant-TRUE-or-FALSE.patch]...

Thanks for working on this, an I just get a complaint about this missed
optimisation 7 hours ago..

I also want to add notnullattnums for the UniqueKey stuff as well, by
comparing your implementation with mine,  I found you didn't consider
the NOT NULL generated by filter. After apply your patch:

create table a(a int);
explain (costs off) select * from a where a > 3 and a is null;
             QUERY PLAN              
-------------------------------------
 Seq Scan on a
   Filter: ((a IS NULL) AND (a > 3))
(2 rows)

This is acutally needed by UniqueKey stuff, do you think it should be
added? To save some of your time, you can check what I did in UniqueKey

[1]
https://www.postgresql.org/message-id/attachment/151254/v1-0001-uniquekey-on-base-relation-and-used-it-for-mark-d.patch

-- 
Best Regards
Andy Fan

Richard Guo <guofenglinux@gmail.com> writes:
>
> The detection of self-inconsistent restrictions already exists in
> planner.
>
> # set constraint_exclusion to on;
> SET
> # explain (costs off) select * from a where a > 3 and a is null;
>         QUERY PLAN
> --------------------------
>  Result
>    One-Time Filter: false
> (2 rows)

It has a different scope and cost from what I suggested.  I'd suggest
to detect the notnull constraint only with lower cost and it can be used
in another user case. the constaint_exclusion can covers more user
cases but more expensivly and default off.


Apart from the abve topic, I'm thinking if we should think about the
case like this: 

create table t1(a int);
create table t2(a int);

explain (costs off) select * from t1 join t2 using(a) where a is NULL;
            QUERY PLAN             
-----------------------------------
 Hash Join
   Hash Cond: (t2.a = t1.a)
   ->  Seq Scan on t2
   ->  Hash
         ->  Seq Scan on t1
               Filter: (a IS NULL)

Here a is nullable at the base relation side, but we know that the query
would not return anything at last.  IIUC, there is no good place to
handle this in our current infrastructure, I still raise this up in case
I missed anything.


-- 
Best Regards
Andy Fan

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please
have a look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4459/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4459

Kind Regards,
Peter Smith.

On Thu, 28 Dec 2023 at 00:38, Andy Fan <zhihuifan1213@163.com> wrote:
> I also want to add notnullattnums for the UniqueKey stuff as well, by
> comparing your implementation with mine,  I found you didn't consider
> the NOT NULL generated by filter. After apply your patch:
>
> create table a(a int);
> explain (costs off) select * from a where a > 3 and a is null;
>              QUERY PLAN
> -------------------------------------
>  Seq Scan on a
>    Filter: ((a IS NULL) AND (a > 3))
> (2 rows)

> [1]
>
https://www.postgresql.org/message-id/attachment/151254/v1-0001-uniquekey-on-base-relation-and-used-it-for-mark-d.patch

I believe these are two different things and we should not mix the two up.

Looking at your patch, I see you have:

+ /* The not null attrs from catalogs or baserestrictinfo. */
+ Bitmapset  *notnullattrs;

Whereas, I have:

/* zero-based set containing attnums of NOT NULL columns */
Bitmapset  *notnullattnums;

I'm a bit worried that your definition of notnullattrs could lead to
confusion about which optimisations will be possible.

Let's say for example I want to write some code that optimises the
expression evaluation code to transform EEOP_FUNCEXPR_STRICT into
EEOP_FUNCEXPR when all function arguments are Vars that have NOT NULL
constraints and are not nullable by any outer join.  With my
definition, it should be safe to do this, but with your definition, we
can't trust we'll not see any NULLs as if the strict function is
evaluated before the strict base qual that filters the NULLs then the
strict function could be called with NULL.

Perhaps we'd want another Bitmapset that has members for strict OpExrs
that filter NULLs and we could document that it's only safe to assume
there are no NULLs beyond the scan level.... but I'd say that's
another patch and I don't want to feed you design ideas here and
derail this patch.

David

On Mon, 22 Jan 2024 at 17:32, Peter Smith <smithpb2250@gmail.com> wrote:
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there were CFbot test failures last time it was run [2].

I've attached v11 which updates the expected results in some newly
added regression tests.

No other changes.

David

On Tue, 23 Jan 2024 at 00:11, David Rowley <dgrowleyml@gmail.com> wrote:
> I've attached v11 which updates the expected results in some newly
> added regression tests.

I went over this again. I did a little more work adjusting comments
and pushed it.

Thanks for all your assistance with this, Richard.

David