Thread: Query plan for "id IS NULL" on PK

On 2/14/23 15:04, Ben Chrobot wrote:
> Hello,
>
> Long time listener, first time caller.
>
> We have a large table (~470 million rows) with integer primary key id 
> (not null) on a Postgres 14.5 cluster. A third-party tool is 
> attempting to perform a SELECT-based full table copy in preparation 
> for log-based sync with a query like the following:
>
> SELECT "id", "other_column_a", "other_column_b", "created_at", 
> "updated_at"
> FROM "public"."my_large_table"
> WHERE (("id" > ? OR "id" IS NULL)) AND (("id" <= ?))
> ORDER  BY "id" LIMIT 50000;
>
> The lower bound increments by batch size (50k) while the upper bound 
> is always the `max(id)`, in our case around 575,000,000.
>
> The query plan produced is very slow as the index condition does 
> basically nothing:
>
>                         QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Limit  (cost=0.57..21901.46 rows=50000 width=417) (actual 
> time=1708920.675..1709198.995 rows=50000 loops=1)
>    ->  Index Scan using my_large_table_pkey on 
> my_large_table  (cost=0.57..135230792.97 rows=308733624 width=417) 
> (actual time=1708920.673..1709195.926 rows=50000 loops=1)
>          Index Cond: (id <= 575187488)
>          Filter: ((id > 193208795) OR (id IS NULL))
>          Rows Removed by Filter: 157784540
>  Planning Time: 0.186 ms
>  Execution Time: 1709200.618 ms
> (7 rows)
> Time: 1709231.721 ms (28:29.232)
>
> We cannot change the query being executed. Is there any way we can 
> make the query planner ignore `OR (id IS NULL)` (as that will never be 
> the case for the PK) and use both `id` clauses in the index condition?
>
> We have provided the vendor with the same query without the `id is 
> null` showing that it's significantly faster (see below). They have 
> informed us that addressing the null check on a not null PK is on 
> their roadmap to address but no timeline.
>
> explain analyze
> SELECT "id", "other_column_a", "other_column_b", "created_at", 
> "updated_at"
> FROM "public"."my_large_table"
> WHERE (("id" > ?)) AND (("id" <= ?))
> ORDER  BY "id" LIMIT 50000;
>
> QUERY PLAN
>
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
> Limit (cost=0.57..13930.19 rows=50000 width=416) (actual 
> time=2.118..400.937 rows=50000 loops=1)
> -> Index Scan using my_large_table_pkey on my_large_table 
> (cost=0.57..85429173.84 rows=306645829 width=416) (actual 
> time=2.117..398.325 rows=50000 loops=1)
> Index Cond: ((id > 193208795) AND (id <= 575187488))
> Planning Time: 0.166 ms
> Execution Time: 402.376 ms
>
> We have tried leading the planner to water with this view but it did 
> not change the slow query plan:
>
> create view my_fast_large_table as
> select *
> from my_large_table
> where id is not null;
>
> Any other tricks to try here?
>
> Thank you,
> Ben Chrobot

When will id be null in a primary key?

On Tue, Feb 14, 2023 at 3:25 PM Rob Sargent <robjsargent@gmail.com> wrote:

When will id be null in a primary key?

The OP seems to be aware of this...

"We cannot change the query being executed. Is there any way we can make the query planner ignore `OR (id IS NULL)` (as that will never be the case for the PK) and use both `id` clauses in the index condition?"

David J.

On 2023-02-14 17:04:51 -0500, Ben Chrobot wrote:
> We have a large table (~470 million rows) with integer primary key id (not
> null) on a Postgres 14.5 cluster. A third-party tool is attempting to perform a
> SELECT-based full table copy in preparation for log-based sync with a query
> like the following:
>
> SELECT "id", "other_column_a", "other_column_b", "created_at", "updated_at"
> FROM "public"."my_large_table"
> WHERE (("id" > ? OR "id" IS NULL)) AND (("id" <= ?))
> ORDER  BY "id" LIMIT 50000;

That doesn't make sense. ("id" <= ?) implies that ("id" IS NULL) is
FALSE. So the where clause can be simplified to
    WHERE (("id" > ? OR FALSE)) AND (("id" <= ?))
and then
    WHERE (("id" > ?)) AND (("id" <= ?))
even without the knowledge that "id" is a primary key (and therefore can
never be null).

Even if the column could contain NULL values, those would never be
selected.

It could therefore be argued that the query as written is broken and
should be fixed.

