Re: cataloguing NOT NULL constraints

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.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Thu, Jul 7, 2011 at 5:34 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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(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.

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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)gmail(dot)com> wrote:
> On Fri, Jul 22, 2011 at 4:39 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(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.

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

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

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
>

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 3 August 2011 04:40, Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)gmail(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 3 August 2011 22:26, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> 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(at)gmail(dot)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.
>

Wow. Yes I think you're right - it is a hairy beast :-)

It was the code immediately above that for CHECK constraints that made
me assume NOT NULLs would need similar logic. But I haven't quite
figured out how a CHECK constraint's dependencies could form a loop,
so I don't know if it could happen for a NOT NULL.

Regards,
Dean

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

Some nit-picking.

AFAICS above, we seem to be only using 'tbinfo' to identify the object
type here - 'table' visavis 'domain'. We could probably reduce the
above two elses to a single one and use the check of tbinfo being not
null to decide which object type name to spit out..

Although, it's difficult to see how we could end up marking NOT NULL
constraints as 'separate' ever. So this code will be rarely exercised,
if ever IMO.

Regards,
Nikhils

On 4 August 2011 09:23, Nikhil Sontakke <nikkhils(at)gmail(dot)com> wrote:
>> 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);
>>
>
> Some nit-picking.
>
> AFAICS above, we seem to be only using 'tbinfo' to identify the object
> type here - 'table' visavis 'domain'. We could probably reduce the
> above two elses to a single one and use the check of tbinfo being not
> null to decide which object type name to spit out..
>
> Although, it's difficult to see how we could end up marking NOT NULL
> constraints as 'separate' ever. So this code will be rarely exercised,
> if ever IMO.
>

There's a related issue that might affect how this code ends up. I'm
not sure if this has been discussed before, but it seems to be a
problem for CHECK constraints currently, and will affect NOT NULL in
the same way - if the constraint is NOT VALID, and some of the
existing data violates the constraint, then pg_dump needs to dump the
constraint separately, after the table's data, otherwise the restore
will fail.

So it looks like this code will have to support dumping NOT NULLs
ultimately anyway.

BTW, this happens automatically for FK constraints, so I don't think
this is a problem for 9.1.

Regards,
Dean

Excerpts from Nikhil Sontakke's message of jue ago 04 04:23:59 -0400 2011:

> Some nit-picking.
>
> AFAICS above, we seem to be only using 'tbinfo' to identify the object
> type here - 'table' visavis 'domain'. We could probably reduce the
> above two elses to a single one and use the check of tbinfo being not
> null to decide which object type name to spit out..

Yeah, I considered that, but I rejected the idea on the grounds that all
the preceding blocks use this style. (Also, if I understand you well,
what you suggest would incur into a translatability problem; we'd have
to create two separate messages for that purpose anyway.)

> Although, it's difficult to see how we could end up marking NOT NULL
> constraints as 'separate' ever. So this code will be rarely exercised,
> if ever IMO.

Well, as Dean points out, as soon as we have NOT VALID constraints it
will be necessary. I prefer to leave that out for a later patch.

Thanks for looking.

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

On tis, 2011-08-02 at 23:40 -0400, Alvaro Herrera wrote:
> 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.

Have you considered just cataloging NOT NULL constraints as CHECK
constraints and teaching the reverse parser to convert "x CHECK (x IS
NOT NULL)" to "x NOT NULL". It seems to me that we're adding a whole
lot of hoopla here that is essentially identical to the existing CHECK
constraint support (it must be, per SQL standard), for no additional
functionality.

Excerpts from Peter Eisentraut's message of jue ago 04 13:57:54 -0400 2011:
> On tis, 2011-08-02 at 23:40 -0400, Alvaro Herrera wrote:
> > 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.

> Have you considered just cataloging NOT NULL constraints as CHECK
> constraints and teaching the reverse parser to convert "x CHECK (x IS
> NOT NULL)" to "x NOT NULL".

Hmm, no, I admit I haven't. The current approach was suggested very
early in the history of this patch. (To be honest I didn't know NOT
NULL constraints where special forms of CHECK constraints until you
mentioned the other day regarding the information schema, and then it
didn't occur to me that it might make sense to represent them as such in
the catalog).

> It seems to me that we're adding a whole
> lot of hoopla here that is essentially identical to the existing CHECK
> constraint support (it must be, per SQL standard), for no additional
> functionality.

Yeah, perhaps you're right. The main reason they were considered
separately is that we wanted to have them to be optimized via
pg_attribute.attnotnull, but my patch does away with the need for that
because it is maintained separately anyway.

