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 null column constraint for the child table in pg_constraint * create parent table with a not null column constraint; create a child table with a no not null column constraint that does not inherit from the parent; alter the child table to inherit from the parent -- verify that the db disallows this * create parent table with a not null column constraint; create a child table with a matching not null constraint that does not inherit from the parent; alter the child table to inherit from the parent -- verify that this is allowed and that the child’s pg_constraint.conihcount = 1 * create parent with a not null column constraint; create child table that inherits from the parent; drop the not null constraint from the child table -- verify that the db does not allow this * create parent with a not null column constraint; create child that inherits from the parent; alter the child table to not inherit from the parent -- verify that the child’s pg_constraint values are conislocal=t and coninhcount=0 * drop the not null constraints in the scenarios above and verify that the corresponding rows are removed from pg_constraint Error messages changed =================== The following error messages have been changed/added: Was: column %s contains null values Patched: column %s of relation %s contains null values Was: cannot alter system column %s Patched: cannot alter system column %s of relation %s New error message: NOT NULL constraint must be added to child tables too Code ==== 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. Documentation =========== 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. Tests ==== 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... 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. However, when I just pipe the /src/test/regress/sql/inherit.sql file through psql, the connection does not close unexpectedly because the error, “function bit_length(integer) does not exist”, is given for the statement in question. I’m not sure why there is a discrepancy here or why the test passes before the patch but not after the patch... Discussion ======== Below are excerpts from the lists about the problem and patch. * The problem, with example, was very succinctly described in this message by Bernd Helmle: http://archives.postgresql.org/pgsql-hackers/2009-11/msg00146.php * The underlying problem was described by Tom Lane here: [http://archives.postgresql.org/pgsql-hackers/2009-11/msg00149.php]: “The ALTER should be rejected, but it is not, because we don't have enough infrastructure to notice that the constraint is inherited and logically can't be dropped. I think the consensus was that the way to fix this (along with some other problems) is to start representing NOT NULL constraints in pg_constraint, turning attnotnull into just a bit of denormalization for performance.” * The basic patch proposal is described here by Bernd Helme: http://archives.postgresql.org/pgsql-hackers/2009-11/msg00536.php My first idea is to just introduce a special contype in pg_constraint representing a NOT NULL constraint on a column, which holds all required information to do the mentioned maintenance stuff on them and to keep most of the current infrastructure. Utility commands need to track all changes in pg_constraint and keep pg_attribute.attnotnull up to date. * Follow-up discussion here from Tom Lane, agreeing that a special constraint is better than handling not null as a generic check: http://archives.postgresql.org/pgsql-hackers/2009-11/msg00556.php Testing attnotnull is significantly cheaper than enforcing a generic constraint expression, and NOT NULL is a sufficiently common case to be worth worrying about optimizing it. Furthermore, removing attnotnull would break an unknown but probably not-negligible amount of client-side code that cares whether columns are known not null (I think both JDBC and ODBC track that). * Detailed description of the patch from Bernd Helme: http://archives.postgresql.org/message-id/57f9d81ffd86336c6f92b...@amenophis The patch creates a new CONSTRAINT_NOTNULL contype and assigns all required information for the NOT NULL constraint to it. Currently the constraint records the attribute number it belongs to and manages the inheritance properties. Passes regression tests with some adjustments to pg_constraint output. The patch as it stands employs a dedicated code path for ATExecDropNotNull(), thus duplicates the behavior of ATExecDropConstraint(). I'm not really satisfied with this, but i did it this way to prevent some heavy conditional rearrangement inATExecDropConstraint(). Maybe its worth to move the code to adjust constraint inheritance properties into a separate function. On Wed, Oct 13, 2010 at 9:41 PM, Andrew Geery <andrew.ge...@gmail.com> wrote: > 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
