cataloguing NOT NULL constraints

On Thu, Jul 7, 2011 at 5:34 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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

It would probably be good to add this to the next CommitFest. Not
sure about anyone else, but I'm too busy looking at patches that were
submitted in April, May, and June to look at any new ones right now.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On tor, 2011-07-07 at 17:34 -0400, Alvaro Herrera wrote:

The attached patch introduces pg_constraint rows for NOT NULL
column constraints.

The information schema views check_constraints and table_constraints
currently make up some artificial constraint names for not-null
constraints. I suppose this patch removes the underlying cause for
that, so could you look into updating the information schema as well?
You could probably just remove the separate union branches for not null
and adjust the contype conditions.

Excerpts from Robert Haas's message of vie jul 08 23:30:10 -0400 2011:

On Thu, Jul 7, 2011 at 5:34 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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

It would probably be good to add this to the next CommitFest. Not
sure about anyone else, but I'm too busy looking at patches that were
submitted in April, May, and June to look at any new ones right now.

Yeah, that's what I did.

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

On 7 July 2011 22:34, Alvaro Herrera <alvherre@commandprompt.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@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

Excerpts from Peter Eisentraut's message of sáb jul 09 14:45:23 -0400 2011:

On tor, 2011-07-07 at 17:34 -0400, Alvaro Herrera wrote:

The attached patch introduces pg_constraint rows for NOT NULL
column constraints.

The information schema views check_constraints and table_constraints
currently make up some artificial constraint names for not-null
constraints. I suppose this patch removes the underlying cause for
that, so could you look into updating the information schema as well?
You could probably just remove the separate union branches for not null
and adjust the contype conditions.

Fixing table_constraints is pretty trivial, just like you suggest;
already done in my private tree.

I checked the check_constraints definition in the standard and it's not
clear to me that NOT NULL constraints are supposed to be there at all.
Are NOT NULL constraints considered to be CHECK constraints too?

The fix is trivial either way: if they are not to be there we should
just remove the UNION arm that deals with them. If they are, we do
likewise and then fix the other arm as you suggest.

Thanks for the pointer.

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

Hello,

Here's an updated version of this patch. Changes from the previous
version:

* Some more cleanup of the original patch. In particular, domain
constraints have been given a look over. They seem to work fine now.

* information_schema has been updated to display the stored names
instead of inventing fake names. (This probably differs from the
previous behavior in the handling of primary keys).

* pg_dump is supposed to work now.

* The code I said I was going to move from pg_constraint.c to
tablecmds.c has been moved.

Some things I haven't addressed:

Excerpts from Dean Rasheed's message of mié jul 13 04:18:00 -0400 2011:

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.

Absolutely true. Another thing that needs to be done here is to let the
ALTER TABLE and ALTER DOMAIN commands use the constraint names; right
now, they simply let you add the constraint but not specify the name.
That should probably be revisited.

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.

Yeah, something like that. Domain NOT NULL constraints required a bit
more than that, because the original code assumed that only check
constraints were possible. Actually figuring out how to make that
simple thing work, however, took a lot more effort than it sounds
because pg_dump is so intrincate.

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.

Interesting idea. I haven't touched this.

I'm pretty happy with the current status of this patch and barring
further reviewer comments, I would commit this as is and work on the
improvements proposed by Dean as separate patches.

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

On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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.

Absolutely true.  Another thing that needs to be done here is to let the
ALTER TABLE and ALTER DOMAIN commands use the constraint names; right
now, they simply let you add the constraint but not specify the name.
That should probably be revisited.

That, at least, seems like something that should be fixed before commit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On ons, 2011-07-20 at 13:53 -0400, Alvaro Herrera wrote:

I checked the check_constraints definition in the standard and it's
not clear to me that NOT NULL constraints are supposed to be there at
all. Are NOT NULL constraints considered to be CHECK constraints too?

Yes, NOT NULL is considered just a notational shorthand for CHECK (x IS
NULL).

Excerpts from Robert Haas's message of vie jul 22 12:14:30 -0400 2011:

On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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.

Absolutely true.  Another thing that needs to be done here is to let the
ALTER TABLE and ALTER DOMAIN commands use the constraint names; right
now, they simply let you add the constraint but not specify the name.
That should probably be revisited.

That, at least, seems like something that should be fixed before commit.

Hmm, which point, Dean's or mine? Dean was saying that the name should
be displayed by some flavor of \d; mine was that we need a command such
as

ALTER TABLE foo ALTER COLUMN bar SET NOT NULL name_of_notnull_constr

where the last bit is what's new.

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