Before embarking on rewriting this patch from scratch, I would like to
know what's your opinion (or the SQL standard's) on the fact that this
patch separated the PRIMARY KEY from NOT NULL constraints, so that they
don't act exactly alike (to wit, the not-nullness of a PK does not
inherit while the one from a NOT NULL constraint does).

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

On tor, 2011-08-04 at 16:15 -0400, Alvaro Herrera wrote:
> > Have you considered just cataloging NOT NULL constraints as CHECK
> > constraints and teaching the reverse parser to convert "x CHECK (x IS
> > NOT NULL)" to "x NOT NULL".
>
> Hmm, no, I admit I haven't. The current approach was suggested very
> early in the history of this patch.

Well, the early patch thought this was a small problem. Now we know
it's a big problem, so it might be better to solve it terms of another
big problem that is already solved. :-)

> > It seems to me that we're adding a whole
> > lot of hoopla here that is essentially identical to the existing CHECK
> > constraint support (it must be, per SQL standard), for no additional
> > functionality.
>
> Yeah, perhaps you're right. The main reason they were considered
> separately is that we wanted to have them to be optimized via
> pg_attribute.attnotnull, but my patch does away with the need for that
> because it is maintained separately anyway.

Hmm, OK, but in any case you could have kept attnotnull and treated it
as a kind of optimization that indicates whether you can derive
not-nullability from existing CHECK constraints (which you can easily do
in enough cases).

> Before embarking on rewriting this patch from scratch, I would like to
> know what's your opinion (or the SQL standard's) on the fact that this
> patch separated the PRIMARY KEY from NOT NULL constraints, so that they
> don't act exactly alike (to wit, the not-nullness of a PK does not
> inherit while the one from a NOT NULL constraint does).

The SQL standard deals with inheritance in terms of composite types,
which don't have constraints, so that doesn't give any guidance.

That said, I think the substitutability property of object-oriented
systems, namely that you can use a child object in place of a parent
object, requires in principle that we inherit all constraints (by
default, at least). We don't inherit primary keys because of
implementation issues with indexes, but at some point in the future we
should fix that. So to some degree, inheriting the not-null property of
primary keys while not inheriting the rest of it is a bit wrong, but it
would appear to be a step in the right direction, and changing
established behavior seems a bit gratuitous to me.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tor, 2011-08-04 at 16:15 -0400, Alvaro Herrera wrote:
>> Yeah, perhaps you're right. The main reason they were considered
>> separately is that we wanted to have them to be optimized via
>> pg_attribute.attnotnull, but my patch does away with the need for that
>> because it is maintained separately anyway.

> Hmm, OK, but in any case you could have kept attnotnull and treated it
> as a kind of optimization that indicates whether you can derive
> not-nullability from existing CHECK constraints (which you can easily do
> in enough cases).

Yes. I thought that was how we were going to do it, and I'm rather
distressed to hear of attnotnull going away. Even if there were not a
performance reason to keep it (and I'll bet there is), you can be sure
that removing that column will break a lot of client-side code. See
recent complaints about Robert removing relistemp, which has only been
around for a release or two. attnotnull goes back to the beginning,
more or less.

regards, tom lane

Excerpts from Tom Lane's message of vie ago 05 21:23:41 -0400 2011:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On tor, 2011-08-04 at 16:15 -0400, Alvaro Herrera wrote:
> >> Yeah, perhaps you're right. The main reason they were considered
> >> separately is that we wanted to have them to be optimized via
> >> pg_attribute.attnotnull, but my patch does away with the need for that
> >> because it is maintained separately anyway.
>
> > Hmm, OK, but in any case you could have kept attnotnull and treated it
> > as a kind of optimization that indicates whether you can derive
> > not-nullability from existing CHECK constraints (which you can easily do
> > in enough cases).
>
> Yes. I thought that was how we were going to do it, and I'm rather
> distressed to hear of attnotnull going away. Even if there were not a
> performance reason to keep it (and I'll bet there is), you can be sure
> that removing that column will break a lot of client-side code. See
> recent complaints about Robert removing relistemp, which has only been
> around for a release or two. attnotnull goes back to the beginning,
> more or less.

Err, obviously I didn't express myself very well. I am not removing the
column. What I tried to say is that we no longer need to optimize the
representation of NOT NULL as separate entities from CHECK constraints,
because attnotnull is maintained separately from the pg_constraint
entries. In other words, from that point of view, representing NOT NULL
as CHECK is not a problem from a performance POV, because it is already
taken care of by letting attnotnull continue to represent them as a
cache.

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

On 4 August 2011 18:57, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Have you considered just cataloging NOT NULL constraints as CHECK
> constraints and teaching the reverse parser to convert "x CHECK (x IS
> NOT NULL)" to "x NOT NULL".  It seems to me that we're adding a whole
> lot of hoopla here that is essentially identical to the existing CHECK
> constraint support (it must be, per SQL standard), for no additional
> functionality.
>

With this approach, if the user explicitly wrote "CHECK (x IS NOT
NULL)" would it end up setting attnotnull? Presumably, since the
pg_constraint entry would be indistinguishable, it would be difficult
to do otherwise. From a performance standpoint that might be a good
thing, but it's going to be bad for compatibility.

