Re: question about is_valid parameter in set_attnotnull

> On Jul 30, 2026, at 02:55, Sami Imseih <samimseih(at)gmail(dot)com> wrote:
>
> Thanks for looking!
>
>>> I was looking at code in tablecmds.c for some other patch and I
>>> spotted that the is_valid parameter in set_attnotnull(), added in
>>> a379061a22a8, is never used in the function body. It's not clear
>>> why it was added, except that it's passed by the caller. The
>>> CompactAttribute is unconditionally set to ATTNULLABLE_VALID:
>>>
>>> ```
>>> thisatt = TupleDescCompactAttr(RelationGetDescr(rel), attnum - 1);
>>> thisatt->attnullability = ATTNULLABLE_VALID;
>>> ```
>>>
>>> The only caller that passes is_valid=false is ATAddCheckNNConstraint
>>> (for NOT VALID constraints).
>>
>> Hmm, yeah -- as I recall, the intention there is that is_valid would be
>> false when you want to create an invalid constraint. I suppose it was
>> used that way during development but we later changed our minds about
>> the design, made the parameter obsolete, and forgot to set it correctly
>> or remove it. I would have to study the code to remember in more
>> detail, but I suspect the right action now is to remove it.
>
> At least I could not find any reason for it, but I may have missed something.
> The function header comment and callers never mention is_valid either;
> only attnotnull and queue_validation.
>
>> Maybe we
>> don't need thisatt at all and it should also be removed?
>
> Right, both the is_valid and thisatt are dead code AFAICT.
>
> Here is a patch that cleans it up.
>
> --
> Sami
> <v1-0001-Remove-dead-code-in-set_attnotnull.patch>

Even from a purely C programming perspective, leaving the unused argument and local variable hurts readability. So +1 for removing them.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/