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

On Thu, 11 Apr 2024 at 18:49, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> 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 was skipping this on purpose as I wasn't sure that we'd never expand
restriction_is_always_false() and restriction_is_always_true() to also
handle partition constraints. However, after thinking about that
more, the partition constraint can only become more strict the deeper
down the partition levels you go. If it was possible to insert a row
directly into a leaf partition that wouldn't be allowed when inserting
via the parent then there's a bug in the partition constraint.

I also considered if we'd ever add support to remove redundant quals
in CHECK constraints and if there was room for problematic mismatches
between partitioned table and leaf partitions, but I see we've thought
about that:

postgres=# alter table only p add constraint p_check check (a = 0);
ERROR: constraint must be added to child tables too

> 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.

I felt the comments you wanted to add at the call site of
apply_child_basequals() knew too much about what lies within that
function. The caller needn't know anything about what optimisations
are applied in apply_child_basequals(). In my book, that's just as bad
as documenting things about the calling function from within a
function. Since functions are designed to be reused, you're just
asking for such comments to become outdated as soon as we teach
apply_child_basequals() some new tricks. In this case, all the caller
needs to care about is properly handling a false return value.

After further work on the comments, I pushed the result.

Thanks for working on this.

David