What if they wrote "CHECK (NOT x IS NULL)", or "CHECK (x IS DISTINCT
FROM NULL)"? How many variants would it reverse parse?

What would this do to error messages when the constraint is violated?

I'm not convinced this simplifies things much; it just moves the
complexity elsewhere, and at the cost of losing compatibility with the
current behaviour.

Regards,
Dean

On 6 August 2011 01:01, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> Before embarking on rewriting this patch from scratch, I would like to
>> know what's your opinion (or the SQL standard's) on the fact that this
>> patch separated the PRIMARY KEY from NOT NULL constraints, so that they
>> don't act exactly alike (to wit, the not-nullness of a PK does not
>> inherit while the one from a NOT NULL constraint does).
>
> The SQL standard deals with inheritance in terms of composite types,
> which don't have constraints, so that doesn't give any guidance.
>
> That said, I think the substitutability property of object-oriented
> systems, namely that you can use a child object in place of a parent
> object, requires in principle that we inherit all constraints (by
> default, at least).  We don't inherit primary keys because of
> implementation issues with indexes, but at some point in the future we
> should fix that.  So to some degree, inheriting the not-null property of
> primary keys while not inheriting the rest of it is a bit wrong, but it
> would appear to be a step in the right direction, and changing
> established behavior seems a bit gratuitous to me.
>

The current behaviour is inconsistent - the not-null property of a PK
is sometimes inherited and sometimes not, depending on whether the PK
is added at table-creation time or later. So a change in either
direction is a change to some current behaviour, unless we leave it
inconsistent, which seems unacceptable.

So I don't think compatibility arguments apply here, and I would tend
to agree that inheriting the not-null property of PKs while not
inheriting the rest seems wrong.

Regards,
Dean

On 6 August 2011 08:17, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> The current behaviour is inconsistent - the not-null property of a PK
> is sometimes inherited and sometimes not, depending on whether the PK
> is added at table-creation time or later.
>

Oops, that should have been "depending on whether the PK is defined
before or after the inheritance is set up".

On lör, 2011-08-06 at 08:04 +0100, Dean Rasheed wrote:
> On 4 August 2011 18:57, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > Have you considered just cataloging NOT NULL constraints as CHECK
> > constraints and teaching the reverse parser to convert "x CHECK (x IS
> > NOT NULL)" to "x NOT NULL". It seems to me that we're adding a whole
> > lot of hoopla here that is essentially identical to the existing CHECK
> > constraint support (it must be, per SQL standard), for no additional
> > functionality.
> >
>
> With this approach, if the user explicitly wrote "CHECK (x IS NOT
> NULL)" would it end up setting attnotnull?

Yes, I would assume so.

> Presumably, since the
> pg_constraint entry would be indistinguishable, it would be difficult
> to do otherwise. From a performance standpoint that might be a good
> thing, but it's going to be bad for compatibility.

What compatibility issue are you concerned about specifically?

> What if they wrote "CHECK (NOT x IS NULL)", or "CHECK (x IS DISTINCT
> FROM NULL)"? How many variants would it reverse parse?

Well, it's already the case that the user can write check constraints in
any number of forms that have the effect of restricting null values; and
attnotnull isn't set in those cases. So in the beginning I'd be quite
happy if we just recognized CHECK (x IS NOT NULL).

Longer term, I think we could tie this in with more general nullability
detection. For example, it is occasionally asked that we can expose
nullability through views or CREATE TABLE AS. The SQL standards has
rules for what cases we should detect (which don't include the two you
give).

> What would this do to error messages when the constraint is violated?

That's a reasonable concern, although not a fatal one, and it can be
solved in any case.

> I'm not convinced this simplifies things much; it just moves the
> complexity elsewhere, and at the cost of losing compatibility with the
> current behaviour.

No, I don't think this would lose compatibility (except perhaps in cases
of error message wording).

