Horton

Thread

Commits

  1. Fix recently introduced typo in code comment

  2. Fix IS [NOT] NULL qual optimization for inheritance tables

  1. Incorrect handling of IS [NOT] NULL quals on inheritance parents

    Richard Guo <guofenglinux@gmail.com> — 2024-04-09T09:54:41Z

    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.  As an example, consider

create table p (a int);
create table c () inherits (p);

alter table only p alter a set not null;

insert into c values (null);

-- The IS NOT NULL qual is droped, causing the NULL value from 'c' to
-- not be filtered out
explain (costs off) select * from p where a is not null;
       QUERY PLAN
-------------------------
 Append
   ->  Seq Scan on p p_1
   ->  Seq Scan on c p_2
(3 rows)

select * from p where a is not null;
 a
---

(1 row)

-- The IS NULL qual is reduced to constant-FALSE, causing the NULL value
-- from 'c' to not be selected out
explain (costs off) select * from p where a is null;
        QUERY PLAN
--------------------------
 Result
   One-Time Filter: false
(2 rows)

select * from p where a is null;
 a
---
(0 rows)


To fix this issue, I think we can avoid calculating notnullattnums for
inheritance parents in get_relation_info().  Meanwhile, when we populate
childrel's base restriction quals from parent rel's quals, we check if
each qual can be proven always false/true, to apply the optimization we
have in b262ad440e to each child.  Something like attached.

This can also be beneficial to partitioned tables in cases where the
parent table does not have NOT NULL constraints, while some of its child
tables do.  Previously, the optimization introduced in b262ad440e was
not applicable in this case.  With this change, the optimization can now
be applied to each child table that has the right NOT NULL constraints.

Thoughts?

Thanks
Richard

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

    David Rowley <dgrowleyml@gmail.com> — 2024-04-10T05:13:22Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-10T05:18:26Z

    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

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

    David Rowley <dgrowleyml@gmail.com> — 2024-04-10T05:35:37Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2024-04-10T07:12:24Z

    On Wed, Apr 10, 2024 at 1:13 PM David Rowley <dgrowleyml@gmail.com> wrote:

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


Hmm, I have thought about your point, but I may have a different
perspective.  I think the oversight discussed here occurred because we
mistakenly recorded NOT NULL columns that are actually nullable for
traditional inheritance parents.  Take the query from my first email as
an example.  There are three RTEs: p(inh), p(non-inh) and c(non-inh).
And we've added a NOT NULL constraint on the column a of 'p' but not of
'c'.  So it seems to me that while we can mark column a of p(non-inh) as
non-nullable, we cannot mark column a of p(inh) as non-nullable, because
there might be NULL values in 'c' and that makes column a of p(inh)
nullable.

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.

Thanks
Richard

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

    David Rowley <dgrowleyml@gmail.com> — 2024-04-11T02:23:31Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2024-04-11T06:48:48Z

    On Thu, Apr 11, 2024 at 10:23 AM David Rowley <dgrowleyml@gmail.com> wrote:

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


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

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

    David Rowley <dgrowleyml@gmail.com> — 2024-04-12T08:19:32Z

    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

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

    Richard Guo <guofenglinux@gmail.com> — 2024-04-12T09:11:25Z

    On Fri, Apr 12, 2024 at 4:19 PM David Rowley <dgrowleyml@gmail.com> wrote:

> After further work on the comments, I pushed the result.
>
> Thanks for working on this.


Thanks for pushing!

BTW, I noticed a typo in the comment of add_base_clause_to_rel.

--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2644,7 +2644,7 @@ add_base_clause_to_rel(PlannerInfo *root, Index relid,
     * apply_child_basequals() sees, whereas the inh==false one is what's
used
     * for the scan node in the final plan.
     *
-    * We make an exception to this is for partitioned tables.  For these,
we
+    * We make an exception to this for partitioned tables.  For these, we

Thanks
Richard