Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Kevin Grittner's message of jue ago 02 10:48:02 -0400 2012: Kevin Grittner kevin.gritt...@wicourts.gov wrote: Don't forget the peculiarities of columns with record types. I forgot to include the type creation in the example: test=# create type a as (a1 int, a2 int); CREATE TYPE Thanks for the example. After playing with this, I think that a NOT NULL constraint attached to a column with a composite type is equivalent to a CHECK (col IS DISTINCT FROM NULL); at least they seem to behave identically. Is that what you would expect? This seems a bit complicated to handle with the way I'm doing things today; at parse analysis time, when my current code is creating the check constraint, we don't know anything about the type of the column IIRC. Maybe I will have to delay creating the constraint until execution. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Alvaro Herrera alvhe...@2ndquadrant.com wrote: I think that a NOT NULL constraint attached to a column with a composite type is equivalent to a CHECK (col IS DISTINCT FROM NULL); at least they seem to behave identically. Is that what you would expect? I had not thought about that, but now that you point it out I think that interpretation makes more sense than any other. In a quick test they behaved identically for me. This seems a bit complicated to handle with the way I'm doing things today; at parse analysis time, when my current code is creating the check constraint, we don't know anything about the type of the column IIRC. Maybe I will have to delay creating the constraint until execution. Why? CHECK (col IS DISTINCT FROM NULL) works correctly for *any* type, doesn't it? -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL 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 alvhe...@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. I have added a URL of this thread to the TODO list. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Alvaro Herrera alvhe...@commandprompt.com wrote: Just over a year ago, I posted a patch (based on a previous patch by Bernd Helmle) that attempted to add pg_constraint rows for NOT NULL constraints. http://archives.postgresql.org/message-id/20110707213401.ga27...@alvh.no-ip.org That patch was rather long and complex, as it tried to handle all the hairy issues directly with a completely new 'contype' value for NOT NULL constraints; so the code had to deal with inheritance of constraints, pg_dump issues, and a lot of nitty-gritty. In the end it was killed by a simple realization of Peter Eisentraut's: Why not just transform these into the equivalent CHECK constraints instead? That ended up being discussing at length, and this patch, much smaller than the previous one, is an attempt to do things that way. This patch is not final yet, because there are some issues still open; but the interesting stuff already works. Simply declaring a column as NOT NULL creates a CHECK pg_constraint row; similarly, declaring a CHECK (foo IS NOT NULL) constraint sets the pg_attribute.attnotnull flag. If you create a child table, the NOT NULL constraint will be inherited. Don't forget the peculiarities of columns with record types. Semantically, these three things are different: colname rectype not null colname rectype check (colname is not null) colname rectype check (not (colname is null)) test=# create table t (id int primary key, v1 a not null, v2 a check (v2 is not null), v3 a check (not (v3 is null))); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index t_pkey for table t CREATE TABLE test=# insert into t values (1, (1,1), (1,1), (1,1)); INSERT 0 1 test=# insert into t values (2, (1, null), (1, null), (1,1)); ERROR: new row for relation t violates check constraint t_v2_check DETAIL: Failing row contains (2, (1,), (1,), (1,1)). test=# insert into t values (3, (1, null), (1,1), (1, null)); INSERT 0 1 test=# insert into t values (4, (null, null), (1,1), (1, null)); INSERT 0 1 test=# insert into t values (5, (null, null), (1,1), (null, null)); ERROR: new row for relation t violates check constraint t_v3_check DETAIL: Failing row contains (5, (,), (1,1), (,)). -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Don't forget the peculiarities of columns with record types. I forgot to include the type creation in the example: test=# create type a as (a1 int, a2 int); CREATE TYPE -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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:. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 4 August 2011 18:57, Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 6 August 2011 01:01, Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 6 August 2011 08:17, Dean Rasheed dean.a.rash...@gmail.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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On lör, 2011-08-06 at 08:04 +0100, Dean Rasheed wrote: On 4 August 2011 18:57, Peter Eisentraut pete...@gmx.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). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 6 August 2011 11:03, Peter Eisentraut pete...@gmx.net wrote: On lör, 2011-08-06 at 08:04 +0100, Dean Rasheed wrote: On 4 August 2011 18:57, Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Dean Rasheed dean.a.rash...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Peter Eisentraut pete...@gmx.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Tom Lane's message of vie ago 05 21:23:41 -0400 2011: Peter Eisentraut pete...@gmx.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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 3 August 2011 22:26, Alvaro Herrera alvhe...@commandprompt.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 robertmh...@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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 4 August 2011 09:23, Nikhil Sontakke nikkh...@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 3 August 2011 04:40, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Dean Rasheed's message of mié ago 03 03:11:32 -0400 2011: On 3 August 2011 04:40, Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support alter_add-3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Dean Rasheed's message of sáb jul 23 04:37:06 -0400 2011:
On 22 July 2011 22:28, Robert Haas robertmh...@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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support alter_add-2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 30 July 2011 01:23, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of sáb jul 23 07:40:12 -0400 2011: On Sat, Jul 23, 2011 at 4:37 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: That looks wrong to me, because a NOT NULL constraint is a column constraint not a table constraint. The CREATE TABLE syntax explicitly distinguishes these 2 cases, and only allows NOT NULLs in column constraints. So from a consistency point-of-view, I think that ALTER TABLE should follow suit. So the new syntax could be: ALTER TABLE table_name ALTER [COLUMN] col_name ADD column_constraint where column_constraint is the same as in CREATE TABLE (i.e., allowing all the other constraint types at the same time). It looks like that approach would probably lend itself to more code-reusability too, especially once we start adding options to the constraint. So you'd end up with something like this? ALTER TABLE foo ALTER COLUMN bar ADD CONSTRAINT somename NOT NULL That works for me. I think sticking the name of the constraint in there at the end of the line as Alvaro proposed would be terrible for future syntax extensibility - we'll be much less likely to paint ourselves into a corner with something like this. Here's a patch that does things more or less in this way. Note that this is separate from the other patch, so while you can specify a constraint name for the NOT NULL clause, it's not stored anywhere. This is preliminary: there's no docs nor new tests. Here's how it works (you can also throw in PRIMARY KEY into the mix, but not EXCLUSION): alvherre=# create table bar (a int); CREATE TABLE alvherre=# alter table bar alter column a add constraint foo_fk references foo initially deferred deferrable check (a 4) constraint a_uq unique constraint fnn not null; NOTICE: ALTER TABLE / ADD UNIQUE creará el índice implícito «a_uq» para la tabla «bar» ALTER TABLE alvherre=# \d bar Tabla «public.bar» Columna | Tipo | Modificadores -+-+--- a | integer | not null Índices: a_uq UNIQUE CONSTRAINT, btree (a) Restricciones CHECK: bar_a_check CHECK (a 4) Restricciones de llave foránea: foo_fk FOREIGN KEY (a) REFERENCES foo(a) DEFERRABLE INITIALLY DEFERRED The implementation is a bit dirty (at least IMO), but I don't see a way around that, mainly because ALTER TABLE / ALTER COLUMN does not have a proper ColumnDef to stick the Constraint nodes into; so while the other constraints can do fine without that, it isn't very helpful for NOT NULL. So it has to create a phony ColumnDef for transformConstraintItems to use. Looks pretty good to me (not too dirty). I suppose given that the parser transforms AT_ColumnConstraint into one of the existing command subtypes, you could just have gram.y emit an AT_AddConstraint with the ColumnDef attached, to save adding a new subtype, but there's probably not much difference. I think you need to be setting skipValidation in transformAlterTableStmt() if one of the new column constraints is a FK, so that it gets validated. Perhaps transformColumnConstraints() is more descriptive of the new function rather than transformConstraintItems(). Regards, Dean -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Robert Haas's message of sáb jul 23 07:40:12 -0400 2011: On Sat, Jul 23, 2011 at 4:37 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: That looks wrong to me, because a NOT NULL constraint is a column constraint not a table constraint. The CREATE TABLE syntax explicitly distinguishes these 2 cases, and only allows NOT NULLs in column constraints. So from a consistency point-of-view, I think that ALTER TABLE should follow suit. So the new syntax could be: ALTER TABLE table_name ALTER [COLUMN] col_name ADD column_constraint where column_constraint is the same as in CREATE TABLE (i.e., allowing all the other constraint types at the same time). It looks like that approach would probably lend itself to more code-reusability too, especially once we start adding options to the constraint. So you'd end up with something like this? ALTER TABLE foo ALTER COLUMN bar ADD CONSTRAINT somename NOT NULL That works for me. I think sticking the name of the constraint in there at the end of the line as Alvaro proposed would be terrible for future syntax extensibility - we'll be much less likely to paint ourselves into a corner with something like this. Here's a patch that does things more or less in this way. Note that this is separate from the other patch, so while you can specify a constraint name for the NOT NULL clause, it's not stored anywhere. This is preliminary: there's no docs nor new tests. Here's how it works (you can also throw in PRIMARY KEY into the mix, but not EXCLUSION): alvherre=# create table bar (a int); CREATE TABLE alvherre=# alter table bar alter column a add constraint foo_fk references foo initially deferred deferrable check (a 4) constraint a_uq unique constraint fnn not null; NOTICE: ALTER TABLE / ADD UNIQUE creará el índice implícito «a_uq» para la tabla «bar» ALTER TABLE alvherre=# \d bar Tabla «public.bar» Columna | Tipo | Modificadores -+-+--- a | integer | not null Índices: a_uq UNIQUE CONSTRAINT, btree (a) Restricciones CHECK: bar_a_check CHECK (a 4) Restricciones de llave foránea: foo_fk FOREIGN KEY (a) REFERENCES foo(a) DEFERRABLE INITIALLY DEFERRED The implementation is a bit dirty (at least IMO), but I don't see a way around that, mainly because ALTER TABLE / ALTER COLUMN does not have a proper ColumnDef to stick the Constraint nodes into; so while the other constraints can do fine without that, it isn't very helpful for NOT NULL. So it has to create a phony ColumnDef for transformConstraintItems to use. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support alter_add.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 22 July 2011 22:28, Robert Haas robertmh...@gmail.com wrote: On Fri, Jul 22, 2011 at 4:39 PM, Alvaro Herrera alvhe...@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 alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On Sat, Jul 23, 2011 at 4:37 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: That looks wrong to me, because a NOT NULL constraint is a column constraint not a table constraint. The CREATE TABLE syntax explicitly distinguishes these 2 cases, and only allows NOT NULLs in column constraints. So from a consistency point-of-view, I think that ALTER TABLE should follow suit. So the new syntax could be: ALTER TABLE table_name ALTER [COLUMN] col_name ADD column_constraint where column_constraint is the same as in CREATE TABLE (i.e., allowing all the other constraint types at the same time). It looks like that approach would probably lend itself to more code-reusability too, especially once we start adding options to the constraint. So you'd end up with something like this? ALTER TABLE foo ALTER COLUMN bar ADD CONSTRAINT somename NOT NULL That works for me. I think sticking the name of the constraint in there at the end of the line as Alvaro proposed would be terrible for future syntax extensibility - we'll be much less likely to paint ourselves into a corner with something like this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On Thu, Jul 21, 2011 at 7:51 PM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On Fri, Jul 22, 2011 at 4:39 PM, Alvaro Herrera alvhe...@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 alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On 7 July 2011 22:34, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
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 alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On Thu, Jul 7, 2011 at 5:34 PM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
