Re: Incorrect handling of IS [NOT] NULL quals on inheritance parents

On Tue, 9 Apr 2024 at 21:55, Richard Guo <guofenglinux@gmail.com> wrote:
>
> In b262ad440e we introduced an optimization that drops IS NOT NULL quals
> on a NOT NULL column, and reduces IS NULL quals on a NOT NULL column to
> constant-FALSE.  I happened to notice that this is not working correctly
> for traditional inheritance parents.  Traditional inheritance parents
> might have NOT NULL constraints marked NO INHERIT, while their child
> tables do not have NOT NULL constraints.  In such a case, we would have
> problems when we have removed redundant IS NOT NULL restriction clauses
> of the parent rel, as this could cause NULL values from child tables to
> not be filtered out, or when we have reduced IS NULL restriction clauses
> of the parent rel to constant-FALSE, as this could cause NULL values
> from child tables to not be selected out.

hmm, yeah, inheritance tables were overlooked.

I looked at the patch and I don't think it's a good idea to skip
recording NOT NULL constraints to fix based on the fact that it
happens to result in this particular optimisation working correctly.
It seems that just makes this work in favour of possibly being wrong
for some future optimisation where we have something else that looks
at the RelOptInfo.notnullattnums and makes some decision that assumes
the lack of corresponding notnullattnums member means the column is
NULLable.

I think a better fix is just to not apply the optimisation for
inheritance RTEs in add_base_clause_to_rel().  If we do it this way,
it's only the inh==true RTE that we skip.  Remember that there are two
RangeTblEntries for an inheritance parent.  The other one will have
inh==false, and we can still have the optimisation as that's the one
that'll be used in the final plan.  It'll be the inh==true one that we
copy the quals from in apply_child_basequals(), so we've no need to
worry about missing baserestrictinfos when applying the base quals to
the child.

For partitioned tables, there's only a single RTE with inh==true.
We're free to include the redundant quals there to be applied or
skipped in apply_child_basequals().  The corresponding RangeTblEntry
is not going to be scanned in the final plan, so it does not matter
about the extra qual.

The revised patch is attached.

David