Re: [HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011: On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: I would summarise the consistency requirements as: 1). ADD CONSTRAINT should leave both parent and child tables in the same state as they would have been if the constraint had been defined at table creation time. 2). DROP CONSTRAINT should leave both parent and child tables in the same state as if the constraint had never existed (completely reversing the effects of ADD CONSTRAINT). I don't have a strong opinion as to whether or not the NOT NULL part of a PK should be inherited, provided that it is consistent with the above. I guess that if I were forced to choose, I would say that the NOT NULL part of a PK should not be inherited, since I do think of it as part of the PK, and PKs are not inherited. OK, I see your point, and I agree with you. Interesting. This whole thing requires quite a bit of rejiggering in the initial transformation phase, I think, but yeah, I see the points here and I will see to them. Does this mean that NOT NULL PRIMARY KEY now behaves differently? I think it does , because if you drop the PK then the field needs to continue being not null. And here I was thinking that this was just a quick job to enable NOT VALID constraints ... -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011: On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: I would summarise the consistency requirements as: 1). ADD CONSTRAINT should leave both parent and child tables in the same state as they would have been if the constraint had been defined at table creation time. 2). DROP CONSTRAINT should leave both parent and child tables in the same state as if the constraint had never existed (completely reversing the effects of ADD CONSTRAINT). I don't have a strong opinion as to whether or not the NOT NULL part of a PK should be inherited, provided that it is consistent with the above. I guess that if I were forced to choose, I would say that the NOT NULL part of a PK should not be inherited, since I do think of it as part of the PK, and PKs are not inherited. OK, I see your point, and I agree with you. Interesting. This whole thing requires quite a bit of rejiggering in the initial transformation phase, I think, but yeah, I see the points here and I will see to them. Does this mean that NOT NULL PRIMARY KEY now behaves differently? I think it does , because if you drop the PK then the field needs to continue being not null. Yeah, I think an implicit not-null because you made it a primary key is now different from one that you write out. And here I was thinking that this was just a quick job to enable NOT VALID constraints ... Bwahaha. -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011:
On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:
Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011:
Interesting. This whole thing requires quite a bit of rejiggering in
the initial transformation phase, I think, but yeah, I see the points
here and I will see to them. Does this mean that NOT NULL PRIMARY KEY
now behaves differently? I think it does , because if you drop the PK
then the field needs to continue being not null.
Yeah, I think an implicit not-null because you made it a primary key
is now different from one that you write out.
Actually, it wasn't that hard, but I'm not really sure I like the
resulting code:
/*
* We want to inherit NOT NULL constraints, but not primary
keys.
* Since attnotnull flags in pg_attribute stores both, we want
to keep only
* the attnotnull flag from those columns that have it from NOT
NULL
* constraints. To do this, we create a copy of the table's
descriptor
* and scribble on it by resetting all the attnotnull bits to
false, and
* the setting them true for columns that appear in a NOT NULL
constraint.
*
* Note: we cannot use CreateTupleDescCopy here, because it'd
lose
* the atthasdef bits, as well as constraints.
*/
tupleDesc =
CreateTupleDescCopyConstr(RelationGetDescr(relation));
constr = tupleDesc-constr;
parent_nns = GetRelationNotNullConstraints(relation);
for (parent_attno = 1; parent_attno = tupleDesc-natts;
parent_attno++)
tupleDesc-attrs[parent_attno - 1]-attnotnull = false;
foreach (cell, parent_nns)
{
NotNullConstraint *constr = lfirst(cell);
tupleDesc-attrs[constr-attnum - 1]-attnotnull = true;
}
Here's the simple example (sorry for the spanish):
alvherre=# create table foo (a int primary key, b int not null);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para
la tabla «foo»
CREATE TABLE
alvherre=# create table bar () inherits (foo);
CREATE TABLE
alvherre=# \d foo
Tabla «public.foo»
Columna | Tipo | Modificadores
-+-+---
a | integer | not null
b | integer | not null
Índices:
foo_pkey PRIMARY KEY, btree (a)
Número de tablas hijas: 1 (Use \d+ para listarlas.)
alvherre=# \d bar
Tabla «public.bar»
Columna | Tipo | Modificadores
-+-+---
a | integer |
b | integer | not null
Hereda: foo
alvherre=# create table baz (a int not null primary key);
NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «baz_pkey» para
la tabla «baz»
CREATE TABLE
alvherre=# create table qux () inherits (baz);
CREATE TABLE
alvherre=# \d baz
Tabla «public.baz»
Columna | Tipo | Modificadores
-+-+---
a | integer | not null
Índices:
baz_pkey PRIMARY KEY, btree (a)
Número de tablas hijas: 1 (Use \d+ para listarlas.)
alvherre=# \d qux
Tabla «public.qux»
Columna | Tipo | Modificadores
-+-+---
a | integer | not null
Hereda: baz
--
Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011: On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011: Interesting. This whole thing requires quite a bit of rejiggering in the initial transformation phase, I think, but yeah, I see the points here and I will see to them. Does this mean that NOT NULL PRIMARY KEY now behaves differently? I think it does , because if you drop the PK then the field needs to continue being not null. Yeah, I think an implicit not-null because you made it a primary key is now different from one that you write out. Actually, it wasn't that hard, but I'm not really sure I like the resulting code: What don't you like about it? My concern is that I'm not sure it's correct... -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Robert Haas's message of mié jun 29 18:16:20 -0400 2011: On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié jun 29 13:07:25 -0400 2011: On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of lun jun 27 10:35:59 -0400 2011: Interesting. This whole thing requires quite a bit of rejiggering in the initial transformation phase, I think, but yeah, I see the points here and I will see to them. Does this mean that NOT NULL PRIMARY KEY now behaves differently? I think it does , because if you drop the PK then the field needs to continue being not null. Yeah, I think an implicit not-null because you made it a primary key is now different from one that you write out. Actually, it wasn't that hard, but I'm not really sure I like the resulting code: What don't you like about it? Scribbling on attnotnull like that seems ... kludgy (we have to walk the attr list three times: first to copy, second to reset all the attnotnull flags, third to set those of interest). The fact that we need a copy to scribble on, seems wrong as well (we weren't creating a copy before). The existing mechanisms to copy tupledescs aren't very flexible, but improving that seems overengineering to me. My concern is that I'm not sure it's correct... Ah, well, I don't see any reason not to trust it currently. I am afraid it could easily break in the future though. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On 27 June 2011 03:31, Robert Haas robertmh...@gmail.com wrote: On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: Really? I would expect the reverse, namely that the not-nullness is part of the PK constraint and dropping the PK *would* then start allowing NULLs. Hmm, OK. I had assumed we were only trying to fix the problem that parent and child inheritance tables could get out of step, but maybe you're right. If we go with that approach, then consider: CREATE TABLE foo (a int); CREATE TABLE bar () INHERITS (foo); Now if someone adds a primary key foo (a), what happens currently is that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should that remain true (on the theory that a primary key constraint is not inherited) or become false (on the theory that parent and child tables should match)? I'm not sure, but my real problem with the current behaviour is its inconsistency. Consider this case: CREATE TABLE foo (a int PRIMARY KEY); CREATE TABLE bar () INHERITS (foo); Currently this results in bar not allowing NULLs, which is inconsistent with adding the PK after defining the inheritance. Then if the PK is dropped, the non-nullness is left behind on both foo and bar. I would summarise the consistency requirements as: 1). ADD CONSTRAINT should leave both parent and child tables in the same state as they would have been if the constraint had been defined at table creation time. 2). DROP CONSTRAINT should leave both parent and child tables in the same state as if the constraint had never existed (completely reversing the effects of ADD CONSTRAINT). I don't have a strong opinion as to whether or not the NOT NULL part of a PK should be inherited, provided that it is consistent with the above. I guess that if I were forced to choose, I would say that the NOT NULL part of a PK should not be inherited, since I do think of it as part of the PK, and PKs are not inherited. But I wouldn't be too upset if it were inherited (consistently!) and I can't think of a use case where that would be a problem. 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: On 27 June 2011 03:31, Robert Haas robertmh...@gmail.com wrote: On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: Really? I would expect the reverse, namely that the not-nullness is part of the PK constraint and dropping the PK *would* then start allowing NULLs. Hmm, OK. I had assumed we were only trying to fix the problem that parent and child inheritance tables could get out of step, but maybe you're right. If we go with that approach, then consider: CREATE TABLE foo (a int); CREATE TABLE bar () INHERITS (foo); Now if someone adds a primary key foo (a), what happens currently is that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should that remain true (on the theory that a primary key constraint is not inherited) or become false (on the theory that parent and child tables should match)? I'm not sure, but my real problem with the current behaviour is its inconsistency. Consider this case: CREATE TABLE foo (a int PRIMARY KEY); CREATE TABLE bar () INHERITS (foo); Currently this results in bar not allowing NULLs, which is inconsistent with adding the PK after defining the inheritance. Then if the PK is dropped, the non-nullness is left behind on both foo and bar. I would summarise the consistency requirements as: 1). ADD CONSTRAINT should leave both parent and child tables in the same state as they would have been if the constraint had been defined at table creation time. 2). DROP CONSTRAINT should leave both parent and child tables in the same state as if the constraint had never existed (completely reversing the effects of ADD CONSTRAINT). I don't have a strong opinion as to whether or not the NOT NULL part of a PK should be inherited, provided that it is consistent with the above. I guess that if I were forced to choose, I would say that the NOT NULL part of a PK should not be inherited, since I do think of it as part of the PK, and PKs are not inherited. OK, I see your point, and I agree with you. -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: Really? I would expect the reverse, namely that the not-nullness is part of the PK constraint and dropping the PK *would* then start allowing NULLs. Hmm, OK. I had assumed we were only trying to fix the problem that parent and child inheritance tables could get out of step, but maybe you're right. If we go with that approach, then consider: CREATE TABLE foo (a int); CREATE TABLE bar () INHERITS (foo); Now if someone adds a primary key foo (a), what happens currently is that foo.a becomes NOT NULL, but bar.a still allows NULLs. Should that remain true (on the theory that a primary key constraint is not inherited) or become false (on the theory that parent and child tables should match)? -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On 25 June 2011 01:59, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of vie jun 24 19:01:49 -0400 2011: I would tend to think of the not-null-ness that is required by the primary constraint as a separate constraint, not an intrinsic part of the primary key. IOW, if you drop the primary key constraint, IMV, that should never cause the column to begin allowing nulls. Really? I would expect the reverse, namely that the not-nullness is part of the PK constraint and dropping the PK *would* then start allowing NULLs. I thought that this was one of the reasons for doing this work in the first place. See http://wiki.postgresql.org/wiki/Todo#CREATE and the bug reports linked from there. Yeah, that is actually what happens. (I had never noticed this, but it seems obvious in hindsight.) alvherre=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para la tabla «foo» CREATE TABLE alvherre=# alter table foo drop constraint foo_pkey; ALTER TABLE alvherre=# \d foo Tabla «public.foo» Columna | Tipo | Modificadores -+-+--- a | integer | not null Yeah, that's one of the bugs linked from the TODO item, that I hoped this patch would fix. [http://archives.postgresql.org/pgsql-hackers/2009-04/msg00062.php] What this says is that this patch needs to be creating pg_constraint entries for all PRIMARY KEY columns too, which answers my question above quite nicely. If by that you mean that you'd end up with 2 pg_constraint entries for the PK, then that seems wrong to me. I think the not-nullness should be part of the PK. 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Another thing I just noticed is that if you pg_upgrade from a previous release, all the NOT NULL pg_constraint rows are going to be missing. I'm not sure what's the best way to deal with this -- should we just provide a script for the user to run on each database? -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Another thing I just noticed is that if you pg_upgrade from a previous release, all the NOT NULL pg_constraint rows are going to be missing. Uh, really? pg_upgrade uses SQL commands to recreate the contents of the system catalogs, so if these entries aren't getting created automatically, that sounds like a bug in the patch... -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Robert Haas's message of vie jun 24 12:06:17 -0400 2011: On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Another thing I just noticed is that if you pg_upgrade from a previous release, all the NOT NULL pg_constraint rows are going to be missing. Uh, really? pg_upgrade uses SQL commands to recreate the contents of the system catalogs, so if these entries aren't getting created automatically, that sounds like a bug in the patch... Ah -- we should be fine then. I haven't tested that yet (actually I haven't finished tinkering with the code). -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 24. Juni 2011 12:06:17 -0400 Robert Haas robertmh...@gmail.com wrote: Uh, really? pg_upgrade uses SQL commands to recreate the contents of the system catalogs, so if these entries aren't getting created automatically, that sounds like a bug in the patch... AFAIR, i've tested it and it worked as expected. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Bernd Helmle's message of vie jun 24 12:56:26 -0400 2011: --On 24. Juni 2011 12:06:17 -0400 Robert Haas robertmh...@gmail.com wrote: Uh, really? pg_upgrade uses SQL commands to recreate the contents of the system catalogs, so if these entries aren't getting created automatically, that sounds like a bug in the patch... AFAIR, i've tested it and it worked as expected. Okay, thanks for the confirmation. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
So remind me ... did we discuss PRIMARY KEY constraints? Are they supposed to show up as inherited not null rows in the child? Obviously, they do not show up as PKs in the child, but they *are* not null so my guess is that they need to be inherited as not null as well. (Right now, unpatched head of course emits the column as attnotnull). In this case, the inherited name (assuming that the child declaration does not explicitely state a constraint name) should be the same as the PK, correct? It is unclear to me that primary keys shouldn't be inherited by default. But I guess that's a separate discussion. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: So remind me ... did we discuss PRIMARY KEY constraints? Are they supposed to show up as inherited not null rows in the child? Obviously, they do not show up as PKs in the child, but they *are* not null so my guess is that they need to be inherited as not null as well. (Right now, unpatched head of course emits the column as attnotnull). In this case, the inherited name (assuming that the child declaration does not explicitely state a constraint name) should be the same as the PK, correct? I would tend to think of the not-null-ness that is required by the primary constraint as a separate constraint, not an intrinsic part of the primary key. IOW, if you drop the primary key constraint, IMV, that should never cause the column to begin allowing nulls. -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Robert Haas's message of vie jun 24 19:01:49 -0400 2011: On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: So remind me ... did we discuss PRIMARY KEY constraints? Are they supposed to show up as inherited not null rows in the child? Obviously, they do not show up as PKs in the child, but they *are* not null so my guess is that they need to be inherited as not null as well. (Right now, unpatched head of course emits the column as attnotnull). In this case, the inherited name (assuming that the child declaration does not explicitely state a constraint name) should be the same as the PK, correct? I would tend to think of the not-null-ness that is required by the primary constraint as a separate constraint, not an intrinsic part of the primary key. IOW, if you drop the primary key constraint, IMV, that should never cause the column to begin allowing nulls. Yeah, that is actually what happens. (I had never noticed this, but it seems obvious in hindsight.) alvherre=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY creará el índice implícito «foo_pkey» para la tabla «foo» CREATE TABLE alvherre=# alter table foo drop constraint foo_pkey; ALTER TABLE alvherre=# \d foo Tabla «public.foo» Columna | Tipo | Modificadores -+-+--- a | integer | not null What this says is that this patch needs to be creating pg_constraint entries for all PRIMARY KEY columns too, which answers my question above quite nicely. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Thanks, I am looking at the new version from Bernd's git repo. One problem I noticed is that it doesn't really work correctly for all callers of heap_create_with_catalog -- you're only passing the cooked not null constraints in DefineRelation, but there are some other places that call heap_create_with_catalog too. In particular, bootstrap mode is not handled; I haven't checked the other callers yet. I'm looking for the best way to handle that. So, question: do we need pg_constraint rows to exist for all NOT NULL constraints, including those in system catalogs, and including those in bootstrap catalogs? If we're going to require that, we're going to need to add a few initial data lines to the pg_constraint catalog definition, plus some code to handle the other bootstrap cases (non bootstrap relations). We could also declare that we don't need pg_constraint rows for NOT NULL constraints in system catalogs; but if we're going to do that, I guess we'd better disallow tables from inheriting system catalogs. Right now it's not disallowed but I guess it's pretty useless alvherre=# create table bar() inherits (pg_class); CREATE TABLE ... on the other hand, being able to use a catalog in a LIKE column definition sounds like it could be useful: alvherre=# create table qux (now timestamp, like pg_class); CREATE TABLE alvherre=# \d qux Tabla «public.qux» Columna |Tipo | Modificadores +-+--- now| timestamp without time zone | relname| name| not null relnamespace | oid | not null reltype| oid | not null -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Alvaro Herrera alvhe...@commandprompt.com writes: So, question: do we need pg_constraint rows to exist for all NOT NULL constraints, including those in system catalogs, and including those in bootstrap catalogs? If we're going to require that, we're going to need to add a few initial data lines to the pg_constraint catalog definition, plus some code to handle the other bootstrap cases (non bootstrap relations). Installing such rows during bootstrap would be problematic, because what do you do for catalogs that are created before pg_constraint? Possible solution is to leave bootstrap's behavior alone, and have a step during initdb's post-bootstrap stuff that creates a matching pg_constraint row for every pg_attribute entry that's marked attnotnull. We could also declare that we don't need pg_constraint rows for NOT NULL constraints in system catalogs; but if we're going to do that, I guess we'd better disallow tables from inheriting system catalogs. I have a feeling that omitting these entries for system catalogs would bite us in other ways down the road, even if inheritance were the only soft spot right now. 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Thu, Jun 16, 2011 at 1:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: Possible solution is to leave bootstrap's behavior alone, and have a step during initdb's post-bootstrap stuff that creates a matching pg_constraint row for every pg_attribute entry that's marked attnotnull. That seems like a pretty good solution. I have a feeling that omitting these entries for system catalogs would bite us in other ways down the road, even if inheritance were the only soft spot right now. I share that feeling. -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 16. Juni 2011 13:25:05 -0400 Tom Lane t...@sss.pgh.pa.us wrote: Possible solution is to leave bootstrap's behavior alone, and have a step during initdb's post-bootstrap stuff that creates a matching pg_constraint row for every pg_attribute entry that's marked attnotnull. +1 for this idea. I never came to an end about this because i didn't have any clue how to do it efficiently. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Bernd Helmle's message of jue jun 16 14:30:48 -0400 2011: --On 16. Juni 2011 13:25:05 -0400 Tom Lane t...@sss.pgh.pa.us wrote: Possible solution is to leave bootstrap's behavior alone, and have a step during initdb's post-bootstrap stuff that creates a matching pg_constraint row for every pg_attribute entry that's marked attnotnull. +1 for this idea. I never came to an end about this because i didn't have any clue how to do it efficiently. Okay, I have done it this way -- adding one more fixup function to initdb is very easy. I only wish that the ending \n in the query to initdb would be optional -- it took me a while to realize that if you omit it, the query doesn't get run at all. Oh well. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
I intend to have a look at this patch and hopefully fix the outstanding issues. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Bernd, are you still working on this? On fre, 2010-10-15 at 13:36 -0400, Tom Lane wrote: Bernd Helmle maili...@oopsware.de writes: Here is an updated version of the patch. It fixes the following issues Andrew discovered during his review cycle: I looked through this a bit. It's not ready to commit unfortunately. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Trailing Whitespace Tips (was: [HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch)
On fre, 2010-10-15 at 22:45 +0200, Dimitri Fontaine wrote: I suppose people using other editors or tools will come up with other tricks and tips. Here is an alternative recipe that I have been using: (require 'show-wspace) (add-hook 'font-lock-mode-hook 'show-ws-highlight-hard-spaces) (add-hook 'font-lock-mode-hook 'show-ws-highlight-tabs) (add-hook 'font-lock-mode-hook 'show-ws-highlight-trailing-whitespace) Maybe it should go in src/tools/editors/emacs.samples, too? Yeah, I think we should recommend some way to highlight faulty whitespace. The problem is, after you turn it on, it will make you cry as you realize how sloppy most code and other files are written. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Trailing Whitespace Tips (was: [HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch)
--On 16. Oktober 2010 12:35:06 +0300 Peter Eisentraut pete...@gmx.net wrote: Maybe it should go in src/tools/editors/emacs.samples, too? Yeah, I think we should recommend some way to highlight faulty whitespace. The problem is, after you turn it on, it will make you cry as you realize how sloppy most code and other files are written. That's exactly why it is mostly off in my case. But you always can put it in a special editing mode, which i currently experimenting with. Thanks for your tips. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed dean.a.rash...@gmail.com wrote: OK, here it is. I've not cured this compiler warning (in fact I'm not sure what it's complaining about, because the variable *is* used): tablecmds.c: In function 'ATExecSetNotNull': tablecmds.c:4747: warning: unused variable 'is_new_constraint' Just a left over from earlier coding, i have removed this in my patch version. I have adapted your fixes and would like to propose that we are proceeding with my version, if that's okay for you? -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On 15 October 2010 08:32, Bernd Helmle maili...@oopsware.de wrote: --On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed dean.a.rash...@gmail.com wrote: OK, here it is. I've not cured this compiler warning (in fact I'm not sure what it's complaining about, because the variable *is* used): tablecmds.c: In function 'ATExecSetNotNull': tablecmds.c:4747: warning: unused variable 'is_new_constraint' Just a left over from earlier coding, i have removed this in my patch version. I have adapted your fixes and would like to propose that we are proceeding with my version, if that's okay for you? Ah, I see it (I was looking at the wrong copy of that variable). Yes, let's proceed with your version. If these were the only problems with this patch, given that they required only trivial changes to initialise variables to NIL/false, perhaps it was premature to mark it as returned with feedback. Thoughts? 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: If these were the only problems with this patch, given that they required only trivial changes to initialise variables to NIL/false, perhaps it was premature to mark it as returned with feedback. Sure. Is someone available to do a detailed code review? Alvaro, were you planning to pick this one up? -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
The new patch applies cleanly to head, there are no compile errors and all the make check tests pass (linux). Thanks Andrew On Thu, Oct 14, 2010 at 4:35 PM, Bernd Helmle maili...@oopsware.de wrote: --On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera alvhe...@commandprompt.com wrote: Looking in that function, there is a similar found variable that isn't being initialised (which my compiler didn't warn about). Initialising that to false, sems to fix the problem and all the regression tests then pass. Excellent. Please send an updated patch. Here is an updated version of the patch. It fixes the following issues Andrew discovered during his review cycle: * Fix compiler warnings and crash due to uninitialized variables (pretty much the fix Dean proposed) * Remove accidentally added pg_latch.c in my own git repos. I will do further cycles over Andrew's review report. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Robert Haas robertmh...@gmail.com writes: On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: If these were the only problems with this patch, given that they required only trivial changes to initialise variables to NIL/false, perhaps it was premature to mark it as returned with feedback. Sure. Is someone available to do a detailed code review? Alvaro, were you planning to pick this one up? I had been planning to take this one once it got past initial review, since it was mostly my wish to start with. 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Bernd Helmle maili...@oopsware.de writes: Here is an updated version of the patch. It fixes the following issues Andrew discovered during his review cycle: I looked through this a bit. It's not ready to commit unfortunately. The main gripe I've got is that you've paid very little attention to updating comments that your code changes invalidate. That's not even a little bit acceptable: people will still have to read this code later. Two examples are that struct CookedConstraint is now used for notnull constraints in addition to its other duties, but you didn't adjust the comments in its declaration; and that you made transformColumnDefinition put NOTNULL constraints into the ckconstraints list, ignoring the fact that both its name and the comments say that that's only CHECK constraints. In the latter case it'd probably be a better idea to add a separate nnconstraints list to CreateStmtContext. Some other random points: * The ALTER TABLE changes seem to be inserting a whole lot of CommandCounterIncrement calls in places where there were none before. Do we really need those? Usually the theory is that one at the end of an operation is sufficient. * All those bits with deconstruct_array could usefully be folded into a subroutine, along the lines of bool constraint_is_for_single_column(HeapTuple constraintTup, int attno) * As best I can tell from looking, the patch *always* generates a name for NOT NULL constraints. It should honor the user's name for the constraint if one is given, ie create table foo (f1 int constraint nn1 not null); Historically we've had to drop such names on the floor, but that should stop. * pg_dump almost certainly needs some updates. I imagine the behavior we'll want from it is to print NOT NULL only when the column's notnull constraint shows that it's locally defined. If it gets printed for columns that only have an inherited NOT NULL, then dump and reload will change the not-null inheritance state. This may be a bit tricky since pg_dump also has to still work against older servers, but with any luck you can steal its logic for inherited check constraints. We probably want it to start preserving the names of notnull constraints, too. * In general you should probably search for all places that reference pg_constraint.contype, as a means of spotting any other code that needs to be updated for this. * Lots of bogus trailing whitespace. git diff --check can help you with that. Also, please try to avoid unnecessary changes of whitespace on lines the patch isn't otherwise changing. That makes it harder for reviewers to see what the patch *is* changing, and it's a useless activity anyway because the next run of pg_indent will undo such changes. * No documentation updates. At the very least, catalogs.sgml has to be updated: both the pg_attribute and pg_constraint pages probably have to have something to say about this. * No regression test updates. There ought to be a few test cases that demonstrate the new behavior. 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
Trailing Whitespace Tips (was: [HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch)
Tom Lane t...@sss.pgh.pa.us writes: * Lots of bogus trailing whitespace. git diff --check can help you with that. This is a repetitive common remark that I think sharing tips to avoid that problem is a good idea. Here's an emacs setup to have trailing whitespace hurt the eyes, and more-than-80 columns lines too. This one is more controversial as we find lots of long lines in the PostgreSQL sources. Still: ;; display only tails of lines longer than 80 columns, and ;; trailing whitespaces (require 'whitespace) (setq whitespace-line-column 80 whitespace-style '(face trailing lines-tail empty)) ;; face for tabs long lines' tails (set-face-attribute 'whitespace-tab nil :background red1 :foreground yellow :weight 'bold) (set-face-attribute 'whitespace-line nil :background red1 :foreground yellow :weight 'bold) ;; activate minor whitespace mode when in some coding modes (add-hook 'emacs-lisp-mode-hook 'whitespace-mode) (add-hook 'python-mode-hook 'whitespace-mode) (add-hook 'c-mode-hook 'whitespace-mode) Now, it's easy to find some more about it, including images of how it looks. You can't miss trailing whitespace any more : http://ruslanspivak.com/2010/09/27/keep-track-of-whitespaces-and-column-80-overflow/ http://panela.blog-city.com/python_and_emacs_4_whitespace_tabs_tabwidth_visualizi.htm http://www.emacswiki.org/emacs/WhiteSpace I suppose people using other editors or tools will come up with other tricks and tips. Maybe it should go in src/tools/editors/emacs.samples, too? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 15. Oktober 2010 13:36:38 -0400 Tom Lane t...@sss.pgh.pa.us wrote: Bernd Helmle maili...@oopsware.de writes: Here is an updated version of the patch. It fixes the following issues Andrew discovered during his review cycle: I looked through this a bit. It's not ready to commit unfortunately. Thanks for looking at this. I didn't reckon that i really got everything done (admitted, docs and regression tests were liberally left out), and given your comments your review is invaluable at this point. The main gripe I've got is that you've paid very little attention to updating comments that your code changes invalidate. That's not even a little bit acceptable: people will still have to read this code later. Two examples are that struct CookedConstraint is now used for notnull constraints in addition to its other duties, but you didn't adjust the comments in its declaration; and that you made transformColumnDefinition put NOTNULL constraints into the ckconstraints list, ignoring the fact that both its name and the comments say that that's only CHECK constraints. In the latter case it'd probably be a better idea to add a separate nnconstraints list to CreateStmtContext. Agreed, there's much more cleanup needed. Some other random points: * The ALTER TABLE changes seem to be inserting a whole lot of CommandCounterIncrement calls in places where there were none before. Do we really need those? Usually the theory is that one at the end of an operation is sufficient. Hmm i seem to remember that i had some problems during coding and some testing, where changes were unvisible during recursionI will go through them again. * All those bits with deconstruct_array could usefully be folded into a subroutine, along the lines of bool constraint_is_for_single_column(HeapTuple constraintTup, int attno) Ok * As best I can tell from looking, the patch *always* generates a name for NOT NULL constraints. It should honor the user's name for the constraint if one is given, ie create table foo (f1 int constraint nn1 not null); Historically we've had to drop such names on the floor, but that should stop. Oh, i really missed that. * pg_dump almost certainly needs some updates. I imagine the behavior we'll want from it is to print NOT NULL only when the column's notnull constraint shows that it's locally defined. If it gets printed for columns that only have an inherited NOT NULL, then dump and reload will change the not-null inheritance state. This may be a bit tricky since pg_dump also has to still work against older servers, but with any luck you can steal its logic for inherited check constraints. We probably want it to start preserving the names of notnull constraints, too. I will look at it. * In general you should probably search for all places that reference pg_constraint.contype, as a means of spotting any other code that needs to be updated for this. Ok * Lots of bogus trailing whitespace. git diff --check can help you with that. Also, please try to avoid unnecessary changes of whitespace on lines the patch isn't otherwise changing. That makes it harder for reviewers to see what the patch *is* changing, and it's a useless activity anyway because the next run of pg_indent will undo such changes. whoops...i've set (setq-default show-trailing-whitespace t) in my .emacs, so i don't miss it again. * No documentation updates. At the very least, catalogs.sgml has to be updated: both the pg_attribute and pg_constraint pages probably have to have something to say about this. * No regression test updates. There ought to be a few test cases that demonstrate the new behavior. I'll include them. It was important for me to get a feeling about wether the direction in refactoring/extending this code is the correct one or needs more thinking. So, thanks again for reviewing. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle maili...@oopsware.de wrote: --On 30. September 2010 10:12:48 +0200 Bernd Helmle maili...@oopsware.de wrote: Yeah, there where some changes in the meantime to the master which generate some merge failures...will provide a new version along with other fixes soon Here's a new patch that addresses all DDL commands around NOT NULL constraints and maintain and follow inheritance information correctly now (but it lags documentation updates). I hope i haven't introduced nasty bugs and broke something badly, some deeper testing is welcome. This appears to be waiting on further review from Andrew Geery. Andrew, will you be posting a new review soon? -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
I'll post it sometime tomorrow... Thanks Andrew On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle maili...@oopsware.de wrote: --On 30. September 2010 10:12:48 +0200 Bernd Helmle maili...@oopsware.de wrote: Yeah, there where some changes in the meantime to the master which generate some merge failures...will provide a new version along with other fixes soon Here's a new patch that addresses all DDL commands around NOT NULL constraints and maintain and follow inheritance information correctly now (but it lags documentation updates). I hope i haven't introduced nasty bugs and broke something badly, some deeper testing is welcome. This appears to be waiting on further review from Andrew Geery. Andrew, will you be posting a new review soon? -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Below is my review of the latest iteration of the Extend NOT NULL Representation to pg_constraint patch found here: http://archives.postgresql.org/message-id/e57a252dfd60c1fca9173...@amenophis Thanks Andrew Basic questions Is the patch in context diff format? Yes Does it apply cleanly to the current git master? Yes patching file src/backend/catalog/heap.c patching file src/backend/commands/tablecmds.c patching file src/backend/parser/parse_utilcmd.c patching file src/backend/port/pg_latch.c patching file src/backend/utils/adt/ruleutils.c Hunk #1 succeeded at 1076 (offset 5 lines). patching file src/include/catalog/heap.h patching file src/include/catalog/pg_constraint.h patching file src/include/nodes/parsenodes.h patching file src/test/regress/expected/alter_table.out patching file src/test/regress/expected/cluster.out However, one of the modified files in the patch is /src/backend/port/pg_latch.c. There are no functional changes in this file, but it does add a line to the top of the file that breaks the build: diff --git a/src/backend/port/pg_latch.c b/src/backend/port/pg_latch.c index ...002f2f4 . *** a/src/backend/port/pg_latch.c --- b/src/backend/port/pg_latch.c *** *** 0 --- 1 + ../../../src/backend/port/unix_latch.c \ No newline at end of file Removing the line allows the build to complete successfully. Overview == The impetus for this patch is to prevent a child table from dropping an inherited not null constraint. Not only does dropping an inherited not null constraint violate the spirit of table inheritance, but it also breaks pg_dump (the dropped constraint on the child table is not in the dump, so any null values in the child data will be disallowed). To fix this problem, the patch adds a new constraint type for not null constraints in the pg_constraint catalog, while continuing to maintain the attnotnull info in pg_attribute. Problem == In 9.0 and before, not null constraints are tracked in the pg_attribute.attnotnull. The problem is that there is nothing in this catalog that indicates whether the not null constraint is inherited. However, the pg_constraint catalog does have columns for tracking whether a constraint is local to the relation or inherited (conislocal and coninhcount), so it makes sense to add a new constraint type (contype=’n’) for not null constraints which enables the db to disallow dropping inherited not null constraints. Not null constraints are given the name (conname) table_name_column_name_not_null. (Note that this also opens up the possibility (if, for example, the alter table syntax was changed) for giving the not null constraint an arbitrary name.) Here’s a simple example of the problem: create table foo_parent ( id int not null ); create table foo_child () inherits ( foo_parent ); alter table foo_child alter column id drop not null; insert into foo_child values ( null ); In 9.0 and before, the db cannot detect that the “alter table ... alter column ... drop not null” should not be allowed because there is no information in the pg_attribute catalog to specify that the relation is inherited. In this example, with the patch, the pg_constraint catalog has two additional rows, foo_parent_id_not_null (conislocal=t, coninhcount=0) and foo_child_id_not_null (conislocal=f, coninhcount=1) and the db can now detect that the “alter table ... alter column ... drop not null” statement should be disallowed because the not null constraint on foo_child is inherited. The db reports the following error for this statement: cannot drop inherited NOT NULL constraint foo_child_id_not_null, relation foo_child [perhaps to make this more consistent with the error message produced when trying to drop, for example, an inherited check constraint, change the comma to the word “of”] Basic tests I performed the following basic SQL tests with the patch: * create table with a column with a not null constraint -- check that the not null constraint is recorded in the pg_constraint table * create table with no not null column constraint; alter table to add it -- check that the column not null constraint is recorded in the pg_constraint table * create parent with a not null column constraint; create child table that inherits from the parent -- check that both have a not null column constraint in pg_constraint and that the child’s not null constraint inherits from the parent’s * create parent table with no not null column constraint; create child table that inherits from the parent; alter the parent table to add a not null column constraint -- check that both the parent and the child have a not null column constraint in pg_constraint * create parent table with no not null column constraint; create child table that inherits from the parent; alter the child table to add a not null column constraint -- check that there is only a not
Re: [HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On Thu, Oct 14, 2010 at 10:02 AM, Andrew Geery andrew.ge...@gmail.com wrote: I didn’t have much time to look at the code. The only thing I’ll mention is that there are a couple of XXX TODO items that should be cleared up. [...] Since this patch actually makes inheritance behave in a more expected way, nothing needs to be changed in the inheritance documentation. However, at the very least, the documentation dealing with the pg_catalog [http://www.postgresql.org/docs/9.0/interactive/catalog-pg-constraint.html] needs to be updated to deal with the new constraint type. Thanks for catching these problems. I did a sanity make clean make make check before applying the patch and all the tests passed. After applying the patch and doing make clean make make check, I got a number of failures of the form “FAILED (test process exited with exit code 2)”. The exact number of failures varies by run, so I’m wondering if I didn’t do something wrong... That indicates that PostgreSQL is crashing. So I think this patch is definitely not ready for prime time yet, and needs some debugging. In view of the fact that we are out of time for this CommitFest, I'm going to mark this Returned with Feedback in the CommitFest application. Hopefully it will be resubmitted for the next CommitFest after further refinement, because I think this is a good and useful improvement. -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On 14 October 2010 16:42, Robert Haas robertmh...@gmail.com wrote: In view of the fact that we are out of time for this CommitFest, ... When is the official end of this commitfest? I remember talk at the start, that the end would be postponed (by a week?) due to time spent on the git migration. Is that still the case? 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 14. Oktober 2010 11:42:27 -0400 Robert Haas robertmh...@gmail.com wrote: I did a sanity make clean make make check before applying the patch and all the tests passed. After applying the patch and doing make clean make make check, I got a number of failures of the form “FAILED (test process exited with exit code 2)”. The exact number of failures varies by run, so I’m wondering if I didn’t do something wrong... That indicates that PostgreSQL is crashing. So I think this patch is definitely not ready for prime time yet, and needs some debugging. In view of the fact that we are out of time for this CommitFest, I'm going to mark this Returned with Feedback in the CommitFest application. Hopefully it will be resubmitted for the next CommitFest after further refinement, because I think this is a good and useful improvement. Yeah, its crashing but it doesn't do it here on my MacBook (passing the regression test is one of my top prio when submitting a patch). Must be some broken pointer or similar oversight which is triggered on Andrew's box. Andrew, which OS and architecture have you tested on? -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 14. Oktober 2010 10:02:12 -0400 Andrew Geery andrew.ge...@gmail.com wrote: The first failure I get is in the inherit tests (tail of /src/test/regress/results/inherit.out): alter table a alter column aa type integer using bit_length(aa); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. Connection to server was lost This test consistently breaks in this location and breaks for both make check and make installcheck. Okay, can reproduce it on a Linux box here, will be back with a fixed version. -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
On 14 October 2010 17:40, Bernd Helmle maili...@oopsware.de wrote: --On 14. Oktober 2010 11:42:27 -0400 Robert Haas robertmh...@gmail.com wrote: I did a sanity make clean make make check before applying the patch and all the tests passed. After applying the patch and doing make clean make make check, I got a number of failures of the form “FAILED (test process exited with exit code 2)”. The exact number of failures varies by run, so I’m wondering if I didn’t do something wrong... That indicates that PostgreSQL is crashing. So I think this patch is definitely not ready for prime time yet, and needs some debugging. In view of the fact that we are out of time for this CommitFest, I'm going to mark this Returned with Feedback in the CommitFest application. Hopefully it will be resubmitted for the next CommitFest after further refinement, because I think this is a good and useful improvement. Yeah, its crashing but it doesn't do it here on my MacBook (passing the regression test is one of my top prio when submitting a patch). Must be some broken pointer or similar oversight which is triggered on Andrew's box. Andrew, which OS and architecture have you tested on? Yeah, it crashes for me too (opensuse 11.2 64-bit) but only in an optimised build. There are a couple of compiler warnings: tablecmds.c: In function 'ATExecSetNotNull': tablecmds.c:4747: warning: unused variable 'is_new_constraint' tablecmds.c: In function 'ATExecDropNotNull': tablecmds.c:4332: warning: 'found' may be used uninitialized in this function tablecmds.c:4246: warning: 'children' may be used uninitialized in this function The last 2 look serious enough to cause problems, but fixing them didn't cure the crash. Digging deeper, I get a segfault running src/test/regress/sql/alter_table.sql: Program received signal SIGSEGV, Segmentation fault. ATExecSetNotNullInternal (is_local=1 '\001', is_new_constraint=value optimized out, atttup=value optimized out, attr_rel=value optimized out, rel=value optimized out) at tablecmds.c:4847 4847Form_pg_constraint constr = (Form_pg_constraint) GETSTRUCT(copy_tuple); Looking in that function, there is a similar found variable that isn't being initialised (which my compiler didn't warn about). Initialising that to false, sems to fix the problem and all the regression tests then pass. 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Looking in that function, there is a similar found variable that isn't being initialised (which my compiler didn't warn about). Initialising that to false, sems to fix the problem and all the regression tests then pass. Excellent. Please send an updated patch. -- Á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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 14. Oktober 2010 19:16:56 +0100 Dean Rasheed dean.a.rash...@gmail.com wrote: Program received signal SIGSEGV, Segmentation fault. ATExecSetNotNullInternal (is_local=1 '\001', is_new_constraint=value optimized out, atttup=value optimized out, attr_rel=value optimized out, rel=value optimized out) at tablecmds.c:4847 4847Form_pg_constraint constr = (Form_pg_constraint) GETSTRUCT(copy_tuple); Looking in that function, there is a similar found variable that isn't being initialised (which my compiler didn't warn about). Initialising that to false, sems to fix the problem and all the regression tests then pass. Yepp, that was it. I had a CFLAGS='-O0' in my dev build from a former debugging cycle and forgot about it (which reminds me to do a maintainer-clean more often between coding). This is also the reason i haven't seen the compiler warnings and the crash in the regression tests. Shame on me, but i think i have learned the lesson ;) -- Thanks Bernd -- 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] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Excerpts from Bernd Helmle's message of jue oct 14 16:44:36 -0300 2010: Yepp, that was it. I had a CFLAGS='-O0' in my dev build from a former debugging cycle and forgot about it (which reminds me to do a maintainer-clean more often between coding). This is also the reason i haven't seen the compiler warnings and the crash in the regression tests. Shame on me, but i think i have learned the lesson ;) A better way to do this is create src/Makefile.custom and add this line: CFLAGS := $(filter-out -O2,$(CFLAGS)) -O0 -- Á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
[HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
--On 29. September 2010 23:05:11 -0400 Andrew Geery andrew.ge...@gmail.com wrote: Reference: https://commitfest.postgresql.org/action/patch_view?id=312 The patch from http://archives.postgresql.org/message-id/ca2e4c4762eae28d68404...@amenop his does not apply cleanly to the current git master: Yeah, there where some changes in the meantime to the master which generate some merge failures...will provide a new version along with other fixes soon. Are you going to update the commitfest page? Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: starting to review the Extend NOT NULL representation to pg_constraint patch
Ok -- I've updated the commitfest page linking in this review and changing the status to waiting on a new patch from you. Thanks Andrew On Thu, Sep 30, 2010 at 4:12 AM, Bernd Helmle maili...@oopsware.de wrote: --On 29. September 2010 23:05:11 -0400 Andrew Geery andrew.ge...@gmail.com wrote: Reference: https://commitfest.postgresql.org/action/patch_view?id=312 The patch from http://archives.postgresql.org/message-id/ca2e4c4762eae28d68404...@amenop his does not apply cleanly to the current git master: Yeah, there where some changes in the meantime to the master which generate some merge failures...will provide a new version along with other fixes soon. Are you going to update the commitfest page? Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
