Re: cataloguing NOT NULL constraints

On 30 July 2011 01:23, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of sáb jul 23 07:40:12 -0400 2011:
>> On Sat, Jul 23, 2011 at 4:37 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> > That looks wrong to me, because a NOT NULL constraint is a column
>> > constraint not a table constraint. The CREATE TABLE syntax explicitly
>> > distinguishes these 2 cases, and only allows NOT NULLs in column
>> > constraints. So from a consistency point-of-view, I think that ALTER
>> > TABLE should follow suit.
>> >
>> > So the new syntax could be:
>> >
>> > ALTER TABLE table_name ALTER [COLUMN] col_name ADD column_constraint
>> >
>> > where column_constraint is the same as in CREATE TABLE (i.e., allowing
>> > all the other constraint types at the same time).
>> >
>> > It looks like that approach would probably lend itself to more
>> > code-reusability too, especially once we start adding options to the
>> > constraint.
>>
>> So you'd end up with something like this?
>>
>> ALTER TABLE foo ALTER COLUMN bar ADD CONSTRAINT somename NOT NULL
>>
>> That works for me.  I think sticking the name of the constraint in
>> there at the end of the line as Alvaro proposed would be terrible for
>> future syntax extensibility - we'll be much less likely to paint
>> ourselves into a corner with something like this.
>
> Here's a patch that does things more or less in this way.  Note that
> this is separate from the other patch, so while you can specify a
> constraint name for the NOT NULL clause, it's not stored anywhere.
>
> This is preliminary: there's no docs nor new tests.  Here's how it works
> (you can also throw in PRIMARY KEY into the mix, but not EXCLUSION):
>
> alvherre=# create table bar (a int);
> CREATE TABLE
> alvherre=# alter table bar alter column a add constraint foo_fk references foo initially deferred deferrable check (a <> 4) constraint a_uq unique constraint fnn not null;
> NOTICE:  ALTER TABLE / ADD UNIQUE creará el índice implícito «a_uq» para la tabla «bar»
> ALTER TABLE
> alvherre=# \d bar
>        Tabla «public.bar»
>  Columna |  Tipo   | Modificadores
> ---------+---------+---------------
>  a       | integer | not null
> Índices:
>    "a_uq" UNIQUE CONSTRAINT, btree (a)
> Restricciones CHECK:
>    "bar_a_check" CHECK (a <> 4)
> Restricciones de llave foránea:
>    "foo_fk" FOREIGN KEY (a) REFERENCES foo(a) DEFERRABLE INITIALLY DEFERRED
>
>
> The implementation is a bit dirty (at least IMO), but I don't see a way
> around that, mainly because ALTER TABLE / ALTER COLUMN does not have a
> proper ColumnDef to stick the Constraint nodes into; so while the other
> constraints can do fine without that, it isn't very helpful for NOT NULL.
> So it has to create a phony ColumnDef for transformConstraintItems to use.
>

Looks pretty good to me (not too dirty). I suppose given that the
parser transforms AT_ColumnConstraint into one of the existing command
subtypes, you could just have gram.y emit an AT_AddConstraint with the
ColumnDef attached, to save adding a new subtype, but there's probably
not much difference.

I think you need to be setting skipValidation in
transformAlterTableStmt() if one of the new column constraints is a
FK, so that it gets validated.

Perhaps transformColumnConstraints() is more descriptive of the new
function rather than transformConstraintItems().

Regards,
Dean

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>