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.geery@gmail.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
Show quoted text
On Thu, Sep 30, 2010 at 4:12 AM, Bernd Helmle <mailings@oopsware.de> wrote:
--On 29. September 2010 23:05:11 -0400 Andrew Geery <andrew.geery@gmail.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@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.
--
Thanks
Bernd
Attachments:
notnull_constraint.patchapplication/octet-stream; name=notnull_constraint.patchDownload+986-804
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> 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 soonHere'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
Show quoted text
On Wed, Oct 13, 2010 at 9:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> 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 soonHere'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 fileRemoving 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.
Show quoted text
On Wed, Oct 13, 2010 at 9:41 PM, Andrew Geery <andrew.geery@gmail.com> wrote:
I'll post it sometime tomorrow...
Thanks
AndrewOn Wed, Oct 13, 2010 at 9:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Oct 5, 2010 at 4:57 PM, Bernd Helmle <mailings@oopsware.de> 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 soonHere'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.geery@gmail.com> wrote:
I didn’t have much time to look at the code. The only thing I’ll
mention is that there are a couple of XXX TODO items that should be
cleared up.
[...]
Since this patch actually makes inheritance behave in a more expected
way, nothing needs to be changed in the inheritance documentation.
However, at the very least, the documentation dealing with the
pg_catalog [http://www.postgresql.org/docs/9.0/interactive/catalog-pg-constraint.html]
needs to be updated to deal with the new constraint type.
Thanks for catching these problems.
I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...
That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas@gmail.com>
wrote:
I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.
Yeah, its crashing but it doesn't do it here on my MacBook.... (passing the
regression test is one of my top prio when submitting a patch). Must be
some broken pointer or similar oversight which is triggered on Andrew's
box. Andrew, which OS and architecture have you tested on?
--
Thanks
Bernd
On 14 October 2010 16:42, Robert Haas <robertmhaas@gmail.com> wrote:
In view of the fact that we are out of time for this CommitFest, ...
When is the official end of this commitfest?
I remember talk at the start, that the end would be postponed (by a
week?) due to time spent on the git migration. Is that still the case?
Regards,
Dean
--On 14. Oktober 2010 10:02:12 -0400 Andrew Geery <andrew.geery@gmail.com>
wrote:
The first failure I get is in the inherit tests (tail of
/src/test/regress/results/inherit.out):alter table a alter column aa type integer using bit_length(aa);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
Connection to server was lostThis 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@oopsware.de> wrote:
--On 14. Oktober 2010 11:42:27 -0400 Robert Haas <robertmhaas@gmail.com>
wrote:I did a sanity make clean && make && make check before applying the
patch and all the tests passed. After applying the patch and doing
make clean && make && make check, I got a number of failures of the
form “FAILED (test process exited with exit code 2)”. The exact
number of failures varies by run, so I’m wondering if I didn’t do
something wrong...That indicates that PostgreSQL is crashing. So I think this patch is
definitely not ready for prime time yet, and needs some debugging. In
view of the fact that we are out of time for this CommitFest, I'm
going to mark this Returned with Feedback in the CommitFest
application. Hopefully it will be resubmitted for the next CommitFest
after further refinement, because I think this is a good and useful
improvement.Yeah, its crashing but it doesn't do it here on my MacBook.... (passing the
regression test is one of my top prio when submitting a patch). Must be some
broken pointer or similar oversight which is triggered on Andrew's box.
Andrew, which OS and architecture have you tested on?
Yeah, it crashes for me too (opensuse 11.2 64-bit) but only in an
optimised build.
There are a couple of compiler warnings:
tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'
tablecmds.c: In function 'ATExecDropNotNull':
tablecmds.c:4332: warning: 'found' may be used uninitialized in this function
tablecmds.c:4246: warning: 'children' may be used uninitialized in this function
The last 2 look serious enough to cause problems, but fixing them
didn't cure the crash.
Digging deeper, I get a segfault running src/test/regress/sql/alter_table.sql:
Program received signal SIGSEGV, Segmentation fault.
ATExecSetNotNullInternal (is_local=1 '\001',
is_new_constraint=<value optimized out>, atttup=<value optimized out>,
attr_rel=<value optimized out>, rel=<value optimized out>)
at tablecmds.c:4847
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@commandprompt.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.a.rasheed@gmail.com> wrote:
Program received signal SIGSEGV, Segmentation fault.
ATExecSetNotNullInternal (is_local=1 '\001',
is_new_constraint=<value optimized out>, atttup=<value optimized out>,
attr_rel=<value optimized out>, rel=<value optimized out>)
at tablecmds.c:4847
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@commandprompt.com> wrote:
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.Excellent. Please send an updated patch.
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
Attachments:
notnull_constraint2.patchapplication/octet-stream; name=notnull_constraint2.patchDownload+985-804
--On 14. Oktober 2010 16:28:51 -0300 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Looking in that function, there is a similar "found" variable that
isn't being initialised (which my compiler didn't warn about).
Initialising that to false, sems to fix the problem and all the
regression tests then pass.Excellent. Please send an updated patch.
Here is an updated version of the patch. It fixes the following issues
Andrew discovered during his review cycle:
* Fix compiler warnings and crash due to uninitialized variables (pretty
much the fix Dean proposed)
* Remove accidentally added pg_latch.c in my own git repos.
I will do further cycles over Andrew's review report.
--
Thanks
Bernd
Attachments:
notnull_constraint.patchapplication/octet-stream; name=notnull_constraint.patchDownload+989-808
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
--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed
<dean.a.rasheed@gmail.com> wrote:
OK, here it is.
I've not cured this compiler warning (in fact I'm not sure what it's
complaining about, because the variable *is* used):tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'
Just a left over from earlier coding, i have removed this in my patch
version. I have adapted your fixes and would like to propose that we are
proceeding with my version, if that's okay for you?
--
Thanks
Bernd
On 15 October 2010 08:32, Bernd Helmle <mailings@oopsware.de> wrote:
--On 14. Oktober 2010 20:47:27 +0100 Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:OK, here it is.
I've not cured this compiler warning (in fact I'm not sure what it's
complaining about, because the variable *is* used):tablecmds.c: In function 'ATExecSetNotNull':
tablecmds.c:4747: warning: unused variable 'is_new_constraint'Just a left over from earlier coding, i have removed this in my patch
version. I have adapted your fixes and would like to propose that we are
proceeding with my version, if that's okay for you?
Ah, I see it (I was looking at the wrong copy of that variable).
Yes, let's proceed with your version.
If these were the only problems with this patch, given that they
required only trivial changes to initialise variables to NIL/false,
perhaps it was premature to mark it as "returned with feedback".
Thoughts?
Regards,
Dean
On Fri, Oct 15, 2010 at 4:06 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
If these were the only problems with this patch, given that they
required only trivial changes to initialise variables to NIL/false,
perhaps it was premature to mark it as "returned with feedback".
Sure. Is someone available to do a detailed code review? Alvaro,
were you planning to pick this one up?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
