Refactor some code around ALTER TABLE [NO] INHERIT

[NO] INHERIT is not supported for partitioned tables, but this portion
of tablecmds.c did not apply the same rules as the other sub-commands,
checking the relkind in the execution phase, not the preparation phase.

This commit refactors the code to centralize the relkind and other
checks in the preparation phase for both command patterns, getting rid
of one translatable string on the way.  ATT_PARTITIONED_TABLE is
removed from ATSimplePermissions(), and the child relation is checked
the same way for both sub-commands.  The ALTER TABLE patterns that now
fail at preparation failed already at execution, hence there should be
no changes from the user perspective except more consistent error
messages generated.

Some comments at the top of ATPrepAddInherit() were incorrect,
CreateInheritance() being the routine checking the columns and
constraints between the parent and its to-be-child.

Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dfdde986236fd2db78f7ed221db5e4922e93dc81..67e42e5df2956e7651d445a990eb022ba735366d 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -701,7 +701,7 @@ static void ATExecEnableDisableTrigger(Relation rel, const char *trigname,
                                                                           LOCKMODE lockmode);
 static void ATExecEnableDisableRule(Relation rel, const char *rulename,
                                                                        char fires_when, LOCKMODE lockmode);
-static void ATPrepAddInherit(Relation child_rel);
+static void ATPrepChangeInherit(Relation child_rel);
 static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode);
 static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode);
 static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
@@ -5227,16 +5227,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
                        break;
                case AT_AddInherit:             /* INHERIT */
                        ATSimplePermissions(cmd->subtype, rel,
-                                                               ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
+                                                               ATT_TABLE | ATT_FOREIGN_TABLE);
                        /* This command never recurses */
-                       ATPrepAddInherit(rel);
+                       ATPrepChangeInherit(rel);
                        pass = AT_PASS_MISC;
                        break;
                case AT_DropInherit:    /* NO INHERIT */
                        ATSimplePermissions(cmd->subtype, rel,
-                                                               ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
+                                                               ATT_TABLE | ATT_FOREIGN_TABLE);
                        /* This command never recurses */
-                       /* No command-specific prep needed */
+                       ATPrepChangeInherit(rel);
                        pass = AT_PASS_MISC;
                        break;
                case AT_AlterConstraint:        /* ALTER CONSTRAINT */
@@ -17472,14 +17472,12 @@ ATExecEnableDisableRule(Relation rel, const char *rulename,
 }
 
 /*
- * ALTER TABLE INHERIT
+ * Preparation phase of [NO] INHERIT
  *
- * Add a parent to the child's parents. This verifies that all the columns and
- * check constraints of the parent appear in the child and that they have the
- * same data types and expressions.
+ * Check the relation defined as a child.
  */
 static void
-ATPrepAddInherit(Relation child_rel)
+ATPrepChangeInherit(Relation child_rel)
 {
        if (child_rel->rd_rel->reloftype)
                ereport(ERROR,
@@ -17490,14 +17488,11 @@ ATPrepAddInherit(Relation child_rel)
                ereport(ERROR,
                                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                 errmsg("cannot change inheritance of a partition")));
-
-       if (child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-               ereport(ERROR,
-                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                                errmsg("cannot change inheritance of partitioned table")));
 }
 
 /*
+ * ALTER TABLE INHERIT
+ *
  * Return the address of the new parent relation.
  */
 static ObjectAddress
@@ -17609,6 +17604,9 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode)
  *             Catalog manipulation portion of creating inheritance between a child
  *             table and a parent table.
  *
+ * This verifies that all the columns and check constraints of the parent
+ * appear in the child and that they have the same data types and expressions.
+ *
  * Common to ATExecAddInherit() and ATExecAttachPartition().
  */
 static void
@@ -18068,11 +18066,6 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
        ObjectAddress address;
        Relation        parent_rel;
 
-       if (rel->rd_rel->relispartition)
-               ereport(ERROR,
-                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                                errmsg("cannot change inheritance of a partition")));
-
        /*
         * AccessShareLock on the parent is probably enough, seeing that DROP
         * TABLE doesn't lock parent tables at all.  We need some lock since we'll

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5998c670aa3ae6e698b5ad5cfefc74e3e92e7dcd..ccd79dfecc03cf8311172bf09b1d38a4cc414852 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4007,10 +4007,19 @@ CREATE TABLE nonpartitioned (
        a int,
        b int
 );
-ALTER TABLE partitioned INHERIT nonpartitioned;
-ERROR:  cannot change inheritance of partitioned table
-ALTER TABLE nonpartitioned INHERIT partitioned;
+ALTER TABLE partitioned INHERIT nonpartitioned; -- fail
+ERROR:  ALTER action INHERIT cannot be performed on relation "partitioned"
+DETAIL:  This operation is not supported for partitioned tables.
+ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail
+ERROR:  ALTER action NO INHERIT cannot be performed on relation "partitioned"
+DETAIL:  This operation is not supported for partitioned tables.
+ALTER TABLE nonpartitioned INHERIT partitioned; -- fail
 ERROR:  cannot inherit from partitioned table "partitioned"
+CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100);
+ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail
+ERROR:  cannot change inheritance of a partition
+ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail
+ERROR:  cannot change inheritance of a partition
 -- cannot add NO INHERIT constraint to partitioned tables
 ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT;
 ERROR:  cannot add NO INHERIT constraint to partitioned table "partitioned"

diff --git a/src/test/regress/expected/typed_table.out b/src/test/regress/expected/typed_table.out
index 2badd8e1498a9738d50ac4e5cb92062edecd02fa..c0a06bf91458dff846368eb95f52df1252b3a1fa 100644 (file)
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -44,7 +44,7 @@ CREATE TABLE stuff (id int);
 ALTER TABLE persons INHERIT stuff; -- error
 ERROR:  cannot change inheritance of typed table
 ALTER TABLE persons NO INHERIT stuff; -- error
-ERROR:  relation "stuff" is not a parent of relation "persons"
+ERROR:  cannot change inheritance of typed table
 CREATE TABLE personsx OF person_type (myname WITH OPTIONS NOT NULL); -- error
 ERROR:  column "myname" does not exist
 CREATE TABLE persons2 OF person_type (

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index d6b6381ae5c707a32c2911df658e84ff946c6b9f..f5f13bbd3e7f609c0171a8398eae35425d898b90 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2405,8 +2405,12 @@ CREATE TABLE nonpartitioned (
        a int,
        b int
 );
-ALTER TABLE partitioned INHERIT nonpartitioned;
-ALTER TABLE nonpartitioned INHERIT partitioned;
+ALTER TABLE partitioned INHERIT nonpartitioned; -- fail
+ALTER TABLE partitioned NO INHERIT nonpartitioned; -- fail
+ALTER TABLE nonpartitioned INHERIT partitioned; -- fail
+CREATE TABLE partitioned_p1 PARTITION OF partitioned FOR VALUES FROM (0, 0) TO (10, 100);
+ALTER TABLE partitioned_p1 INHERIT nonpartitioned; -- fail
+ALTER TABLE partitioned_p1 NO INHERIT nonpartitioned; -- fail
 
 -- cannot add NO INHERIT constraint to partitioned tables
 ALTER TABLE partitioned ADD CONSTRAINT chk_a CHECK (a > 0) NO INHERIT;