On Fri, Jul 22, 2011 at 4:39 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of vie jul 22 12:14:30 -0400 2011:

On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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.

Absolutely true.  Another thing that needs to be done here is to let the
ALTER TABLE and ALTER DOMAIN commands use the constraint names; right
now, they simply let you add the constraint but not specify the name.
That should probably be revisited.

That, at least, seems like something that should be fixed before commit.

Hmm, which point, Dean's or mine?  Dean was saying that the name should
be displayed by some flavor of \d;

That might not be 100% necessary for the initial commit, but seems
easy to fix, so why not?

mine was that we need a command such
as

ALTER TABLE foo ALTER COLUMN bar SET NOT NULL name_of_notnull_constr

where the last bit is what's new.

Well, if you don't have that, I don't see how you have any chance of
pg_dump working correctly. Though I think it should use the table
constraint syntax:

CONSTRAINT name_of_notnull_constr constraint_definition

I'm not exactly sure what to propose for the constraint_definition.
Perhaps just:

CONSTRAINT name_of_notnull_constr NOT NULL column_name

Though Peter seemed to think it should be:

CONSTRAINT name_of_notnull_constr CHECK (column_name IS NOT NULL)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 22 July 2011 22:28, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jul 22, 2011 at 4:39 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of vie jul 22 12:14:30 -0400 2011:

On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

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.

Absolutely true.  Another thing that needs to be done here is to let the
ALTER TABLE and ALTER DOMAIN commands use the constraint names; right
now, they simply let you add the constraint but not specify the name.
That should probably be revisited.

That, at least, seems like something that should be fixed before commit.

Hmm, which point, Dean's or mine?  Dean was saying that the name should
be displayed by some flavor of \d;

That might not be 100% necessary for the initial commit, but seems
easy to fix, so why not?

Agreed.

mine was that we need a command such
as

ALTER TABLE foo ALTER COLUMN bar SET NOT NULL name_of_notnull_constr

where the last bit is what's new.

Well, if you don't have that, I don't see how you have any chance of
pg_dump working correctly.

