On Thu, 2019-04-25 at 09:55 +0900, Michael Paquier wrote:
> On Sun, Apr 14, 2019 at 05:51:47PM +0200, Laurenz Albe wrote:
> > test=> INSERT INTO ser (id) VALUES (DEFAULT);
> > ERROR: more than one owned sequence found
>
> Yes this should never be user-triggerable, so it seems that we need to
> fix and back-patch something if possible.
>
> > I propose that we check if there already is a dependent sequence
> > before adding an identity column.
>
> That looks awkward. Souldn't we make sure that when dropping the
> default associated with a serial column then the dependency between
> the column and the sequence is removed instead? This implies more
> complication in ATExecColumnDefault().
>
> > The attached patch does that, and also forbids setting the ownership
> > of a sequence to an identity column.
> >
> > I think this should be backpatched.
>
> Could you add some test cases with what you think is adapted?
You are right! Dropping the dependency with the DEFAULT is the
cleanest approach.
I have left the checks to prevent double sequence ownership from
happening.
I added regression tests covering all added code.
Yours,
Laurenz Albe
From 36bcd47cf0aaf88cd6ca84ad68b246063b838488 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.a...@cybertec.at>
Date: Fri, 26 Apr 2019 12:39:16 +0200
Subject: [PATCH] Fix interaction of owned sequences and identity columns
If an identity column owned more that one sequence, inserting the default
value would fail with "ERROR: more than one owned sequence found".
This was particularly likely to happen in attempts to convert a "serial"
column to an identity column by first dropping the column default,
because doing that didn't remove the sequence ownership. Not cool.
Hence we should make sure that this does not happen:
- remove all sequence ownerships from pg_depend if the column default
value is dropped
- forbid turning a column that owns a sequence into an identity column
- forbid that identity columns become owners of a second sequence
---
src/backend/catalog/heap.c | 10 ++++-
src/backend/catalog/pg_depend.c | 56 ++++++++++++++++++++++++++
src/backend/commands/sequence.c | 20 +++++++--
src/backend/commands/tablecmds.c | 11 +++++
src/include/catalog/dependency.h | 1 +
src/test/regress/expected/identity.out | 22 ++++++++++
src/test/regress/sql/identity.sql | 18 +++++++++
7 files changed, 134 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 6b77eff0af..26aadc8c8d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1723,6 +1723,9 @@ RemoveAttrDefault(Oid relid, AttrNumber attnum,
object.objectId = attrtuple->oid;
object.objectSubId = 0;
+ /* renounce ownership of all sequences */
+ disownSequences(relid, attnum);
+
performDeletion(&object, behavior,
internal ? PERFORM_DELETION_INTERNAL : 0);
@@ -1778,7 +1781,12 @@ RemoveAttrDefaultById(Oid attrdefId)
/* Get an exclusive lock on the relation owning the attribute */
myrel = relation_open(myrelid, AccessExclusiveLock);
- /* Now we can delete the pg_attrdef row */
+ /*
+ * Now we can delete the pg_attrdef row.
+ * Note that we don't have to explicitly disown any sequences OWNED BY
+ * the column here: This function is only called if the column is
+ * dropped, and that will remove the sequence and the dependency anyway.
+ */
CatalogTupleDelete(attrdef_rel, &tuple->t_self);
systable_endscan(scan);
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index d63bf5e56d..7cb9ac3ea8 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -721,6 +721,62 @@ getOwnedSequence(Oid relid, AttrNumber attnum)
return linitial_oid(seqlist);
}
+/*
+ * Remove dependencies between an attribute and sequences OWNED BY it.
+ * Returns the number of disowned sequences.
+ */
+void
+disownSequences(Oid relid, AttrNumber attnum)
+{
+ Relation depRel;
+ ScanKeyData key[3];
+ SysScanDesc scan;
+ HeapTuple tup;
+
+ if (attnum <= 0)
+ elog(ERROR, "cannot remove sequence ownership for system columns");
+
+ depRel = table_open(DependRelationId, RowExclusiveLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_depend_refclassid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(RelationRelationId));
+ ScanKeyInit(&key[1],
+ Anum_pg_depend_refobjid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(relid));
+ ScanKeyInit(&key[2],
+ Anum_pg_depend_refobjsubid,
+ BTEqualStrategyNumber, F_INT4EQ,
+ Int32GetDatum(attnum));
+
+ scan = systable_beginscan(depRel, DependReferenceIndexId, true,
+ NULL, 3, key);
+
+ while (HeapTupleIsValid(tup = systable_getnext(scan)))
+ {
+ Form_pg_depend deprec = (Form_pg_depend) GETSTRUCT(tup);
+
+ /*
+ * We assume any auto dependency of a sequence
+ * must be what we are looking for. (We need the relkind test because
+ * indexes can also have auto dependencies on columns.)
+ */
+ if (deprec->classid == RelationRelationId &&
+ deprec->objsubid == 0 &&
+ deprec->deptype == DEPENDENCY_AUTO &&
+ get_rel_relkind(deprec->objid) == RELKIND_SEQUENCE)
+ {
+ CatalogTupleDelete(depRel, &tup->t_self);
+ }
+ }
+
+ systable_endscan(scan);
+
+ table_close(depRel, RowExclusiveLock);
+}
+
/*
* get_constraint_index
* Given the OID of a unique or primary-key constraint, return the
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index e9add1b987..dc3bff5fdf 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1644,6 +1644,7 @@ process_owned_by(Relation seqrel, List *owned_by, bool for_identity)
int nnames;
Relation tablerel;
AttrNumber attnum;
+ HeapTuple heaptup;
deptype = for_identity ? DEPENDENCY_INTERNAL : DEPENDENCY_AUTO;
@@ -1694,9 +1695,22 @@ process_owned_by(Relation seqrel, List *owned_by, bool for_identity)
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("sequence must be in same schema as table it is linked to")));
- /* Now, fetch the attribute number from the system cache */
- attnum = get_attnum(RelationGetRelid(tablerel), attrname);
- if (attnum == InvalidAttrNumber)
+ /* Now, fetch the attribute from the system cache */
+ heaptup = SearchSysCacheAttName(RelationGetRelid(tablerel), attrname);
+ if (HeapTupleIsValid(heaptup))
+ {
+ Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(heaptup);
+ bool is_identity = att_tup->attidentity;
+
+ attnum = att_tup->attnum;
+ ReleaseSysCache(heaptup);
+ if (is_identity && !for_identity)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("column \"%s\" of relation \"%s\" is an identity column",
+ attrname, RelationGetRelationName(tablerel))));
+ }
+ else
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d48a947f7c..9ea9dae2fb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6378,6 +6378,17 @@ ATExecAddIdentity(Relation rel, const char *colName,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("column \"%s\" of relation \"%s\" already has a default value",
colName, RelationGetRelationName(rel))));
+ /*
+ * Make sure that the column does not already own a sequence.
+ * Otherwise, inserting a default value would fail, since it is not
+ * clear which sequence should be used.
+ */
+ if (getOwnedSequences(RelationGetRelid(rel), attnum) != NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("column \"%s\" of relation \"%s\" already owns a sequence",
+ colName, RelationGetRelationName(rel)),
+ errhint("Drop the dependent sequence before making the column an identity column.")));
attTup->attidentity = cdef->identity;
CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 4f9dde9df9..0fe8480c69 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -207,6 +207,7 @@ extern Oid getExtensionOfObject(Oid classId, Oid objectId);
extern bool sequenceIsOwned(Oid seqId, char deptype, Oid *tableId, int32 *colId);
extern List *getOwnedSequences(Oid relid, AttrNumber attnum);
extern Oid getOwnedSequence(Oid relid, AttrNumber attnum);
+extern void disownSequences(Oid relid, AttrNumber attnum);
extern Oid get_constraint_index(Oid constraintId);
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index f8f3ae8d11..85dabf5d12 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -387,3 +387,25 @@ CREATE TABLE itest_child PARTITION OF itest_parent (
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
ERROR: identity columns are not supported on partitions
DROP TABLE itest_parent;
+-- test interaction with owned sequences and column defaults
+CREATE TABLE itest14(id serial);
+ALTER TABLE itest14 ALTER id DROP DEFAULT; -- should remove the dependency
+SELECT count(*) FROM pg_catalog.pg_depend
+WHERE refclassid = 'pg_class'::regclass
+ AND refobjid = 'itest14'::regclass
+ AND refobjsubid = 1 AND deptype = 'a';
+ count
+-------
+ 0
+(1 row)
+
+DROP SEQUENCE itest14_id_seq;
+CREATE SEQUENCE itest14_silly_seq OWNED BY itest14.id;
+ALTER TABLE itest14 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY; -- error
+ERROR: column "id" of relation "itest14" already owns a sequence
+HINT: Drop the dependent sequence before making the column an identity column.
+DROP SEQUENCE itest14_silly_seq;
+ALTER TABLE itest14 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY;
+CREATE SEQUENCE itest14_id_bad_seq OWNED BY itest14.id; -- error
+ERROR: column "id" of relation "itest14" is an identity column
+DROP TABLE itest14;
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 43c2a59d02..b9b9e9f477 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -248,3 +248,21 @@ CREATE TABLE itest_child PARTITION OF itest_parent (
f3 WITH OPTIONS GENERATED ALWAYS AS IDENTITY
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); -- error
DROP TABLE itest_parent;
+
+-- test interaction with owned sequences and column defaults
+
+CREATE TABLE itest14(id serial);
+ALTER TABLE itest14 ALTER id DROP DEFAULT; -- should remove the dependency
+SELECT count(*) FROM pg_catalog.pg_depend
+WHERE refclassid = 'pg_class'::regclass
+ AND refobjid = 'itest14'::regclass
+ AND refobjsubid = 1 AND deptype = 'a';
+DROP SEQUENCE itest14_id_seq;
+
+CREATE SEQUENCE itest14_silly_seq OWNED BY itest14.id;
+ALTER TABLE itest14 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY; -- error
+DROP SEQUENCE itest14_silly_seq;
+ALTER TABLE itest14 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY;
+
+CREATE SEQUENCE itest14_id_bad_seq OWNED BY itest14.id; -- error
+DROP TABLE itest14;
--
2.20.1
