Fix bogus dependency handling for GENERATED expressions.

For GENERATED columns, we record all dependencies of the generation
expression as AUTO dependencies of the column itself.  This means
that the generated column is silently dropped if any dependency
is removed, even if CASCADE wasn't specified.  This is at least
a POLA violation, but I think it's actually based on a misreading
of the standard.  The standard does say that you can't drop a
dependent GENERATED column in RESTRICT mode; but that's buried down
in a subparagraph, on a different page from some pseudocode that
makes it look like an AUTO drop is being suggested.

Change this to be more like the way that we handle regular default
expressions, ie record the dependencies as NORMAL dependencies of
the pg_attrdef entry.  Also, make the pg_attrdef entry's dependency
on the column itself be INTERNAL not AUTO.  That has two effects:

* the column will go away, not just lose its default, if any
dependency of the expression is dropped with CASCADE.  So we
don't need any special mechanism to make that happen.

* it provides an additional cross-check preventing someone from
dropping the default expression without dropping the column.

catversion bump because of change in the contents of pg_depend
(which also requires a change in one information_schema view).

Per bug #17439 from Kevin Humphreys.  Although this is a longstanding
bug, it seems impractical to back-patch because of the need for
catalog contents changes.

Discussion: https://postgr.es/m/17439-7df4421197e928f0@postgresql.org

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index c4ef8e78a5f22ac8c5fb577dcf202ec6be5023b4..18725a02d1fb6ffda3d218033b972a0ff23aac3b 100644 (file)
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -514,16 +514,18 @@ CREATE VIEW column_column_usage AS
            CAST(ad.attname AS sql_identifier) AS dependent_column
 
     FROM pg_namespace n, pg_class c, pg_depend d,
-         pg_attribute ac, pg_attribute ad
+         pg_attribute ac, pg_attribute ad, pg_attrdef atd
 
     WHERE n.oid = c.relnamespace
           AND c.oid = ac.attrelid
           AND c.oid = ad.attrelid
-          AND d.classid = 'pg_catalog.pg_class'::regclass
+          AND ac.attnum <> ad.attnum
+          AND ad.attrelid = atd.adrelid
+          AND ad.attnum = atd.adnum
+          AND d.classid = 'pg_catalog.pg_attrdef'::regclass
           AND d.refclassid = 'pg_catalog.pg_class'::regclass
-          AND d.objid = d.refobjid
-          AND c.oid = d.objid
-          AND d.objsubid = ad.attnum
+          AND d.objid = atd.oid
+          AND d.refobjid = ac.attrelid
           AND d.refobjsubid = ac.attnum
           AND ad.attgenerated <> ''
           AND pg_has_role(c.relowner, 'USAGE');

diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 490a52a086f1ca7e8dd49e6901b144902dac3784..2d63c6e62a3f5671298c3f91060a1ca6f2cc3d59 100644 (file)
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -174,37 +174,23 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
 
    /*
     * Make a dependency so that the pg_attrdef entry goes away if the column
-    * (or whole table) is deleted.
+    * (or whole table) is deleted.  In the case of a generated column, make
+    * it an internal dependency to prevent the default expression from being
+    * deleted separately.
     */
    colobject.classId = RelationRelationId;
    colobject.objectId = RelationGetRelid(rel);
    colobject.objectSubId = attnum;
 
-   recordDependencyOn(&defobject, &colobject, DEPENDENCY_AUTO);
+   recordDependencyOn(&defobject, &colobject,
+                      attgenerated ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO);
 
    /*
     * Record dependencies on objects used in the expression, too.
     */