Ah yes, pg_dump's dumpConstraint() needs a clause to alter a table
adding a named NOT NULL constraint (and the DOMAIN case should be
preserving the constraint's name too). So it looks like some new
syntax for ALTER TABLE to add named NOT NULL constraints is probably
needed before this can be committed.

 Though I think it should use the table
constraint syntax:

CONSTRAINT name_of_notnull_constr constraint_definition

I'm not exactly sure what to propose for the constraint_definition.
Perhaps just:

CONSTRAINT name_of_notnull_constr NOT NULL column_name

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.

Regards,
Dean

On Sat, Jul 23, 2011 at 4:37 AM, Dean Rasheed <dean.a.rasheed@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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

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

On 30 July 2011 01:23, Alvaro Herrera <alvherre@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.rasheed@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

Excerpts from Dean Rasheed's message of sáb jul 30 18:40:46 -0400 2011:

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.

Thanks. I've done the other changes you suggested, but I don't see that
it's desirable to have gram.y emit AT_AddConstraint directly. It seems
cleaner to be able to turn a NOT NULL constraint into AT_SetNotNull
in parse_utilcmd instead. (Maybe I'll have to bite the bullet and make
AT_AddConstraint work for not null constraints as well, as part of the
larger patch. Not sure.) Currently, the table constraint syntax only
lets you do a single constraint at a time, but you can do multiple
constraints with the column constraint syntax. I am not sure how hard
it is to rework the grammar so that only a single constraint is
allowed, but I'm not sure that it's worth the trouble either.

Attached is an updated version, touching the docs and adding a new
simple regression test.

But ... I just noticed that I need to touch ALTER DOMAIN in a similar
way as well.

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

On 3 August 2011 04:40, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Excerpts from Dean Rasheed's message of sáb jul 30 18:40:46 -0400 2011:

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.

Thanks.  I've done the other changes you suggested, but I don't see that
it's desirable to have gram.y emit AT_AddConstraint directly.  It seems
cleaner to be able to turn a NOT NULL constraint into AT_SetNotNull
in parse_utilcmd instead.

I wasn't proposing getting rid of that bit in parse_utilcmd. I just
meant move the block that calls transformColumnConstraints() to the
AT_AddConstraint case in transformAlterTableStmt(). So it would test
if it has a ColumnDef attached, and either process a table constraint
or a set of column constraints. That would avoid the need for a new
command subtype, and so the changes to tablecmds.c would not be
needed. I think it would also avoid the need to add colname to the
Constraint struct, so none of the parsenodes.h changes would be needed
either.

 (Maybe I'll have to bite the bullet and make

AT_AddConstraint work for not null constraints as well, as part of the
larger patch.  Not sure.)  Currently, the table constraint syntax only
lets you do a single constraint at a time, but you can do multiple
constraints with the column constraint syntax.  I am not sure how hard
it is to rework the grammar so that only a single constraint is
allowed, but I'm not sure that it's worth the trouble either.

Attached is an updated version, touching the docs and adding a new
simple regression test.

But ... I just noticed that I need to touch ALTER DOMAIN in a similar
way as well.

Oh I keep forgetting about domains. But after a quick glance at the
docs, doesn't it already have syntax for this?

Regards,
Dean

Excerpts from Dean Rasheed's message of mié ago 03 03:11:32 -0400 2011:

On 3 August 2011 04:40, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Excerpts from Dean Rasheed's message of sáb jul 30 18:40:46 -0400 2011:

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.

Thanks.  I've done the other changes you suggested, but I don't see that
it's desirable to have gram.y emit AT_AddConstraint directly.  It seems
cleaner to be able to turn a NOT NULL constraint into AT_SetNotNull
in parse_utilcmd instead.

I wasn't proposing getting rid of that bit in parse_utilcmd. I just
meant move the block that calls transformColumnConstraints() to the
AT_AddConstraint case in transformAlterTableStmt(). So it would test
if it has a ColumnDef attached, and either process a table constraint
or a set of column constraints. That would avoid the need for a new
command subtype, and so the changes to tablecmds.c would not be
needed. I think it would also avoid the need to add colname to the
Constraint struct, so none of the parsenodes.h changes would be needed
either.

Oh, I see. Well, the problem is precisely that we don't have a
ColumnDef at that point.

... ah, maybe what we could do is have gram.y create a ColumnDef in the
new production, and stick that in cmd->def instead of the list of
constraints. So parse_utilcmd would have to know that if that node
IsA(ColumnDef) then it needs to deal with column constraints. It seems
a bit cleaner overall, though it's still wart-ish.

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

Excerpts from Alvaro Herrera's message of mié ago 03 13:12:38 -0400 2011:

... ah, maybe what we could do is have gram.y create a ColumnDef in the
new production, and stick that in cmd->def instead of the list of
constraints. So parse_utilcmd would have to know that if that node
IsA(ColumnDef) then it needs to deal with column constraints. It seems
a bit cleaner overall, though it's still wart-ish.

Yes, this works, thanks for the suggestion. Attached.

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

Excerpts from Dean Rasheed's message of sáb jul 23 04:37:06 -0400 2011:

On 22 July 2011 22:28, Robert Haas <robertmhaas@gmail.com> wrote:

mine was that we need a command such as

ALTER TABLE foo ALTER COLUMN bar SET NOT NULL name_of_notnull_constr

where the last bit is what's new.

Well, if you don't have that, I don't see how you have any chance of
pg_dump working correctly.

Ah yes, pg_dump's dumpConstraint() needs a clause to alter a table
adding a named NOT NULL constraint (and the DOMAIN case should be
preserving the constraint's name too). So it looks like some new
syntax for ALTER TABLE to add named NOT NULL constraints is probably
needed before this can be committed.

So after writing the code to handle named NOT NULL constraints for
tables, I'm thinking that dumpConstraints needs to be fixed thusly:

@@ -12888,6 +12968,27 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
                         NULL, NULL);
        }
    }
+   else if (coninfo->contype == 'n' && tbinfo)
+   {
+       /* NOT NULL constraint on a table */
+       if (coninfo->separate)
+       {
+           write_msg(NULL, "NOT NULL constraints cannot be dumped separately from their owning table\n");
+           exit_nicely();
+       }
+   }
+   else if (coninfo->contype == 'n' && tbinfo == NULL)
+   {
+       /* NOT NULL constraint on a domain */
+       TypeInfo   *tyinfo = coninfo->condomain;
+
+       /* Ignore if not to be dumped separately */
+       if (coninfo->separate)
+       {
+           write_msg(NULL, "NOT NULL constraints cannot be dumped separately from their owning domain\n");
+           exit_nicely();
+       }
+   }
    else
    {
        write_msg(NULL, "unrecognized constraint type: %c\n", coninfo->contype);

There doesn't seem to be any point in giving pg_dump the ability to dump
such constraints separately; I don't think there's any situation in
which dependencies between the table/domain and NOT NULL constraints
would get circular (which is what causes them to acquire the "separate"
flag). I don't want to write code that is going to be always
unused, particularly not in a beast as hairy such as pg_dump.

Note that the code dumps not null constraints along with the table
definition, which includes the constraint name, so it works perfectly
fine already.

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