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

--On 15. Oktober 2010 13:36:38 -0400 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> 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.

Thanks for looking at this. I didn't reckon that i really got everything
done (admitted, docs and regression tests were liberally left out), and
given your comments your review is invaluable at this point.

> 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.
>

Agreed, there's much more cleanup needed.

> 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.

Hmm i seem to remember that i had some problems during coding and some
testing, where changes were unvisible during recursion....I will go through
them again.

>
> * 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)
>

Ok

> * 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.
>

Oh, i really missed that.

> * 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.
>

I will look at it.

> * 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.
>

Ok

> * 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.
>

whoops...i've set (setq-default show-trailing-whitespace t) in my .emacs,
so i don't miss it again.

> * 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.
>

I'll include them. It was important for me to get a feeling about wether
the direction in refactoring/extending this code is the correct one or
needs more thinking. So, thanks again for reviewing.

--
Thanks

Bernd