-   if (attgenerated)
-   {
-       /*
-        * Generated column: Dropping anything that the generation expression
-        * refers to automatically drops the generated column.
-        */
-       recordDependencyOnSingleRelExpr(&colobject, expr, RelationGetRelid(rel),
-                                       DEPENDENCY_AUTO,
-                                       DEPENDENCY_AUTO, false);
-   }
-   else
-   {
-       /*
-        * Normal default: Dropping anything that the default refers to
-        * requires CASCADE and drops the default only.
-        */
-       recordDependencyOnSingleRelExpr(&defobject, expr, RelationGetRelid(rel),
-                                       DEPENDENCY_NORMAL,
-                                       DEPENDENCY_NORMAL, false);
-   }
+   recordDependencyOnSingleRelExpr(&defobject, expr, RelationGetRelid(rel),
+                                   DEPENDENCY_NORMAL,
+                                   DEPENDENCY_NORMAL, false);
 
    /*
     * Post creation hook for attribute defaults.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fc3fc9b3846d2f4a069c1eb0ad35ea8bc6fba138..80faae985e924ba0ae61ef136d424952003995e4 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7899,6 +7899,7 @@ ATExecDropExpression(Relation rel, const char *colName, bool missing_ok, LOCKMOD
    Form_pg_attribute attTup;
    AttrNumber  attnum;
    Relation    attrelation;
+   Oid         attrdefoid;
    ObjectAddress address;
 
    attrelation = table_open(AttributeRelationId, RowExclusiveLock);
@@ -7936,71 +7937,44 @@ ATExecDropExpression(Relation rel, const char *colName, bool missing_ok, LOCKMOD
        }
    }
 
+   /*
+    * Mark the column as no longer generated.  (The atthasdef flag needs to
+    * get cleared too, but RemoveAttrDefault will handle that.)
+    */
    attTup->attgenerated = '\0';
    CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
 
    InvokeObjectPostAlterHook(RelationRelationId,
                              RelationGetRelid(rel),
-                             attTup->attnum);
-   ObjectAddressSubSet(address, RelationRelationId,
-                       RelationGetRelid(rel), attnum);
+                             attnum);
    heap_freetuple(tuple);
 
    table_close(attrelation, RowExclusiveLock);
 
-   CommandCounterIncrement();
-
-   RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, false, false);
-
    /*
-    * Remove all dependencies of this (formerly generated) column on other
-    * columns in the same table.  (See StoreAttrDefault() for which
-    * dependencies are created.)  We don't expect there to be dependencies
-    * between columns of the same table for other reasons, so it's okay to
-    * remove all of them.
+    * Drop the dependency records of the GENERATED expression, in particular
+    * its INTERNAL dependency on the column, which would otherwise cause
+    * dependency.c to refuse to perform the deletion.
     */
-   {
-       Relation    depRel;
-       ScanKeyData key[3];
-       SysScanDesc scan;
-       HeapTuple   tup;
-
-       depRel = table_open(DependRelationId, RowExclusiveLock);
-
-       ScanKeyInit(&key[0],
-                   Anum_pg_depend_classid,
-                   BTEqualStrategyNumber, F_OIDEQ,
-                   ObjectIdGetDatum(RelationRelationId));
-       ScanKeyInit(&key[1],
-                   Anum_pg_depend_objid,
-                   BTEqualStrategyNumber, F_OIDEQ,
-                   ObjectIdGetDatum(RelationGetRelid(rel)));
-       ScanKeyInit(&key[2],
-                   Anum_pg_depend_objsubid,
-                   BTEqualStrategyNumber, F_INT4EQ,
-                   Int32GetDatum(attnum));
+   attrdefoid = GetAttrDefaultOid(RelationGetRelid(rel), attnum);
+   if (!OidIsValid(attrdefoid))
+       elog(ERROR, "could not find attrdef tuple for relation %u attnum %d",
+            RelationGetRelid(rel), attnum);
+   (void) deleteDependencyRecordsFor(AttrDefaultRelationId, attrdefoid, false);
 
-       scan = systable_beginscan(depRel, DependDependerIndexId, true,
-                                 NULL, 3, key);
-
-       while (HeapTupleIsValid(tup = systable_getnext(scan)))
-       {
-           Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
-
-           if (depform->refclassid == RelationRelationId &&
-               depform->refobjid == RelationGetRelid(rel) &&
-               depform->refobjsubid != 0 &&
-               depform->deptype == DEPENDENCY_AUTO)
-           {
-               CatalogTupleDelete(depRel, &tup->t_self);
-           }
-       }
-
-       systable_endscan(scan);
+   /* Make above changes visible */
+   CommandCounterIncrement();
 
-       table_close(depRel, RowExclusiveLock);
-   }
+   /*
+    * Get rid of the GENERATED expression itself.  We use RESTRICT here for
+    * safety, but at present we do not expect anything to depend on the
+    * default.
+    */
+   RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT,
+                     false, false);
 
