Re: 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/E57A252DFD60C1FCA91731BD@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/57F9D81FFD86336C6F92B2CD@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(dot)geery(at)gmail(dot)com> wrote:
> I'll post it sometime tomorrow...
> Thanks
> Andrew
>
> On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>>> --On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings(at)oopsware(dot)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
>>
>