Re: [HACKERS] Review of patch renaming constraints
Hi, Peter Eisentraut pete...@gmx.net writes: On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote: Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints: New patch which works around that issue. I've been reviewing this new patch and it seems ready for commiter for me: the code indeed looks like it's always been there, the corner cases are covered in the added regression tests, including the one Josh ran into problem with in the previous round of testing. The regression test covering made me lazy enough not to retry the patch here, I trust Peter on testing his own work here :) I'll update my command trigger patch as soon as this one makes it in. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of patch renaming constraints
On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote:
Most normal uses of alter table ... rename constraint ... worked
normally. However, the patch does not deal correctly with constraints
which are not inherited, such as primary key constraints:
New patch which works around that issue.
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 951b63b..c3039c8 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -25,6 +25,8 @@ ALTER TABLE [ IF EXISTS ] [ ONLY ] replaceable class=PARAMETERname/replacea
replaceable class=PARAMETERaction/replaceable [, ... ]
ALTER TABLE [ IF EXISTS ] [ ONLY ] replaceable class=PARAMETERname/replaceable [ * ]
RENAME [ COLUMN ] replaceable class=PARAMETERcolumn/replaceable TO replaceable class=PARAMETERnew_column/replaceable
+ALTER TABLE [ IF EXISTS ] [ ONLY ] replaceable class=PARAMETERname/replaceable [ * ]
+RENAME CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable TO replaceable class=PARAMETERnew_constraint_name/replaceable
ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
RENAME TO replaceable class=PARAMETERnew_name/replaceable
ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
@@ -569,8 +571,8 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
listitem
para
The literalRENAME/literal forms change the name of a table
- (or an index, sequence, or view) or the name of an individual column in
- a table. There is no effect on the stored data.
+ (or an index, sequence, or view), the name of an individual column in
+ a table, or the name of a constraint of the table. There is no effect on the stored data.
/para
/listitem
/varlistentry
@@ -883,7 +885,8 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable
para
If a table has any descendant tables, it is not permitted to add,
-rename, or change the type of a column in the parent table without doing
+rename, or change the type of a column, or rename an inherited constraint
+in the parent table without doing
the same to the descendants. That is, commandALTER TABLE ONLY/command
will be rejected. This ensures that the descendants always have
columns matching the parent.
@@ -983,6 +986,13 @@ ALTER TABLE distributors RENAME TO suppliers;
/para
para
+ To rename an existing constraint:
+programlisting
+ALTER TABLE distributors RENAME CONSTRAINT zipchk TO zip_check;
+/programlisting
+ /para
+
+ para
To add a not-null constraint to a column:
programlisting
ALTER TABLE distributors ALTER COLUMN street SET NOT NULL;
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 9175405..4dd9927 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -57,6 +57,10 @@ ExecRenameStmt(RenameStmt *stmt)
RenameCollation(stmt-object, stmt-newname);
break;
+ case OBJECT_CONSTRAINT:
+ RenameConstraint(stmt);
+ break;
+
case OBJECT_CONVERSION:
RenameConversion(stmt-object, stmt-newname);
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 07dc326..8dcaedc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2324,6 +2324,108 @@ renameatt(RenameStmt *stmt)
stmt-behavior);
}
+
+/*
+ * same logic as renameatt_internal
+ */
+static void
+rename_constraint_internal(Oid myrelid,
+ const char *oldconname,
+ const char *newconname,
+ bool recurse,
+ bool recursing,
+ int expected_parents)
+{
+ Relation targetrelation;
+ Oid constraintOid;
+ HeapTuple tuple;
+ Form_pg_constraint con;
+
+ targetrelation = relation_open(myrelid, AccessExclusiveLock);
+ /* don't tell it whether we're recursing; we allow changing typed tables here */
+ renameatt_check(myrelid, RelationGetForm(targetrelation), false);
+
+ constraintOid = get_constraint_oid(myrelid, oldconname, false);
+
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintOid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, cache lookup failed for constraint %u,
+ constraintOid);
+ con = (Form_pg_constraint) GETSTRUCT(tuple);
+
+ if (con-contype == CONSTRAINT_CHECK !con-conisonly)
+ {
+ if (recurse)
+ {
+ List *child_oids,
+*child_numparents;
+ ListCell *lo,
+*li;
+
+ child_oids = find_all_inheritors(myrelid, AccessExclusiveLock,
+ child_numparents);
+
+ forboth(lo, child_oids, li, child_numparents)
+ {
+Oid childrelid = lfirst_oid(lo);
+int numparents = lfirst_int(li);
+
+if (childrelid == myrelid)
+ continue;
+
+rename_constraint_internal(childrelid, oldconname, newconname, false, true, numparents);
+ }
+ }
+ else
+ {
+ if (expected_parents == 0
+find_inheritance_children(myrelid, NoLock) != NIL)
+ereport(ERROR,
+
Re: [HACKERS] Review of patch renaming constraints
On fre, 2012-01-20 at 11:32 +0530, Nikhil Sontakke wrote: Agreed. And right now primary key constraints are not marked as only making them available for inheritance in the future. Or you prefer it otherwise? Anyways, fail to see the direct connection between this and renaming. Might have to look at this patch for that. It checks conisonly to determine whether it needs to rename the constraint in child tables as well. Since a primary has conisonly = false, it goes to the child tables, but the constraint it not there. In the past, we have treated this merely as an implementation artifact: check constraints are inherited, primary key constraints are not. Now we can choose for check constraints, with inherited being the default. Having inheritable primary key constraints is a possible future feature. So we need to think a littler harder now how to work that into the existing logic. This also ties in with the other thread about having this in CREATE TABLE syntax. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of patch renaming constraints
On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote: Compiling on Ubuntu 10.04 LTS AMD64 on a GoGrid virtual machine from 2012-01-12 git checkout. Patch applied fine. Docs are present, build, look good and are clear. Changes to gram.y required Bison 2.5 to compile. Are we requiring Bison 2.5 now? There's no configure check for it, so it took me quite a while to figure out what was wrong. I can't reproduce that. I think there might be something wrong with your installation. The same issue was reported for my COLLATION FOR patch from the same environment. Make check passed. Patch has tests for rename constraint. Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints: That appears to be because creating a primary key constraint does not set pg_constraint.conisonly correctly. This was introduced recently with noninherited check constraints. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of patch renaming constraints
Make check passed. Patch has tests for rename constraint. Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints: That appears to be because creating a primary key constraint does not set pg_constraint.conisonly correctly. This was introduced recently with noninherited check constraints. Umm, conisonly is set as false from primary key entries in pg_constraint. And primary keys are anyways not inherited. So why is the conisonly field interfering in rename? Seems quite orthogonal to me. Regards, Nikhils
Re: [HACKERS] Review of patch renaming constraints
On fre, 2012-01-20 at 09:08 +0530, Nikhil Sontakke wrote: Umm, conisonly is set as false from primary key entries in pg_constraint. And primary keys are anyways not inherited. So why is the conisonly field interfering in rename? Seems quite orthogonal to me. In the past, each kind of constraint was either always inherited or always not, implicitly. Now, for check constraints we can choose what we want, and in the future, perhaps we will want to choose for primary keys as well. So having conisonly is really a good step into that future, and we should use it uniformly. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of patch renaming constraints
And primary keys are anyways not inherited. So why is the conisonly field interfering in rename? Seems quite orthogonal to me. In the past, each kind of constraint was either always inherited or always not, implicitly. Now, for check constraints we can choose what we want, and in the future, perhaps we will want to choose for primary keys as well. So having conisonly is really a good step into that future, and we should use it uniformly. Agreed. And right now primary key constraints are not marked as only making them available for inheritance in the future. Or you prefer it otherwise? Anyways, fail to see the direct connection between this and renaming. Might have to look at this patch for that. Regards, Nikhils
[HACKERS] Review of patch renaming constraints
Compiling on Ubuntu 10.04 LTS AMD64 on a GoGrid virtual machine from 2012-01-12 git checkout. Patch applied fine. Docs are present, build, look good and are clear. Changes to gram.y required Bison 2.5 to compile. Are we requiring Bison 2.5 now? There's no configure check for it, so it took me quite a while to figure out what was wrong. Make check passed. Patch has tests for rename constraint. Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints: create table master ( category text not null, status int not null, value text ); alter table master add constraint master_key primary key ( category, status ); alter table master rename constraint master_key to master_primary_key; create table partition_1 () inherits ( master ); create table partition_2 () inherits ( master ); alter table master rename constraint master_primary_key to master_key; postgres=# alter table master rename constraint master_primary_key to master_key; ERROR: constraint master_primary_key for table partition_1 does not exist STATEMENT: alter table master rename constraint master_primary_key to master_key; ERROR: constraint master_primary_key for table partition_1 does not exist -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com San Francisco -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