OTOH it could also be argued that the optimizer should be able to
perform the same simplifications as I did above and produce the same
code for WHERE (("id" > ? OR "id" IS NULL)) AND (("id" <= ?))
as for WHERE (("id" > ?)) AND (("id" <= ?)).

        hp

--
   _  | Peter J. Holzer    | Story must make more sense than reality.
|_|_) |                    |
| |   | hjp@hjp.at         |    -- Charles Stross, "Creative writing
__/   | http://www.hjp.at/ |       challenge!"

On 2023-02-14 15:36:32 -0700, Rob Sargent wrote:
> But if the query is supposed to be generic and re-used in a situation where id
> could be null, wouldn't the null id records be fetched every time? 

No, they will never be fetched because of the AND (("id" <= ?)).

        hp

--
   _  | Peter J. Holzer    | Story must make more sense than reality.
|_|_) |                    |
| |   | hjp@hjp.at         |    -- Charles Stross, "Creative writing
__/   | http://www.hjp.at/ |       challenge!"

On 2023-02-14 15:36:32 -0700, Rob Sargent wrote:

But if the query is supposed to be generic and re-used in a situation where id
could be null, wouldn't the null id records be fetched every time? 

No, they will never be fetched because of the AND (("id" <= ?)).
        hp

On Wed, 15 Feb 2023 at 11:39, Peter J. Holzer <hjp-pgsql@hjp.at> wrote:
> OTOH it could also be argued that the optimizer should be able to
> perform the same simplifications as I did above and produce the same
> code for WHERE (("id" > ? OR "id" IS NULL)) AND (("id" <= ?))
> as for WHERE (("id" > ?)) AND (("id" <= ?)).

You're right, and it has been brought up quite a few times in the
past.  To make it work, it's a fairly trivial change. We'd just need
to record all the attnotnull columns during something like
get_relation_info() then when adding baserestrictinfos to the base
relations, we could look to see if the qual is a NullTest and skip
that if we deem the qual as constantly true.

The problem with that is that doing that has an above zero cost and
since it likely only applies to nearly zero real-world cases, it just
does not seem like useful cycles to add to the planner. That might be
different if this was some optimisation that there was no other way to
make work, but that's not the case. All you need to do is remove the
redundant null check.   In the planner, if we had some other reason to
record which columns are NOT NULL then the additional overhead of just
looking at the NullTest quals would likely be cheap enough to be
worthwhile.  I imagine we'd need to find some other reason to record
attnotnull columns before we'd consider doing this.

David

David Rowley <dgrowleyml@gmail.com> writes:
> On Wed, 15 Feb 2023 at 11:39, Peter J. Holzer <hjp-pgsql@hjp.at> wrote:
>> OTOH it could also be argued that the optimizer should be able to
>> perform the same simplifications as I did above and produce the same
>> code for WHERE (("id" > ? OR "id" IS NULL)) AND (("id" <= ?))
>> as for WHERE (("id" > ?)) AND (("id" <= ?)).

> You're right, and it has been brought up quite a few times in the
> past.  To make it work, it's a fairly trivial change. We'd just need
> to record all the attnotnull columns during something like
> get_relation_info() then when adding baserestrictinfos to the base
> relations, we could look to see if the qual is a NullTest and skip
> that if we deem the qual as constantly true.

There's an order-of-operations issue that makes this more painful
than you might think at first.  In the above example, the NullTest
node *isn't* going to be a top-level restrictinfo: it's buried inside
an OR.  Really, the only reasonable place to suppress such a NullTest
is during eval_const_expressions, which already has the logic that would
get rid of the now-unnecessary OR above it.  And that's problematic
because it's done way ahead of where we know any relation-specific
information.  (Since eval_const_expressions happens ahead of join
removal, for $good_reasons, moving the plancat.c fetching to someplace
earlier than that wouldn't be cost-free either.)

> The problem with that is that doing that has an above zero cost and
> since it likely only applies to nearly zero real-world cases, it just
> does not seem like useful cycles to add to the planner.

Yeah, this.  In the end there is a low threshold on expensive stuff
that we're willing to do to clean up after brain-dead ORMs, because
the costs of that will also be paid by not-so-brain-dead applications.
In the example at hand, it's hard to argue that the query generator
sending this query shouldn't know better, since as Peter points out
the IS NULL check is redundant on its face, primary key or not.

            regards, tom lane

On 2/14/23 18:21, David Rowley wrote:
[snip]
> since it likely only applies to nearly zero real-world cases

Are you sure?

-- 
Born in Arizona, moved to Babylonia.