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

Поиск
Список
Период
Сортировка
От Richard Guo
Тема Re: Incorrect handling of IS [NOT] NULL quals on inheritance parents
Дата
Msg-id CAMbWs49LC-qLKL=-ezCHY-_V+ttS0Zi-MuU66WOrj3db2pKW=Q@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Incorrect handling of IS [NOT] NULL quals on inheritance parents  (David Rowley <dgrowleyml@gmail.com>)
Ответы Re: Incorrect handling of IS [NOT] NULL quals on inheritance parents  (David Rowley <dgrowleyml@gmail.com>)
Список pgsql-hackers

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
Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: jian he
Дата:
Сообщение: Re: Can't find not null constraint, but \d+ shows that
Следующее
От: Ashutosh Bapat
Дата:
Сообщение: Re: apply_scanjoin_target_to_paths and partitionwise join