Re: Re: starting to review the Extend NOT NULL representation to pg_constraint patch

Reference: https://commitfest.postgresql.org/action/patch_view?id=312

The patch from http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@amenophis
does not apply cleanly to the current git master:

$ patch -p1 < extend_not_null.patch
patching file src/backend/catalog/heap.c
patching file q
Hunk #3 succeeded at 290 (offset 1 line).
Hunk #4 succeeded at 351 (offset 1 line).
Hunk #5 succeeded at 530 (offset 1 line).
Hunk #6 FAILED at 2709.
Hunk #7 succeeded at 2957 (offset 18 lines).
Hunk #8 succeeded at 4227 (offset 68 lines).
Hunk #9 succeeded at 4574 (offset 68 lines).
Hunk #10 succeeded at 4584 (offset 68 lines).
Hunk #11 succeeded at 4673 (offset 68 lines).
Hunk #12 succeeded at 6298 (offset 72 lines).
Hunk #13 succeeded at 6312 (offset 72 lines).
Hunk #14 succeeded at 6385 (offset 72 lines).
Hunk #15 succeeded at 7098 (offset 89 lines).
Hunk #16 succeeded at 7860 (offset 89 lines).
Hunk #17 succeeded at 7947 (offset 89 lines).
Hunk #18 succeeded at 8027 (offset 89 lines).
Hunk #19 succeeded at 8075 (offset 89 lines).
Hunk #20 succeeded at 8118 (offset 89 lines).
Hunk #21 succeeded at 8146 (offset 89 lines).
Hunk #22 succeeded at 8163 (offset 89 lines).
Hunk #23 succeeded at 8246 (offset 89 lines).
Hunk #24 succeeded at 8260 (offset 89 lines).
Hunk #25 succeeded at 8310 (offset 89 lines).
Hunk #26 succeeded at 8325 (offset 89 lines).
Hunk #27 succeeded at 8333 (offset 89 lines).
Hunk #28 succeeded at 8387 (offset 89 lines).
Hunk #29 succeeded at 8417 (offset 89 lines).
1 out of 29 hunks FAILED -- saving rejects to file
src/backend/commands/tablecmds.c.rej
patching file src/backend/parser/parse_utilcmd.c
patching file src/backend/port/pg_latch.c
patching file src/backend/utils/adt/ruleutils.c
patching file src/include/catalog/heap.h
patching file src/include/catalog/pg_constraint.h
patching file src/include/nodes/parsenodes.h
Hunk #1 succeeded at 1117 (offset 1 line).
patching file src/test/regress/expected/alter_table.out
patching file src/test/regress/expected/cluster.out

$ cat src/backend/commands/tablecmds.c.rej
*** src/backend/commands/tablecmds.c
--- src/backend/commands/tablecmds.c
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2709,2715 ****
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;
--- 2752,2761 ----
break;
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(rel, false);
!
! if (recurse)
! cmd->subtype = AT_DropNotNullRecurse;
!
/* No command-specific prep needed */
pass = AT_PASS_DROP;
break;

--On 29. September 2010 23:05:11 -0400 Andrew Geery
<andrew(dot)geery(at)gmail(dot)com> wrote:

> Reference: https://commitfest.postgresql.org/action/patch_view?id=312
>
> The patch from
> http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@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

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 <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 29. September 2010 23:05:11 -0400 Andrew Geery <andrew(dot)geery(at)gmail(dot)com>
> wrote:
>
>> Reference: https://commitfest.postgresql.org/action/patch_view?id=312
>>
>> The patch from
>> http://archives.postgresql.org/message-id/CA2E4C4762EAE28D68404E75@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
>
>
>
>

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

--
Thanks

Bernd

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

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
>

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

On Thu, Oct 14, 2010 at 10:02 AM, Andrew Geery <andrew(dot)geery(at)gmail(dot)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

--On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas(at)gmail(dot)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

On 14 October 2010 16:42, Robert Haas <robertmhaas(at)gmail(dot)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

--On 14. Oktober 2010 10:02:12 -0400 Andrew Geery <andrew(dot)geery(at)gmail(dot)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

On 14 October 2010 17:40, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas(at)gmail(dot)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
4847 Form_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

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

--On 14. Oktober 2010 19:16:56 +0100 Dean Rasheed
<dean(dot)a(dot)rasheed(at)gmail(dot)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
> 4847 Form_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

On 14 October 2010 20:28, Alvaro Herrera <alvherre(at)commandprompt(dot)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.
>

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'

Regards,
Dean

--On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
<alvherre(at)commandprompt(dot)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

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

--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed
<dean(dot)a(dot)rasheed(at)gmail(dot)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

On 15 October 2010 08:32, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

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 <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
> <alvherre(at)commandprompt(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Bernd Helmle <mailings(at)oopsware(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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.

--On 16. Oktober 2010 12:35:06 +0300 Peter Eisentraut <peter_e(at)gmx(dot)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

Bernd, are you still working on this?

On fre, 2010-10-15 at 13:36 -0400, Tom Lane wrote:
> Bernd Helmle <mailings(at)oopsware(dot)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.

I intend to have a look at this patch and hopefully fix the outstanding
issues.

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

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

Alvaro Herrera <alvherre(at)commandprompt(dot)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

On Thu, Jun 16, 2011 at 1:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

--On 16. Juni 2011 13:25:05 -0400 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

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

On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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

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

--On 24. Juni 2011 12:06:17 -0400 Robert Haas <robertmhaas(at)gmail(dot)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

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

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

On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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

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

On 25 June 2011 01:59, Alvaro Herrera <alvherre(at)commandprompt(dot)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

On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 27 June 2011 03:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 27 June 2011 03:31, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

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

On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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(dot)a(dot)rasheed(at)gmail(dot)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

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

On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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
>> <alvherre(at)commandprompt(dot)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

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