+   ObjectAddressSubSet(address, RelationRelationId,
+                       RelationGetRelid(rel), attnum);
    return address;
 }
 
@@ -12548,21 +12522,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                         */
                        Assert(foundObject.objectSubId == 0);
                    }
-                   else if (relKind == RELKIND_RELATION &&
-                            foundObject.objectSubId != 0 &&
-                            get_attgenerated(foundObject.objectId, foundObject.objectSubId))
-                   {
-                       /*
-                        * Changing the type of a column that is used by a
-                        * generated column is not allowed by SQL standard. It
-                        * might be doable with some thinking and effort.
-                        */
-                       ereport(ERROR,
-                               (errcode(ERRCODE_SYNTAX_ERROR),
-                                errmsg("cannot alter type of a column used by a generated column"),
-                                errdetail("Column \"%s\" is used by generated column \"%s\".",
-                                          colName, get_attname(foundObject.objectId, foundObject.objectSubId, false))));
-                   }
                    else
                    {
                        /* Not expecting any other direct dependencies... */
@@ -12625,13 +12584,39 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                break;
 
            case OCLASS_DEFAULT:
+               {
+                   ObjectAddress col = GetAttrDefaultColumnAddress(foundObject.objectId);
 
-               /*
-                * Ignore the column's default expression, since we will fix
-                * it below.
-                */
-               Assert(defaultexpr);
-               break;
+                   if (col.objectId == RelationGetRelid(rel) &&
+                       col.objectSubId == attnum)
+                   {
+                       /*
+                        * Ignore the column's own default expression, which
+                        * we will deal with below.
+                        */
+                       Assert(defaultexpr);
+                   }
+                   else
+                   {
+                       /*
+                        * This must be a reference from the expression of a
+                        * generated column elsewhere in the same table.
+                        * Changing the type of a column that is used by a
+                        * generated column is not allowed by SQL standard, so
+                        * just punt for now.  It might be doable with some
+                        * thinking and effort.
+                        */
+                       ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot alter type of a column used by a generated column"),
+                                errdetail("Column \"%s\" is used by generated column \"%s\".",
+                                          colName,
+                                          get_attname(col.objectId,
+                                                      col.objectSubId,
+                                                      false))));
+                   }
+                   break;
+               }
 
            case OCLASS_STATISTIC_EXT:
 
@@ -12694,9 +12679,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 
    /*
     * Now scan for dependencies of this column on other things.  The only
-    * thing we should find is the dependency on the column datatype, which we
-    * want to remove, possibly a collation dependency, and dependencies on
-    * other columns if it is a generated column.
+    * things we should find are the dependency on the column datatype and
+    * possibly a collation dependency.  Those can be removed.
     */
    ScanKeyInit(&key[0],
                Anum_pg_depend_classid,
@@ -12723,18 +12707,13 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
        foundObject.objectId = foundDep->refobjid;
        foundObject.objectSubId = foundDep->refobjsubid;
 
-       if (foundDep->deptype != DEPENDENCY_NORMAL &&
-           foundDep->deptype != DEPENDENCY_AUTO)
+       if (foundDep->deptype != DEPENDENCY_NORMAL)
            elog(ERROR, "found unexpected dependency type '%c'",
                 foundDep->deptype);
        if (!(foundDep->refclassid == TypeRelationId &&
              foundDep->refobjid == attTup->atttypid) &&
            !(foundDep->refclassid == CollationRelationId &&
-             foundDep->refobjid == attTup->attcollation) &&
-           !(foundDep->refclassid == RelationRelationId &&
-             foundDep->refobjid == RelationGetRelid(rel) &&
-             foundDep->refobjsubid != 0)
-           )
+             foundDep->refobjid == attTup->attcollation))
            elog(ERROR, "found unexpected dependency for column: %s",
                 getObjectDescription(&foundObject, false));
 