On lör, 2011-08-06 at 08:17 +0100, Dean Rasheed wrote:
> The current behaviour is inconsistent - the not-null property of a PK
> is sometimes inherited and sometimes not, depending on whether the PK
> is added at table-creation time or later. So a change in either
> direction is a change to some current behaviour, unless we leave it
> inconsistent, which seems unacceptable.

Yeah, OK, that's wrong then.

On 6 August 2011 11:03, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On lör, 2011-08-06 at 08:04 +0100, Dean Rasheed wrote:
>> On 4 August 2011 18:57, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> > Have you considered just cataloging NOT NULL constraints as CHECK
>> > constraints and teaching the reverse parser to convert "x CHECK (x IS
>> > NOT NULL)" to "x NOT NULL".  It seems to me that we're adding a whole
>> > lot of hoopla here that is essentially identical to the existing CHECK
>> > constraint support (it must be, per SQL standard), for no additional
>> > functionality.
>> >
>>
>> With this approach, if the user explicitly wrote "CHECK (x IS NOT
>> NULL)" would it end up setting attnotnull?
>
> Yes, I would assume so.
>
>> Presumably, since the
>> pg_constraint entry would be indistinguishable, it would be difficult
>> to do otherwise. From a performance standpoint that might be a good
>> thing, but it's going to be bad for compatibility.
>
> What compatibility issue are you concerned about specifically?
>
>> What if they wrote "CHECK (NOT x IS NULL)", or "CHECK (x IS DISTINCT
>> FROM NULL)"? How many variants would it reverse parse?
>
> Well, it's already the case that the user can write check constraints in
> any number of forms that have the effect of restricting null values; and
> attnotnull isn't set in those cases.  So in the beginning I'd be quite
> happy if we just recognized CHECK (x IS NOT NULL).
>
> Longer term, I think we could tie this in with more general nullability
> detection.  For example, it is occasionally asked that we can expose
> nullability through views or CREATE TABLE AS.  The SQL standards has
> rules for what cases we should detect (which don't include the two you
> give).
>
>> What would this do to error messages when the constraint is violated?
>
> That's a reasonable concern, although not a fatal one, and it can be
> solved in any case.
>
>> I'm not convinced this simplifies things much; it just moves the
>> complexity elsewhere, and at the cost of losing compatibility with the
>> current behaviour.
>
> No, I don't think this would lose compatibility (except perhaps in cases
> of error message wording).
>

Hmm, maybe my compatibility concerns are not so serious. I'm still
trying to work out exactly what user-visible changes this approach
would lead to. Suppose you had:

CREATE TABLE foo
(
a int NOT NULL,
b int CHECK (b IS NOT NULL),
c int CHECK (NOT c IS NULL)
);

Right now \d gives:

Table "public.foo"
Column | Type | Modifiers
--------+---------+-----------
a | integer | not null
b | integer |
c | integer |
Check constraints:
"foo_b_check" CHECK (b IS NOT NULL)
"foo_c_check" CHECK (NOT c IS NULL)

With this approach, one change would be that you'd gain an extra "not
null" in the Modifiers column for "b".

But how many CHECK constraints would show? I guess it would show 3,
although it could be changed to just show 1. But it certainly couldn't
continue to show 2, since nothing in the catalogs could distinguish
the constraints on "a" from those on "b".

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> [ wonders what psql's \d will do with NOT NULL constraints ]

I think this might be taking the notion of "it should be backwards
compatible" too far. We're not expecting this patch to not change
the wording of error messages, for instance (in fact, I think a large
part of the point is to add constraint names to not-null errors).
Why would we expect it to not change what \d shows?

regards, tom lane

On lör, 2011-08-06 at 12:58 +0100, Dean Rasheed wrote:
> Right now \d gives:
>
> Table "public.foo"
> Column | Type | Modifiers
> --------+---------+-----------
> a | integer | not null
> b | integer |
> c | integer |
> Check constraints:
> "foo_b_check" CHECK (b IS NOT NULL)
> "foo_c_check" CHECK (NOT c IS NULL)
>
> With this approach, one change would be that you'd gain an extra "not
> null" in the Modifiers column for "b".
>
> But how many CHECK constraints would show? I guess it would show 3,
> although it could be changed to just show 1. But it certainly couldn't
> continue to show 2, since nothing in the catalogs could distinguish
> the constraints on "a" from those on "b".

I'd say we would show all (what is currently known as) NOT NULL
constraints under "Check constraints:".

On Fri, Jul 22, 2011 at 12:14:30PM -0400, Robert Haas wrote:
> On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)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.

I have added a URL of this thread to the TODO list.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +