Re: cataloguing NOT NULL constraints

On 7 July 2011 22:34, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Hi,
>
> The attached patch introduces pg_constraint rows for NOT NULL
> column constraints.
>
> This patch is a heavily reworked version of Bernd Helmle's patch here:
> http://archives.postgresql.org/message-id/E57A252DFD60C1FCA91731BD@amenophis
>
> I fixed a bunch of issues with that patch.  I think the most
> visible one is that NOT NULL constraints are completely separate beasts
> from PRIMARY KEYS.  This has a number of consequences, including
> different inheritance properties (NOT NULL constraints inherit, while
> primary keys do not; and ALTER TABLE commands propagate to children
> correctly and consistently for both of them); if you drop one of them
> but the other remains, the attnotnull marking is not removed from
> pg_attribute; if you drop both or if there's only one of them and you
> drop it, attnotnull is reset.
>
> Note: if we want to assume the position that PRIMARY KEYS ought to
> inherit (that is to say, the NOT NULL bit of them should), we'd need to
> discuss that.  Not inheriting is a bit of a change in behavior, but
> inheriting seems counterintuitive to some.
>
> One bit that was present in Bernd's patch and I left as is is support
> for domain NOT NULL constraints.  I didn't check this part very much
> (which is to say "at all"), so please bear with me if it doesn't work,
> bugs are probably mine because of the way I ripped out the underlying
> implementation.
>
> I had to invent a new AlterTable command type, because the code to add a
> new primary key was injecting a SET NOT NULL command; this didn't work
> very well because it automatically created a NOT NULL pg_constraint row,
> which obviously isn't what we wanted.  So that's AT_SetAttNotNull for
> you, which only updates attnotnull without creating a pg_constraint
> entry.
>
> (Another thing I changed is that the code no longer uses
> CookedConstraint for not null constraints.  I invented a new struct to
> carry them over.  The reason was mostly because MergeAttributes needed
> to record attname in some cases and attnum in others, and the Cooked
> struct wasn't letting me; instead of changing it, which wasn't a very
> good fit for other reasons anyway, it seemed better to invent a new
> one more suited to the task.)
>
> Also, there was some attempt to share code to handle DROP NOT NULL with
> DROP CONSTRAINT (as discussed in the original thread, see message with
> Id D1815DA0B6288201EC1CFAC3(at)amenophis).  I ripped that out because it
> was rather messy, and since NOT NULL seems to have some particular
> requirements anyway, it seems better to have them separate.  It's
> cleaner and easier to understand anyway.  If someone wants to have a go
> at merging things, be my guest ...
>
> Note that I'm not adding support for ALTER TABLE / DROP CONSTRAINT to
> drop a not null constraint.  We can do that of course, I don't think
> it's all that hard -- but this patch is large enough already and we can
> add that separately.  (There's no way to display the NOT NULL constraint
> names anyway, though they are preserved on inheritance, per request in
> the original threads).
>
> One thing I intend to change before committing is moving all the code I
> added to pg_constraint.c back into tablecmds.c (and removing the
> accompanying constraint.h header).  I thought at one point that this was
> better modularity (code that touches pg_constraint should be in the
> eponymous file), but seeing how constraints are thoroughly messed up
> with in tablecmds.c this seems an exercise in futility.  Another change
> is eyeballing the new tests a couple more times to ensure
>
> I haven't looked at pg_dump at all, either.
>

Nice work!

It appears to work as expected and I find this behaviour much more intuitive.

I think that there probably ought to be a way to display the NOT NULL
constraint names (perhaps through \d+). For example, if you're
planning to support NOT VALID on top of this in the future, then there
needs to be a way to get the constraint's name to validate it.

You said you haven't looked at pg_dump yet. I'm guessing that the only
change needed is to dump the NOT NULL constraint name along with the
table definition so that it is restored correctly.

I had one thought regarding the code: perhaps the NOT NULL constraints
could be represented using a new struct hanging off the TupleConstr
struct of a TupleDesc, in the same way as CHECK constraints. Then they
could be read in by the relcache code at the same time as CHECK
constraints, rather than by GetRelationNotNullConstraints(), and they
would be more accessible throughout the backend code. For example,
supporting NOT VALID on top of this would require a similar change to
the constraint exclusion code as for CHECK constraints, so the planner
would need to access the constraint details.

Regards,
Dean