Re: Re: starting to review the Extend NOT NULL representation to pg_constraint patch

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> Here is an updated version of the patch. It fixes the following issues
> Andrew discovered during his review cycle:

I looked through this a bit. It's not ready to commit unfortunately.
The main gripe I've got is that you've paid very little attention to
updating comments that your code changes invalidate. That's not even
a little bit acceptable: people will still have to read this code later.
Two examples are that struct CookedConstraint is now used for notnull
constraints in addition to its other duties, but you didn't adjust the
comments in its declaration; and that you made transformColumnDefinition
put NOTNULL constraints into the ckconstraints list, ignoring the fact
that both its name and the comments say that that's only CHECK
constraints. In the latter case it'd probably be a better idea to add
a separate "nnconstraints" list to CreateStmtContext.

Some other random points:

* The ALTER TABLE changes seem to be inserting a whole lot of
CommandCounterIncrement calls in places where there were none before.
Do we really need those? Usually the theory is that one at the end
of an operation is sufficient.

* All those bits with deconstruct_array could usefully be folded into
a subroutine, along the lines of
bool constraint_is_for_single_column(HeapTuple constraintTup, int attno)

* As best I can tell from looking, the patch *always* generates a name
for NOT NULL constraints. It should honor the user's name for the
constraint if one is given, ie
create table foo (f1 int constraint nn1 not null);
Historically we've had to drop such names on the floor, but that should
stop.

* pg_dump almost certainly needs some updates. I imagine the behavior
we'll want from it is to print NOT NULL only when the column's notnull
constraint shows that it's locally defined. If it gets printed for
columns that only have an inherited NOT NULL, then dump and reload
will change the not-null inheritance state. This may be a bit tricky
since pg_dump also has to still work against older servers, but with
any luck you can steal its logic for inherited check constraints.
We probably want it to start preserving the names of notnull
constraints, too.

* In general you should probably search for all places that reference
pg_constraint.contype, as a means of spotting any other code that needs
to be updated for this.

* Lots of bogus trailing whitespace. "git diff --check" can help you
with that. Also, please try to avoid unnecessary changes of whitespace
on lines the patch isn't otherwise changing. That makes it harder for
reviewers to see what the patch *is* changing, and it's a useless
activity anyway because the next run of pg_indent will undo such
changes.

* No documentation updates. At the very least, catalogs.sgml has to
be updated: both the pg_attribute and pg_constraint pages probably
have to have something to say about this.

* No regression test updates. There ought to be a few test cases that
demonstrate the new behavior.

regards, tom lane