Re: BUG #6712: PostgreSQL 9.2 beta2: alter table drop constraint does not work on inherited master table
On Mon, Jul 02, 2012 at 04:16:31PM +0530, Amit Kapila wrote:
From: pgsql-bugs-owner@postgresql.org [mailto:pgsql-bugs-owner@postgresql.org] On Behalf Of miroslav.sulc@fordfrog.com
Sent: Saturday, June 30, 2012 4:28 PM
test=# create table test_constraints (id int, val1 varchar, val2 int, unique
(val1, val2));
NOTICE: CREATE TABLE / UNIQUE will create implicit index
"test_constraints_val1_val2_key" for table "test_constraints"
CREATE TABLE
test=# create table test_constraints_inh () inherits (test_constraints);
CREATE TABLE
test=# alter table only test_constraints drop constraint
test_constraints_val1_val2_key;
ERROR: constraint "test_constraints_val1_val2_key" of relation
"test_constraints_inh" does not exist
postgresql tries to drop the constraint even from descendant table though
"only" is specified.
ONLY shouldn't be necessary, either.
In function ATExecDropConstraint(), for the constarint "test_constraints_val1_val2_key" con->connoinherit is false,
due to which it tries to drop the constrint from child table as well.
I have checked that from function index_constraint_create() when it calls function CreateConstraintEntry(), the flag for noinherit passed is false.
I think this is the reason of failure for the same.
Agreed. The other non-CHECK callers also get this wrong:
[local] regression=# select contype,connoinherit,count(*) from pg_constraint group by 1,2;
contype | connoinherit | count
---------+--------------+-------
p | f | 7
x | f | 2
c | f | 17
f | f | 5
One can construct similar bugs around dropping foreign key and exclusion
constraints. Though it may be irrelevant for command semantics, additionally
using connoinherit = 't' for contype = 't' (CONSTRAINT_TRIGGER) would
more-accurately represent the nature of those constraints.
Care to prepare a patch with a test case addition?
Thanks,
nm
From: Noah Misch [mailto:noah@leadboat.com]
Sent: Monday, July 16, 2012 2:54 AM
From: pgsql-bugs-owner@postgresql.org
[mailto:pgsql-bugs-owner@postgresql.org] On Behalf Of
miroslav.sulc@fordfrog.com
Sent: Saturday, June 30, 2012 4:28 PM
Care to prepare a patch with a test case addition?
Yes. I have started working.
With Regards,
Amit Kapila.
From: Noah Misch [mailto:noah@leadboat.com]
Sent: Monday, July 16, 2012 2:54 AM
One can construct similar bugs around dropping foreign key and exclusion
constraints. Though it may be irrelevant for command semantics,
additionally
using connoinherit = 't' for contype = 't' (CONSTRAINT_TRIGGER) would
more-accurately represent the nature of those constraints.
Code Changes
----------------
I will make changes in following functions to ensure that connoinherit
should be appropriately set(pass the value as true).
a. index_constraint_create()
b. ATAddForeignKeyConstraint()
c. CreateTrigger().
Other places I have checked seems to be fine.
Test
--------
I will create testcases similar to mentioned in bug report for
a. unique key case, same as in bug-report
b. foreign key case
c. exclusion constraint case
Care to prepare a patch with a test case addition?
Let me know if above is sufficient or shall I include anything more in
patch.
With Regards,
Amit Kapila.
On Mon, Jul 16, 2012 at 04:49:46PM +0530, Amit Kapila wrote:
Code Changes
----------------
I will make changes in following functions to ensure that connoinherit
should be appropriately set(pass the value as true).
a. index_constraint_create()
b. ATAddForeignKeyConstraint()
c. CreateTrigger().Other places I have checked seems to be fine.
Looks right.
Test
--------
I will create testcases similar to mentioned in bug report for
a. unique key case, same as in bug-report
Good.
b. foreign key case
c. exclusion constraint case
Test coverage for those is optional.
Care to prepare a patch with a test case addition?
Let me know if above is sufficient or shall I include anything more in
patch.
I can't think of anything else.
As a side question for the list, should we fix this differently in 9.2 to
avoid forcing an initdb for the next beta? Perhaps have
ATExecDropConstraint() only respect connoinherit for CONSTRAINT_CHECK?
Import Notes
Reply to msg id not found: 5003f940.49dc440a.671f.3d72SMTPIN_ADDED@mx.google.com
Noah Misch <noah@leadboat.com> writes:
As a side question for the list, should we fix this differently in 9.2 to
avoid forcing an initdb for the next beta?
I'm confused, why would an initdb be involved?
regards, tom lane
On Mon, Jul 16, 2012 at 12:47:37PM -0400, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
As a side question for the list, should we fix this differently in 9.2 to
avoid forcing an initdb for the next beta?I'm confused, why would an initdb be involved?
pg_constraint.connoinherit is presently only correct for CHECK constraints.
It will take an initdb to update those values comprehensively.
From: Noah Misch [noah@leadboat.com]
Sent: Monday, July 16, 2012 8:42 PM
On Mon, Jul 16, 2012 at 04:49:46PM +0530, Amit Kapila wrote:
Care to prepare a patch with a test case addition?
Let me know if above is sufficient or shall I include anything more in
patch.
I can't think of anything else.
Patch is attached with this mail.
As a side question for the list, should we fix this differently in 9.2 to
avoid forcing an initdb for the next beta? Perhaps have
ATExecDropConstraint() only respect connoinherit for CONSTRAINT_CHECK?
Let me know if anything needs to be done for this?
With Regards,
Amit Kapila.
Attachments:
defect_fix_6712.patchtext/plain; name=defect_fix_6712.patchDownload+156-3
On Tue, Jul 17, 2012 at 08:59:50AM +0000, Amit kapila wrote:
Patch is attached with this mail.
Thanks. This patch is ready for committer.
+-- Test non-inheritable indices [UNIQUE, EXCLUDE] contraints +CREATE TABLE test_constraints (id int, val1 varchar, val2 int, UNIQUE(val1, val2)); +CREATE TABLE test_constraints_inh () INHERITS (test_constraints); +\d+ test_constraints +ALTER TABLE ONLY test_constraints DROP CONSTRAINT test_constraints_val1_val2_key; +\d+ test_constraints +\d+ test_constraints_inh
To keep output terse, I would have omitted "\d" commands or retained only the
post-DROP "\d+ test_constraints". Granted, that's subjective.
From: Noah Misch [mailto:noah@leadboat.com]
Sent: Thursday, July 19, 2012 5:23 PM
On Tue, Jul 17, 2012 at 08:59:50AM +0000, Amit kapila wrote:
Patch is attached with this mail.
Thanks. This patch is ready for committer.
+-- Test non-inheritable indices [UNIQUE, EXCLUDE] contraints +CREATE TABLE test_constraints (id int, val1 varchar, val2 int,
UNIQUE(val1, val2));
+CREATE TABLE test_constraints_inh () INHERITS (test_constraints); +\d+ test_constraints +ALTER TABLE ONLY test_constraints DROP CONSTRAINT
test_constraints_val1_val2_key;
+\d+ test_constraints
+\d+ test_constraints_inh
To keep output terse, I would have omitted "\d" commands or retained only
the
post-DROP "\d+ test_constraints". Granted, that's subjective.
Thanks for the review of patch.
With Regards,
Amit Kapila.
Excerpts from Amit Kapila's message of jue jul 19 22:57:04 -0400 2012:
From: Noah Misch [mailto:noah@leadboat.com]
Sent: Thursday, July 19, 2012 5:23 PM
On Tue, Jul 17, 2012 at 08:59:50AM +0000, Amit kapila wrote:Patch is attached with this mail.
Thanks. This patch is ready for committer.
Thanks, will see about it.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Noah Misch's message of lun jul 16 11:12:01 -0400 2012:
As a side question for the list, should we fix this differently in 9.2 to
avoid forcing an initdb for the next beta? Perhaps have
ATExecDropConstraint() only respect connoinherit for CONSTRAINT_CHECK?
My answer here was "yes", so the patch I just pushed to the 9.2 branch
contains such an override. I also included a catversion bump in HEAD
even though the catalog inconsistency is likly to be very minor.
Thanks.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support