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;

Search Discussions

  • Bernd Helmle at Sep 30, 2010 at 8:49 am
    --On 29. September 2010 23:05:11 -0400 Andrew Geery
    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. Are you going to update the commitfest page?

    Thanks
    Bernd
  • Andrew Geery at Sep 30, 2010 at 12:45 pm
    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 wrote:


    --On 29. September 2010 23:05:11 -0400 Andrew Geery 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. Are you going to update the commitfest page?

    Thanks
    Bernd


  • Bernd Helmle at Oct 5, 2010 at 9:21 pm

    --On 30. September 2010 10:12:48 +0200 Bernd Helmle 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
  • Robert Haas at Oct 14, 2010 at 9:40 am

    On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle wrote:
    --On 30. September 2010 10:12:48 +0200 Bernd Helmle 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
  • Andrew Geery at Oct 14, 2010 at 9:48 am
    I'll post it sometime tomorrow...
    Thanks
    Andrew
    On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas wrote:
    On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle wrote:
    --On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@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
  • Andrew Geery at Oct 14, 2010 at 2:26 pm
    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 wrote:
    I'll post it sometime tomorrow...
    Thanks
    Andrew
    On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas wrote:
    On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle wrote:
    --On 30. September 2010 10:12:48 +0200 Bernd Helmle <mailings@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
  • Robert Haas at Oct 14, 2010 at 3:50 pm

    On Thu, Oct 14, 2010 at 10:02 AM, Andrew Geery 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
  • Dean Rasheed at Oct 14, 2010 at 4:44 pm

    On 14 October 2010 16:42, Robert Haas 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
  • Bernd Helmle at Oct 14, 2010 at 5:05 pm

    --On 14. Oktober 2010 11:42:27 -0400 Robert Haas 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
  • Dean Rasheed at Oct 14, 2010 at 6:22 pm

    On 14 October 2010 17:40, Bernd Helmle wrote:

    --On 14. Oktober 2010 11:42:27 -0400 Robert Haas 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
  • Alvaro Herrera at Oct 14, 2010 at 7:29 pm

    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Dean Rasheed at Oct 14, 2010 at 7:47 pm

    On 14 October 2010 20:28, Alvaro Herrera 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
  • Bernd Helmle at Oct 15, 2010 at 7:34 am
    --On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed
    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
  • Dean Rasheed at Oct 15, 2010 at 8:06 am

    On 15 October 2010 08:32, Bernd Helmle wrote:

    --On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed 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
  • Robert Haas at Oct 15, 2010 at 1:22 pm

    On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed 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
  • Tom Lane at Oct 15, 2010 at 2:27 pm

    Robert Haas writes:
    On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed 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 at Oct 14, 2010 at 9:03 pm
    --On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
    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
  • Andrew Geery at Oct 15, 2010 at 1:27 pm
    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 wrote:


    --On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
    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
  • Tom Lane at Oct 15, 2010 at 5:36 pm

    Bernd Helmle 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
  • Dimitri Fontaine at Oct 15, 2010 at 8:45 pm

    Tom Lane 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
  • Peter Eisentraut at Oct 16, 2010 at 9:35 am

    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.
  • Bernd Helmle at Oct 16, 2010 at 4:44 pm

    --On 16. Oktober 2010 12:35:06 +0300 Peter Eisentraut 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
  • Peter Eisentraut at Jan 3, 2011 at 8:19 pm
    Bernd, are you still working on this?

    On fre, 2010-10-15 at 13:36 -0400, Tom Lane wrote:
    Bernd Helmle <mailings@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.
  • Alvaro Herrera at May 27, 2011 at 9:02 pm
    I intend to have a look at this patch and hopefully fix the outstanding
    issues.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Alvaro Herrera at Jun 16, 2011 at 5:02 pm
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Jun 16, 2011 at 5:25 pm

    Alvaro Herrera 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
  • Robert Haas at Jun 16, 2011 at 5:34 pm

    On Thu, Jun 16, 2011 at 1:25 PM, Tom Lane 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
  • Bernd Helmle at Jun 16, 2011 at 6:31 pm

    --On 16. Juni 2011 13:25:05 -0400 Tom Lane 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
  • Alvaro Herrera at Jun 16, 2011 at 7:34 pm

    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 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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Alvaro Herrera at Jun 24, 2011 at 3:17 pm
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Jun 24, 2011 at 4:06 pm

    On Fri, Jun 24, 2011 at 11:17 AM, Alvaro Herrera 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
  • Alvaro Herrera at Jun 24, 2011 at 4:17 pm

    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
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Bernd Helmle at Jun 24, 2011 at 4:56 pm

    --On 24. Juni 2011 12:06:17 -0400 Robert Haas 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
  • Alvaro Herrera at Jun 24, 2011 at 5:17 pm

    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 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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Alvaro Herrera at Jun 24, 2011 at 10:39 pm
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Jun 24, 2011 at 11:02 pm

    On Fri, Jun 24, 2011 at 6:39 PM, Alvaro Herrera 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
  • Alvaro Herrera at Jun 25, 2011 at 1:00 am

    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
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Dean Rasheed at Jun 25, 2011 at 6:15 am

    On 25 June 2011 01:59, Alvaro Herrera 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
  • Robert Haas at Jun 27, 2011 at 2:31 am

    On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed 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
  • Dean Rasheed at Jun 27, 2011 at 7:08 am

    On 27 June 2011 03:31, Robert Haas wrote:
    On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed 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
  • Robert Haas at Jun 27, 2011 at 2:36 pm

    On Mon, Jun 27, 2011 at 3:08 AM, Dean Rasheed wrote:
    On 27 June 2011 03:31, Robert Haas wrote:
    On Sat, Jun 25, 2011 at 2:15 AM, Dean Rasheed 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
  • Alvaro Herrera at Jun 29, 2011 at 4:51 pm

    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 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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Jun 29, 2011 at 5:09 pm

    On Wed, Jun 29, 2011 at 12:51 PM, Alvaro Herrera 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 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
  • Alvaro Herrera at Jun 29, 2011 at 9:00 pm

    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
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Robert Haas at Jun 29, 2011 at 10:16 pm

    On Wed, Jun 29, 2011 at 4:59 PM, Alvaro Herrera 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
    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
  • Alvaro Herrera at Jun 29, 2011 at 10:42 pm

    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
    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
    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Bernd Helmle at Oct 14, 2010 at 8:12 pm
    --On 14. Oktober 2010 19:16:56 +0100 Dean Rasheed
    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
  • Alvaro Herrera at Oct 15, 2010 at 2:56 am

    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Bernd Helmle at Oct 14, 2010 at 6:14 pm

    --On 14. Oktober 2010 10:02:12 -0400 Andrew Geery 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
  • Bernd Helmle at Oct 16, 2010 at 1:31 am

    --On 15. Oktober 2010 13:36:38 -0400 Tom Lane wrote:

    Bernd Helmle <mailings@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 recursion....I 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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 30, '10 at 3:05a
activeJun 29, '11 at 10:42p
posts51
users9
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase