Re: question about is_valid parameter in set_attnotnull

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