Обсуждение: [MASSMAIL]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

David Rowley <dgrowleyml@gmail.com> writes:
> I think a better fix is just to not apply the optimisation for
> inheritance RTEs in add_base_clause_to_rel().

Is it worth paying attention to whether the constraint is marked
connoinherit?  If that involves an extra syscache fetch, I'd tend to
agree that it's not worth it; but if we can get that info for free
it seems worthwhile to not break this for inheritance cases.

            regards, tom lane

On Wed, 10 Apr 2024 at 17:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I think a better fix is just to not apply the optimisation for
> > inheritance RTEs in add_base_clause_to_rel().
>
> Is it worth paying attention to whether the constraint is marked
> connoinherit?  If that involves an extra syscache fetch, I'd tend to
> agree that it's not worth it; but if we can get that info for free
> it seems worthwhile to not break this for inheritance cases.

I think everything should be optimised as we like without that.
Effectively get_relation_info() looks at the pg_attribute.attnotnull
column for the relation in question. We never look at the
pg_constraint record to figure out the nullability of the column.

David

On Wed, 10 Apr 2024 at 19:12, Richard Guo <guofenglinux@gmail.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.

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.

Resulting patch attached.

David

On Thu, 11 Apr 2024 at 18:49, Richard Guo <guofenglinux@gmail.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