mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix bogus dependency handling for GENERATED expressions.

Browse Source 
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
This commit is contained in:
Tom Lane 2022-03-21 14:58:49 -04:00
parent 17f3bc0928
commit cb02fcb4c9
7 changed files with 113 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/backend/catalog/information_schema.sql
View File

@ -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');

30
src/backend/catalog/pg_attrdef.c
View File

@ -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.

163
src/backend/commands/tablecmds.c
View File

@ -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);
/*
* 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.
*/
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 above changes visible */
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.
* 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.
*/
{
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));
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);
table_close(depRel, RowExclusiveLock);
}
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();
/*

8
src/bin/pg_dump/pg_dump_sort.c
View File

@ -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)
{

2
src/include/catalog/catversion.h
View File

@ -53,6 +53,6 @@
*/
/* yyyymmddN */
#define CATALOG_VERSION_NO 202203171
#define CATALOG_VERSION_NO 202203211
#endif

11
src/test/regress/expected/generated.out
View File

@ -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;

4
src/test/regress/sql/generated.sql
View File

@ -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;