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

On Thu, Apr 11, 2024 at 10:23 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> On Wed, 10 Apr 2024 at 19:12, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > And I think recording NOT NULL columns for traditional inheritance
> > parents can be error-prone for some future optimization where we look
> > at an inheritance parent's notnullattnums and make decisions based on
> > the assumption that the included columns are non-nullable. The issue
> > discussed here serves as an example of this potential problem.
>
> I admit that it seems more likely that having a member set in
> notnullattnums for an inheritance parent is more likely to cause
> future bugs than if we just leave them blank. But I also don't
> believe leaving them all blank is the right thing unless we document
> that the field isn't populated for traditional inheritance parent
> tables and if the code needs to not the NOT NULL status of a column
> for that table ONLY, then the code should look at the RelOptInfo
> corresponding to the inh==false RangeTblEntry for that relation. If we
> don't document the fact that we don't set the notnullattnums field
> then someone might write some code thinking we correctly populate it.
> If the parent and all children have NOT NULL constraints for a
> column, then unless we document we don't populate notnullattnums, it
> seems reasonable to assume that's a bug.

Fair point. I agree that we should document that we don't populate
notnullattnums for traditional inheritance parents.

> If we skip populating notnullattnums for inh==true non-partitioned
> tables, I think we also still need to skip applying the NOT NULL qual
> optimisation for inh==true RTEs as my version of the code did.
> Reasons being: 1) it's a pointless exercise since we'll always end up
> adding the RestrictInfo without modification to the RelOptInfo's
> baserestrictinfo, and 2) The optimisation in question would be looking
> at the notnullattnums that isn't correctly populated.

I agree with both of your points. But I also think we do not need to
skip applying the NOT NULL qual optimization for partitioned tables.
For partitioned tables, if the parent is marked NOT NULL, then all its
children must also be marked NOT NULL. And we've already populated
notnullattnums for partitioned tables in get_relation_info. Applying
this optimization for partitioned tables can help save some cycles in
apply_child_basequals if we've reduced or skipped some restriction
clauses for a partitioned table. This means in add_base_clause_to_rel
we need to also check rte->relkind:

- if (!rte->inh)
+ if (!rte->inh || rte->relkind == RELKIND_PARTITIONED_TABLE)

I also think we should update the related comments for
apply_child_basequals and its caller, as my v1 patch does, since now we
might reduce or skip some of the resulting clauses.

Attached is a revised patch.

Thanks
Richard