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(at)postgresql(dot)org [mailto:pgsql-bugs-owner(at)postgresql(dot)org] On Behalf Of miroslav(dot)sulc(at)fordfrog(dot)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(at)leadboat(dot)com]
Sent: Monday, July 16, 2012 2:54 AM
> > From: pgsql-bugs-owner(at)postgresql(dot)org
[mailto:pgsql-bugs-owner(at)postgresql(dot)org] On Behalf Of
miroslav(dot)sulc(at)fordfrog(dot)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(at)leadboat(dot)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?

Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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.

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