Re: not null constraints, again

On Wed, Dec 4, 2024 at 10:52 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
>
> heap_create_with_catalog argument (cooked_constraints):
> passed as NIL in function {create_toast_table, make_new_heap}
> passed as list_concat(cookedDefaults,old_constraints) in DefineRelation
>
> in DefineRelation we have function call:
> MergeAttributes
> heap_create_with_catalog
> StoreConstraints
>
> StoreConstraints second argument: cooked_constraints, some is comes from
> DefineRelation->MergeAttributes old_constraints:
> {
> stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids,
> stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints,
> &old_notnulls);
> }
>
> My understanding from DefineRelation->MergeAttributes is that old_constraints
> will only have CHECK constraints.
> that means heap_create_with_catalog->StoreConstraints
> StoreConstraints didn't actually handle CONSTR_NOTNULL.
>
> heap_create_with_catalog comments also says:
> * cooked_constraints: list of precooked check constraints and defaults
>
> coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html
> also shows StoreConstraints, CONSTR_NOTNULL never being called,
> which is added by this thread.
>
>
> my question is can we remove StoreConstraints, CONSTR_NOTNULL handling.
> we have 3 functions {StoreConstraints, AddRelationNotNullConstraints,
> AddRelationNewConstraints} that will call StoreRelNotNull to store the not-null
> constraint. That means if we want to bullet proof that something is conflicting
> with not-null, we need to add code to check all these 3 places.
> removing StoreConstraints handling not-null seems helpful.
>
>
> also comments in MergeAttributes:
> * Output arguments:
> * 'supconstr' receives a list of constraints belonging to the parents,
> * updated as necessary to be valid for the child.
> * 'supnotnulls' receives a list of CookedConstraints that corresponds to
> * constraints coming from inheritance parents.
>
> can we be explicit that "supconstr" is only about CHECK constraint,
> "supnotnulls" is
> only about NOT-NULL constraint.

patch attached.

also change comments of heap_create_with_catalog,
StoreConstraints, MergeAttributes.
so we can clear idea what's kind of constraints we are dealing with
in these functions.