Обсуждение: BUG #17439: DROP FUNCTION functionName(); drops associated generated column without using CASCADE
BUG #17439: DROP FUNCTION functionName(); drops associated generated column without using CASCADE
От
PG Bug reporting form
Дата:
The following bug has been logged on the website:
Bug reference: 17439
Logged by: Kevin Humphreys
Email address: kmanh999@gmail.com
PostgreSQL version: 13.3
Operating system: docker linux
Description:
We have the following DDL
create table schemaA.building
(
id integer default
nextval('layer0_data.instance_id_seq'::regclass) not null
primary key
unique,
serial_number text,
name text
not null,
geometry geometry(Geometry, 4326)
constraint geom_check
check (geometrytype(geometry) = ANY (ARRAY ['POLYGON'::text,
'MULTIPOLYGON'::text, 'POINT'::text])),
feature_id integer
unique
references route.feature
on update restrict on delete restrict,
type text
not null
references layer0_enum.building_type
on update restrict on delete restrict,
ownership text
not null
references layer0_enum.building_ownership
on update restrict on delete restrict,
height numeric default 0
not null,
length numeric default 0
not null,
width numeric default 0
not null,
import_info text,
altname text,
iversion text,
area double precision generated always as (map.area(geometry))
stored
);
If I execute `DROP FUNCTION IF EXISTS map.area(geometry)`, it should error
out saying it is depended on by building.area. However, instead it
successfully drops map.area(geometry) and also drops the building.area
column. According to the documentation, RESTRICT is the default so it should
refuse to drop instead of dropping the column unless I explicitly call DROP
using CASCADE.
PG Bug reporting form <noreply@postgresql.org> writes:
> We have the following DDL
> ...
> area double precision generated always as (map.area(geometry)) stored
> If I execute `DROP FUNCTION IF EXISTS map.area(geometry)`, it should error
> out saying it is depended on by building.area. However, instead it
> successfully drops map.area(geometry) and also drops the building.area
> column.
Yeah. I think this might be intentional, but it's surely a POLA
violation. To reproduce:
regression=# create function foo(int) returns int as 'select $1+1' language sql immutable;
CREATE FUNCTION
regression=# create table bar (x int, y int generated always as (foo(x)) stored);
CREATE TABLE
regression=# select pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where objid > ...;
obj | ref | deptype
-----------------------------------------+------------------------------------+---------
function foo(integer) | transform for integer language sql | n
function foo(integer) | schema public | n
type bar | table bar | i
type bar[] | type bar | i
table bar | schema public | n
default value for column y of table bar | column y of table bar | a
column y of table bar | column x of table bar | a
column y of table bar | function foo(integer) | a
(8 rows)
So the dependencies of the generation expression have been attached
to the column itself with 'a' (automatic) deptype, which explains
the behavior. But is that actually sensible? I think 'n' deptype
would provide the semantics that one would expect. Maybe there is
something in the SQL spec motivating references to other columns of
the same table to be handled this way, but surely that's not sane
for references to anything else.
It also seems dubious for the default -> column deptype to be 'a'
rather than 'i' for a GENERATED column. I see that we have some
special-case code that prevents a direct drop:
regression=# alter table bar alter column y drop default;
ERROR: column "y" of relation "bar" is a generated column
HINT: Use ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION instead.
but I don't have a lot of faith that that covers all possible
code paths. An 'i' dependency would make it much harder to
screw this up.
regards, tom lane
I wrote:
> So the dependencies of the generation expression have been attached
> to the column itself with 'a' (automatic) deptype, which explains
> the behavior. But is that actually sensible? I think 'n' deptype
> would provide the semantics that one would expect. Maybe there is
> something in the SQL spec motivating references to other columns of
> the same table to be handled this way, but surely that's not sane
> for references to anything else.
I looked into SQL:2021, and AFAICS the existing behavior is flat wrong,
even for cross references to other table columns. I think you read
11.23 <drop column definition> general rule 3, which seems to say to
unconditionally drop any generated column depending on the target column
... but you missed syntax rule 7f, which says
7) If RESTRICT is specified, then C shall not be referenced in any of the
following:
...
f) The generation expression of any column descriptor.
GR3 would be very strange if read in isolation anyway, because it
says to drop the generated column with CASCADE, which could cause
arbitrary stuff to go away. That is sensible if you know that 7f
prevents us from getting here unless the original drop said CASCADE,
but otherwise it's a pretty astonishing thing.
So it looks to me like the generation expression's dependencies
should be NORMAL not AUTO in all cases. I'm less sure about
whether to mess with any other aspects of the dependency linkages.
That might not be something to fool with in back branches, anyway.
regards, tom lane
I wrote:
> So it looks to me like the generation expression's dependencies
> should be NORMAL not AUTO in all cases. I'm less sure about
> whether to mess with any other aspects of the dependency linkages.
> That might not be something to fool with in back branches, anyway.
Ugh ... this opens a much larger can of worms than I thought.
There are two problems:
1. If these dependencies are NORMAL, then we cannot tell them apart from
the column's other dependencies -- such as the ones on its type and
collation. (The generation expression could easily have dependencies on
types and collations.) I think we really have to switch them so that
the referencing object is the pg_attrdef entry not the column itself,
just as is done for ordinary defaults. That's easy so far as
StoreAttrDefault itself is concerned, but ...
2. ALTER TABLE contains multiple assumptions about the structure of
dependencies for generation expressions, and they'll all be broken
by such a change. There's even a very explicit claim that any such
dependency could only be on another column of the same table :-(.
The regression tests reveal two or three places in tablecmds.c that
need to change, and I'm worried there may be other places that
aren't covered.
So it's looking to me like we probably can't fix this in the back
branches; it'll have to be a HEAD-only change.
regards, tom lane
Thanks for deep-diving into this Tom! I don't have any experience with the internal workings of Postgres but if I am understanding correctly:
- This is definitely a bug and not intended or expected behavior and goes against SQL specifications
- This is a non-trivial fix
- This is a fix that can not be back-ported to Postgres 13?
- This is a fix that can be made to Postgres 14?
Is there any recommendation you would have for mitigation besides not dropping functions that may be used by generated columns?
Thanks,
Kevin Humphreys
On Tue, Mar 15, 2022 at 5:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
> So it looks to me like the generation expression's dependencies
> should be NORMAL not AUTO in all cases. I'm less sure about
> whether to mess with any other aspects of the dependency linkages.
> That might not be something to fool with in back branches, anyway.
Ugh ... this opens a much larger can of worms than I thought.
There are two problems:
1. If these dependencies are NORMAL, then we cannot tell them apart from
the column's other dependencies -- such as the ones on its type and
collation. (The generation expression could easily have dependencies on
types and collations.) I think we really have to switch them so that
the referencing object is the pg_attrdef entry not the column itself,
just as is done for ordinary defaults. That's easy so far as
StoreAttrDefault itself is concerned, but ...
2. ALTER TABLE contains multiple assumptions about the structure of
dependencies for generation expressions, and they'll all be broken
by such a change. There's even a very explicit claim that any such
dependency could only be on another column of the same table :-(.
The regression tests reveal two or three places in tablecmds.c that
need to change, and I'm worried there may be other places that
aren't covered.
So it's looking to me like we probably can't fix this in the back
branches; it'll have to be a HEAD-only change.
regards, tom lane
Kevin Humphreys
Kevin Humphreys <kmanh999@gmail.com> writes:
> Thanks for deep-diving into this Tom! I don't have any experience with the
> internal workings of Postgres but if I am understanding correctly:
> - This is definitely a bug and not intended or expected behavior and goes
> against SQL specifications
Looks like a bug to me. The aspect of this that a drop of a table
column causes silent drop of dependent generated columns is clearly
intentional, but AFAICS that's based on a misreading of the spec.
I suspect that the fact that it carries over to other dependencies
of the generation expression was just failure to consider that case.
> - This is a non-trivial fix
> - This is a fix that can not be back-ported to Postgres 13?
> - This is a fix that can be made to Postgres 14?
Yes, yes, no. I don't think we'd risk trying to change this for
any released branch, because redefining catalog contents in
existing installations carries all sorts of hazards. Given that
it's been wrong since v12 and you're the first to complain,
I judge that fixing it in released branches is not worth taking
such risks for. We should definitely endeavor to get it fixed
for v15 though.
> Is there any recommendation you would have for mitigation besides not
> dropping functions that may be used by generated columns?
I don't see any good user-level workaround :-(.
regards, tom lane
Re: BUG #17439: DROP FUNCTION functionName(); drops associated generated column without using CASCADE
От
"David G. Johnston"
Дата:
On Tuesday, March 15, 2022, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Is there any recommendation you would have for mitigation besides not
> dropping functions that may be used by generated columns?
I don't see any good user-level workaround :-(.
Create a dummy view that simply calls the function with null inputs. That view then will at least enforce the dependency.
David J.
I wrote:
> Ugh ... this opens a much larger can of worms than I thought.
After some fooling around, here's a draft patch for this.
I needed functions to convert between pg_attrdef OIDs and owning
column's table OID + attnum. There was already some ad-hoc code
for that in objectaddress.c, which I extracted into standalone
functions. It seemed cleaner to put those into heap.c (beside
StoreAttrDefault) than keep them in objectaddress.c; but perhaps
someone else will see that differently. I'm about half tempted
to shove StoreAttrDefault, RemoveAttrDefault, and these new
functions into a new file catalog/pg_attrdef.c, just to make heap.c
a bit smaller. But I didn't undertake that here.
Otherwise it seems mostly straightforward, but I remain concerned
that I've missed place(s) that depend on the previous arrangement.
regards, tom lane
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 7e99de88b3..62989dcfea 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2336,37 +2336,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.
@@ -2382,6 +2368,86 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
return attrdefOid;
}
+/*
+ * Get the pg_attrdef OID of the default expression for a column
+ * identified by relation OID and and column number.
+ *
+ * Returns InvalidOid if there is no such pg_attrdef entry.
+ */
+Oid
+GetAttrDefaultOid(Oid relid, AttrNumber attnum)
+{
+ Oid result = InvalidOid;
+ Relation attrdef;
+ ScanKeyData keys[2];
+ SysScanDesc scan;
+ HeapTuple tup;
+
+ attrdef = table_open(AttrDefaultRelationId, AccessShareLock);
+ ScanKeyInit(&keys[0],
+ Anum_pg_attrdef_adrelid,
+ BTEqualStrategyNumber,
+ F_OIDEQ,
+ ObjectIdGetDatum(relid));
+ ScanKeyInit(&keys[1],
+ Anum_pg_attrdef_adnum,
+ BTEqualStrategyNumber,
+ F_INT2EQ,
+ Int16GetDatum(attnum));
+ scan = systable_beginscan(attrdef, AttrDefaultIndexId, true,
+ NULL, 2, keys);
+
+ if (HeapTupleIsValid(tup = systable_getnext(scan)))
+ {
+ Form_pg_attrdef atdform = (Form_pg_attrdef) GETSTRUCT(tup);
+
+ result = atdform->oid;
+ }
+
+ systable_endscan(scan);
+ table_close(attrdef, AccessShareLock);
+
+ return result;
+}
+
+/*
+ * Given a pg_attrdef OID, return the relation OID and column number of
+ * the owning column (represented as an ObjectAddress for convenience).
+ *
+ * Returns InvalidObjectAddress if there is no such pg_attrdef entry.
+ */
+ObjectAddress
+GetAttrDefaultColumnAddress(Oid attrdefoid)
+{
+ ObjectAddress result = InvalidObjectAddress;
+ Relation attrdef;
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+ HeapTuple tup;
+
+ attrdef = table_open(AttrDefaultRelationId, AccessShareLock);
+ ScanKeyInit(&skey[0],
+ Anum_pg_attrdef_oid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(attrdefoid));
+ scan = systable_beginscan(attrdef, AttrDefaultOidIndexId, true,
+ NULL, 1, skey);
+
+ if (HeapTupleIsValid(tup = systable_getnext(scan)))
+ {
+ Form_pg_attrdef atdform = (Form_pg_attrdef) GETSTRUCT(tup);
+
+ result.classId = RelationRelationId;
+ result.objectId = atdform->adrelid;
+ result.objectSubId = atdform->adnum;
+ }
+
+ systable_endscan(scan);
+ table_close(attrdef, AccessShareLock);
+
+ return result;
+}
+
/*
* Store a check-constraint expression for the given relation.
*
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index c4ef8e78a5..18725a02d1 100644
--- 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/objectaddress.c b/src/backend/catalog/objectaddress.c
index f30c742d48..5d94d18d2a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -21,6 +21,7 @@
#include "access/sysattr.h"
#include "access/table.h"
#include "catalog/catalog.h"
+#include "catalog/heap.h"
#include "catalog/objectaddress.h"
#include "catalog/pg_am.h"
#include "catalog/pg_amop.h"
@@ -1578,39 +1579,11 @@ get_object_address_attrdef(ObjectType objtype, List *object,
tupdesc = RelationGetDescr(relation);
- /* Look up attribute number and scan pg_attrdef to find its tuple */
+ /* Look up attribute number and fetch the pg_attrdef OID */
attnum = get_attnum(reloid, attname);
defoid = InvalidOid;
if (attnum != InvalidAttrNumber && tupdesc->constr != NULL)
- {
- Relation attrdef;
- ScanKeyData keys[2];
- SysScanDesc scan;
- HeapTuple tup;
-
- attrdef = relation_open(AttrDefaultRelationId, AccessShareLock);
- ScanKeyInit(&keys[0],
- Anum_pg_attrdef_adrelid,
- BTEqualStrategyNumber,
- F_OIDEQ,
- ObjectIdGetDatum(reloid));
- ScanKeyInit(&keys[1],
- Anum_pg_attrdef_adnum,
- BTEqualStrategyNumber,
- F_INT2EQ,
- Int16GetDatum(attnum));
- scan = systable_beginscan(attrdef, AttrDefaultIndexId, true,
- NULL, 2, keys);
- if (HeapTupleIsValid(tup = systable_getnext(scan)))
- {
- Form_pg_attrdef atdform = (Form_pg_attrdef) GETSTRUCT(tup);
-
- defoid = atdform->oid;
- }
-
- systable_endscan(scan);
- relation_close(attrdef, AccessShareLock);
- }
+ defoid = GetAttrDefaultOid(reloid, attnum);
if (!OidIsValid(defoid))
{
if (!missing_ok)
@@ -3161,48 +3134,21 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
case OCLASS_DEFAULT:
{
- Relation attrdefDesc;
- ScanKeyData skey[1];
- SysScanDesc adscan;
- HeapTuple tup;
- Form_pg_attrdef attrdef;
ObjectAddress colobject;
- attrdefDesc = table_open(AttrDefaultRelationId, AccessShareLock);
+ colobject = GetAttrDefaultColumnAddress(object->objectId);
- ScanKeyInit(&skey[0],
- Anum_pg_attrdef_oid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(object->objectId));
-
- adscan = systable_beginscan(attrdefDesc, AttrDefaultOidIndexId,
- true, NULL, 1, skey);
-
- tup = systable_getnext(adscan);
-
- if (!HeapTupleIsValid(tup))
+ if (!OidIsValid(colobject.objectId))
{
if (!missing_ok)
elog(ERROR, "could not find tuple for attrdef %u",
object->objectId);
-
- systable_endscan(adscan);
- table_close(attrdefDesc, AccessShareLock);
break;
}
- attrdef = (Form_pg_attrdef) GETSTRUCT(tup);
-
- colobject.classId = RelationRelationId;
- colobject.objectId = attrdef->adrelid;
- colobject.objectSubId = attrdef->adnum;
-
/* translator: %s is typically "column %s of table %s" */
appendStringInfo(&buffer, _("default value for %s"),
getObjectDescription(&colobject, false));
-
- systable_endscan(adscan);
- table_close(attrdefDesc, AccessShareLock);
break;
}
@@ -5006,50 +4952,22 @@ getObjectIdentityParts(const ObjectAddress *object,
case OCLASS_DEFAULT:
{
- Relation attrdefDesc;
- ScanKeyData skey[1];
- SysScanDesc adscan;
-
- HeapTuple tup;
- Form_pg_attrdef attrdef;
ObjectAddress colobject;
- attrdefDesc = table_open(AttrDefaultRelationId, AccessShareLock);
-
- ScanKeyInit(&skey[0],
- Anum_pg_attrdef_oid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(object->objectId));
-
- adscan = systable_beginscan(attrdefDesc, AttrDefaultOidIndexId,
- true, NULL, 1, skey);
+ colobject = GetAttrDefaultColumnAddress(object->objectId);
- tup = systable_getnext(adscan);
-
- if (!HeapTupleIsValid(tup))
+ if (!OidIsValid(colobject.objectId))
{
if (!missing_ok)
elog(ERROR, "could not find tuple for attrdef %u",
object->objectId);
-
- systable_endscan(adscan);
- table_close(attrdefDesc, AccessShareLock);
break;
}
- attrdef = (Form_pg_attrdef) GETSTRUCT(tup);
-
- colobject.classId = RelationRelationId;
- colobject.objectId = attrdef->adrelid;
- colobject.objectSubId = attrdef->adnum;
-
appendStringInfo(&buffer, "for %s",
getObjectIdentityParts(&colobject,
objname, objargs,
false));
-
- systable_endscan(adscan);
- table_close(attrdefDesc, AccessShareLock);
break;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dc5872f988..adc80b4a4d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -34,6 +34,7 @@
#include "catalog/objectaccess.h"
#include "catalog/partition.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_attrdef.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_depend.h"
@@ -7873,6 +7874,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);
@@ -7910,71 +7912,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;
}
@@ -12522,21 +12497,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... */
@@ -12599,13 +12559,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:
@@ -12668,9 +12654,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,
@@ -12697,18 +12682,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));
@@ -12824,7 +12804,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 d979f93b3d..1592090839 100644
--- 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/heap.h b/src/include/catalog/heap.h
index c4757bda2d..3e76e9849e 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -121,6 +121,9 @@ extern Oid StoreAttrDefault(Relation rel, AttrNumber attnum,
Node *expr, bool is_internal,
bool add_column_mode);
+extern Oid GetAttrDefaultOid(Oid relid, AttrNumber attnum);
+extern ObjectAddress GetAttrDefaultColumnAddress(Oid attrdefoid);
+
extern Node *cookDefault(ParseState *pstate,
Node *raw_default,
Oid atttypid,
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index cb9373227d..bb4190340e 100644
--- 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 b7eb072671..378297e6ea 100644
--- 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;
Re: BUG #17439: DROP FUNCTION functionName(); drops associated generated column without using CASCADE
От
Peter Eisentraut
Дата:
On 15.03.22 21:06, Tom Lane wrote: > I looked into SQL:2021, and AFAICS the existing behavior is flat wrong, > even for cross references to other table columns. I think you read > 11.23 <drop column definition> general rule 3, which seems to say to > unconditionally drop any generated column depending on the target column > ... but you missed syntax rule 7f, which says > > 7) If RESTRICT is specified, then C shall not be referenced in any of the > following: > ... > f) The generation expression of any column descriptor. > > GR3 would be very strange if read in isolation anyway, because it > says to drop the generated column with CASCADE, which could cause > arbitrary stuff to go away. That is sensible if you know that 7f > prevents us from getting here unless the original drop said CASCADE, > but otherwise it's a pretty astonishing thing. The reported case is a DROP FUNCTION, but looking at <drop routine statement>, it doesn't say anything about what to do with generation expressions. That might be a bug in the standard, too.