On 30 July 2011 01:23, Alvaro Herrera <alvhe...@commandprompt.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.a.rash...@gmail.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 <alvhe...@commandprompt.com> > The PostgreSQL Company - Command Prompt, Inc. > PostgreSQL Replication, Consulting, Custom Development, 24x7 support > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers