Re: not null constraints, again

On Fri, Sep 20, 2024 at 8:08 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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)