Thread: Re: not null constraints, again

On 2024-Sep-20, jian he wrote:

> about set_attnotnull.
> 
> we can make set_attnotnull  look less recursive.
> instead of calling find_inheritance_children,
> let's just one pass, directly call  find_all_inheritors
> overall, I think it would be more intuitive.
> 
> please check the attached refactored set_attnotnull.
> regress test passed, i only test regress.

Hmm, what do we gain from doing this change?  It's longer in number of
lines of code, and it's not clear to me that it is simpler.

> I am also beginning to wonder if ATExecSetNotNull inside can also call
> find_all_inheritors.

The point of descending levels one by one in ATExecSetNotNull is that we
can stop for any child on which a constraint already exists.  We don't
need to scan any children thereof, which saves work.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"

On Fri, Sep 20, 2024 at 8:08 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Sep-20, jian he wrote:
>
> > about set_attnotnull.
> >
> > we can make set_attnotnull  look less recursive.
> > instead of calling find_inheritance_children,
> > let's just one pass, directly call  find_all_inheritors
> > overall, I think it would be more intuitive.
> >
> > please check the attached refactored set_attnotnull.
> > regress test passed, i only test regress.
>
> Hmm, what do we gain from doing this change?  It's longer in number of
> lines of code, and it's not clear to me that it is simpler.
>


static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
               LOCKMODE lockmode)
{
    HeapTuple    tuple;
    Form_pg_attribute attForm;
    bool        changed = false;
    List       *all_oids;
    Relation    thisrel;
    AttrNumber    childattno;
    const char *attrname;
    CheckAlterTableIsSafe(rel);
    attrname = get_attname(RelationGetRelid(rel), attnum, false);
    if (recurse)
        all_oids = find_all_inheritors(RelationGetRelid(rel), lockmode,
                                         NULL);
    else
        all_oids = list_make1_int(RelationGetRelid(rel));
    foreach_oid(reloid, all_oids)
    {
        thisrel = table_open(reloid, NoLock);
        if (reloid != RelationGetRelid(rel))
            CheckAlterTableIsSafe(thisrel);
        childattno = get_attnum(reloid, attrname);
        tuple = SearchSysCacheCopyAttNum(reloid, childattno);
        if (!HeapTupleIsValid(tuple))
            elog(ERROR, "cache lookup failed for attribute %d of relation %s",
                attnum, RelationGetRelationName(thisrel));
        attForm = (Form_pg_attribute) GETSTRUCT(tuple);
        if (!attForm->attnotnull)
        {
            Relation    attr_rel;
            attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
            attForm->attnotnull = true;
            CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
            table_close(attr_rel, RowExclusiveLock);
            if (wqueue && !NotNullImpliedByRelConstraints(thisrel, attForm))
            {
                AlteredTableInfo *tab;
                tab = ATGetQueueEntry(wqueue, thisrel);
                tab->verify_new_notnull = true;
            }
            changed = true;
        }
        if (changed)
            CommandCounterIncrement();
        changed = false;
        table_close(thisrel, NoLock);
    }
}


What do you think of the above refactor?
(I intentionally deleted empty new line)

On 2024-Sep-24, jian he wrote:

> static void
> set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
>                LOCKMODE lockmode)
> {

> What do you think of the above refactor?
> (I intentionally deleted empty new line)

Looks nicer ... but you know what?  After spending some more time with
it, I realized that one caller is dead code (in
AttachPartitionEnsureIndexes) and another caller doesn't need to ask for
recursion, because it recurses itself (in ATAddCheckNNConstraint).  So
that leaves us with a grand total of zero callers that need the
recursion here ... which means we can simplify it to the case that it
only examines a single relation and never recurses.

So I've stripped it down to its bare minimum:

/*
 * Helper to set pg_attribute.attnotnull if it isn't set, and to tell phase 3
 * to verify it.
 *
 * When called to alter an existing table, 'wqueue' must be given so that we
 * can queue a check that existing tuples pass the constraint.  When called
 * from table creation, 'wqueue' should be passed as NULL.
 */
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum,
               LOCKMODE lockmode)
{
    Oid            reloid = RelationGetRelid(rel);
    HeapTuple    tuple;
    Form_pg_attribute attForm;

    CheckAlterTableIsSafe(rel);

    tuple = SearchSysCacheCopyAttNum(reloid, attnum);
    if (!HeapTupleIsValid(tuple))
        elog(ERROR, "cache lookup failed for attribute %d of relation %u",
             attnum, reloid);
    attForm = (Form_pg_attribute) GETSTRUCT(tuple);
    if (!attForm->attnotnull)
    {
        Relation    attr_rel;

        attr_rel = table_open(AttributeRelationId, RowExclusiveLock);

        attForm->attnotnull = true;
        CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);

        if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm))
        {
            AlteredTableInfo *tab;

            tab = ATGetQueueEntry(wqueue, rel);
            tab->verify_new_notnull = true;
        }

        CommandCounterIncrement();

        table_close(attr_rel, RowExclusiveLock);
    }

    heap_freetuple(tuple);
}

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/