@@ -12850,7 +12829,25 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
     */
    if (defaultexpr)
    {
-       /* Must make new row visible since it will be updated again */
+       /*
+        * If it's a GENERATED default, drop its dependency records, in
+        * particular its INTERNAL dependency on the column, which would
+        * otherwise cause dependency.c to refuse to perform the deletion.
+        */
+       if (attTup->attgenerated)
+       {
+           Oid         attrdefoid = GetAttrDefaultOid(RelationGetRelid(rel), attnum);
+
+           if (!OidIsValid(attrdefoid))
+               elog(ERROR, "could not find attrdef tuple for relation %u attnum %d",
+                    RelationGetRelid(rel), attnum);
+           (void) deleteDependencyRecordsFor(AttrDefaultRelationId, attrdefoid, false);
+       }
+
+       /*
+        * Make updates-so-far visible, particularly the new pg_attribute row
+        * which will be updated again.
+        */
        CommandCounterIncrement();
 
        /*

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index d979f93b3d62ce76a4e91c1bc4b4d1fc425278ac..1592090839528f76577123183f9a748550be9744 100644 (file)
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -1203,10 +1203,10 @@ repairDependencyLoop(DumpableObject **loop,
     * Loop of table with itself --- just ignore it.
     *
     * (Actually, what this arises from is a dependency of a table column on
-    * another column, which happens with generated columns; or a dependency
-    * of a table column on the whole table, which happens with partitioning.
-    * But we didn't pay attention to sub-object IDs while collecting the
-    * dependency data, so we can't see that here.)
+    * another column, which happened with generated columns before v15; or a
+    * dependency of a table column on the whole table, which happens with
+    * partitioning.  But we didn't pay attention to sub-object IDs while
+    * collecting the dependency data, so we can't see that here.)
     */
    if (nLoop == 1)
    {

diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index dcc0aba82a0394d348b3a824383d0d17fbf4a58b..1383761c1f8ce1069ed5b8d0a74977900bcd3335 100644 (file)
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*                         yyyymmddN */
-#define CATALOG_VERSION_NO 202203171
+#define CATALOG_VERSION_NO 202203211
 
 #endif

diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index cb9373227d454d261e7c3fdfdb355606bb8bc09e..bb4190340e76b52a05c586b003461def60557736 100644 (file)
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -477,7 +477,12 @@ SELECT * FROM gtest_tableoid;
 
 -- drop column behavior
 CREATE TABLE gtest10 (a int PRIMARY KEY, b int, c int GENERATED ALWAYS AS (b * 2) STORED);
-ALTER TABLE gtest10 DROP COLUMN b;
+ALTER TABLE gtest10 DROP COLUMN b;  -- fails
+ERROR:  cannot drop column b of table gtest10 because other objects depend on it
+DETAIL:  column c of table gtest10 depends on column b of table gtest10
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
+ALTER TABLE gtest10 DROP COLUMN b CASCADE;  -- drops c too
+NOTICE:  drop cascades to column c of table gtest10
 \d gtest10
               Table "public.gtest10"
  Column |  Type   | Collation | Nullable | Default 
@@ -519,6 +524,10 @@ SELECT a, c FROM gtest12s;  -- allowed
 (2 rows)
 
 RESET ROLE;
+DROP FUNCTION gf1(int);  -- fail
+ERROR:  cannot drop function gf1(integer) because other objects depend on it
+DETAIL:  column c of table gtest12s depends on function gf1(integer)
+HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 DROP TABLE gtest11s, gtest12s;
 DROP FUNCTION gf1(int);
 DROP USER regress_user11;

diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index b7eb072671e3cbbd1c218b3d68f2389a8b4ef71f..378297e6ea675851f79b263ecc51039544d1e318 100644 (file)
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -231,7 +231,8 @@ SELECT * FROM gtest_tableoid;
 
 -- drop column behavior
 CREATE TABLE gtest10 (a int PRIMARY KEY, b int, c int GENERATED ALWAYS AS (b * 2) STORED);
-ALTER TABLE gtest10 DROP COLUMN b;
+ALTER TABLE gtest10 DROP COLUMN b;  -- fails
+ALTER TABLE gtest10 DROP COLUMN b CASCADE;  -- drops c too
 
 \d gtest10
 
@@ -260,6 +261,7 @@ SELECT gf1(10);  -- not allowed
 SELECT a, c FROM gtest12s;  -- allowed
 RESET ROLE;
 
+DROP FUNCTION gf1(int);  -- fail
 DROP TABLE gtest11s, gtest12s;
 DROP FUNCTION gf1(int);
 DROP USER regress_user11;