Re: Remove redundant NULL check in clause_selectivity_ext() function

Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com> writes:
> In the `clause_selectivity_ext()` function, there’s a comment regarding
> a NULL clause check that asks, "can this still happen?". I decided to
> investigate whether this scenario is still valid.

> Here’s what I found after reviewing the relevant cases:

> - approx_tuple_count(): The function iterates over a loop of other clauses.
> - get_foreign_key_join_selectivity(): The function is invoked within an
> `if (clause)` condition.
> - consider_new_or_clause(): The clause is generated by
> `make_restrictinfo()`, which never returns NULL.
> - clauselist_selectivity_ext(): This function either checks
> `list_length(clauses) == 1` before being called, or it is called within
> a loop of clauses.

That list_length check doesn't prove anything about whether the list
element is NULL, though.

While I suspect that you may be right that the case doesn't occur
anymore (if it ever did), I'm inclined to leave this test in place.
It's cheap enough compared to what the rest of the function will do,
and more importantly we can't assume that all interesting call sites
are within Postgres core. There are definitely extensions calling
clauselist_selectivity and related functions. It's possible that
some of them rely on clause_selectivity not crashing on a NULL.
Certainly, such an assumption could be argued to be a bug they
should fix; but I'm disinclined to make them jump through that
hoop for a vanishingly small performance improvement.

Also, there are boatloads of other places where the planner has
possibly-redundant checks for null clause pointers. It's likely
that some of the other ones are more performance-critical than
this. But I wouldn't be in favor of retail removal of the others
either. Maybe with a more holistic approach we could remove a
whole lot of them and make a measurable improvement; but it
would require some careful thought about what invariants we
want to assume. There's not really any design principles
about this right now, and where we've ended up is that most
functions operating on expression trees assume they've got to
defend against NULL inputs. To remove those checks, we'd
need a clear understanding of where caller-side checks need
to be added instead.

regards, tom lane