Identity columns should own only one sequence
Identity columns don't work if they own more than one sequence.
So if one tries to convert a "serial" column to an identity column,
the following can happen:
test=> CREATE TABLE ser(id serial);
CREATE TABLE
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ERROR: column "id" of relation "ser" already has a default value
Hm, ok, let's drop the column default value.
test=> ALTER TABLE ser ALTER id DROP DEFAULT;
ALTER TABLE
Now it works:
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLE
But not very much:
test=> INSERT INTO ser (id) VALUES (DEFAULT);
ERROR: more than one owned sequence found
I propose that we check if there already is a dependent sequence
before adding an identity column.
The attached patch does that, and also forbids setting the ownership
of a sequence to an identity column.
I think this should be backpatched.
Yours,
Laurenz Albe
Attachments:
0001-Make-sure-identity-columns-own-only-a-single-sequenc.patchtext/x-patch; charset=UTF-8; name=0001-Make-sure-identity-columns-own-only-a-single-sequenc.patchDownload
From ab536da87fa8ffc70469d3dbdaf3e1b84b0ef793 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Sun, 14 Apr 2019 17:37:03 +0200
Subject: [PATCH] Make sure identity columns own only a single sequence
If an identity column owns more than one sequence, inserting the
default value will fail with "more than one owned sequence found".
Consequently, we should prevent this from happening, and disallow
a) turning a column that already owns a sequence into an identity column
b) setting ownership of a sequence to an identity column
---
src/backend/commands/sequence.c | 20 +++++++++++++++++---
src/backend/commands/tablecmds.c | 11 +++++++++++
2 files changed, 28 insertions(+), 3 deletions(-)
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);
--
2.20.1
I wrote:
Identity columns don't work if they own more than one sequence.
[...]
test=> INSERT INTO ser (id) VALUES (DEFAULT);
ERROR: more than one owned sequence foundI propose that we check if there already is a dependent sequence
before adding an identity column.The attached patch does that, and also forbids setting the ownership
of a sequence to an identity column.
Alternatively, maybe getOwnedSequence should only consider sequences
with an "internal" dependency on the column. That would avoid the problem
without forbidding anything, since normal OWNED BY dependencies are "auto".
What do you think?
Yours,
Laurenz Albe
On Sun, 2019-04-14 at 20:15 +0200, I wrote:
I wrote:
Identity columns don't work if they own more than one sequence.
Alternatively, maybe getOwnedSequence should only consider sequences
with an "internal" dependency on the column. That would avoid the problem
without forbidding anything, since normal OWNED BY dependencies are "auto".What do you think?
Here is a patch that illustrates the second approach.
I'll add this thread to the next commitfest.
Yours,
Laurenz Albe
Attachments:
0001-Change-getOwnedSequence-to-only-find-sequences-for-i.patchtext/x-patch; charset=UTF-8; name=0001-Change-getOwnedSequence-to-only-find-sequences-for-i.patchDownload
From 7f7bae5315b7770f1327a80eb192bb098ee9df89 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Wed, 24 Apr 2019 16:46:39 +0200
Subject: [PATCH] Change getOwnedSequence to only find sequences for identity
columns
This makes identity columns work even if there is another sequence
owned by the column (with an auto dependency).
---
src/backend/catalog/pg_depend.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index d63bf5e56d..4d8c333243 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -684,14 +684,18 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
Form_pg_depend deprec = (Form_pg_depend) GETSTRUCT(tup);
/*
- * We assume any auto or internal dependency of a sequence on a column
- * must be what we are looking for. (We need the relkind test because
- * indexes can also have auto dependencies on columns.)
+ * If no "attnum" was given, we are looking for sequences with an
+ * auto or internal dependency.
+ * If "attnum" was given, only look for sequences with an internal
+ * dependency, because that is what we need for identity columns.
+ * (We need the relkind test because indexes can also have auto
+ * dependencies on columns.)
*/
if (deprec->classid == RelationRelationId &&
deprec->objsubid == 0 &&
deprec->refobjsubid != 0 &&
- (deprec->deptype == DEPENDENCY_AUTO || deprec->deptype == DEPENDENCY_INTERNAL) &&
+ ((!attnum && deprec->deptype == DEPENDENCY_AUTO) ||
+ deprec->deptype == DEPENDENCY_INTERNAL) &&
get_rel_relkind(deprec->objid) == RELKIND_SEQUENCE)
{
result = lappend_oid(result, deprec->objid);
--
2.20.1
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?
--
Michael
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 foundYes 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
Attachments:
0001-Fix-interaction-of-owned-sequences-and-identity-colu.patchtext/x-patch; charset=UTF-8; name=0001-Fix-interaction-of-owned-sequences-and-identity-colu.patchDownload
From 36bcd47cf0aaf88cd6ca84ad68b246063b838488 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@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
On 2019-04-14 17:51, Laurenz Albe wrote:
Identity columns don't work if they own more than one sequence.
Well, they shouldn't, because then how do they know which sequence they
should use?
So if one tries to convert a "serial" column to an identity column,
the following can happen:test=> CREATE TABLE ser(id serial);
CREATE TABLE
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ERROR: column "id" of relation "ser" already has a default valueHm, ok, let's drop the column default value.
test=> ALTER TABLE ser ALTER id DROP DEFAULT;
ALTER TABLENow it works:
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLEBut not very much:
test=> INSERT INTO ser (id) VALUES (DEFAULT);
ERROR: more than one owned sequence found
You also need to run
ALTER SEQUENCE ser_id_seq OWNED BY NONE;
because dropping the default doesn't release the linkage of the sequence
with the table. These are just weird artifacts of how serial is
implemented, but that's why identity columns were added to improve
things. I don't think we need to make things more complicated here.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 2019-04-26 at 15:23 +0200, Peter Eisentraut wrote:
So if one tries to convert a "serial" column to an identity column,
the following can happen:test=> CREATE TABLE ser(id serial);
CREATE TABLE
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ERROR: column "id" of relation "ser" already has a default valueHm, ok, let's drop the column default value.
test=> ALTER TABLE ser ALTER id DROP DEFAULT;
ALTER TABLENow it works:
test=> ALTER TABLE ser ALTER id ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLEBut not very much:
test=> INSERT INTO ser (id) VALUES (DEFAULT);
ERROR: more than one owned sequence foundYou also need to run
ALTER SEQUENCE ser_id_seq OWNED BY NONE;
because dropping the default doesn't release the linkage of the sequence
with the table. These are just weird artifacts of how serial is
implemented, but that's why identity columns were added to improve
things. I don't think we need to make things more complicated here.
What do you think of the patch I just posted on this thread to
remove ownership automatically when the default is dropped, as Michael
suggested? I think that would make things much more intuitive from
the user's perspective.
Correct me if I am wrong, but the sequence behind identity columns
should be an implementation detail that the user doesn't have to know about.
So the error message about "owned sequences" is likely to confuse users.
I have had a report by a confused user, so I think the problem is real.
Yours,
Laurenz Albe
On 2019-Apr-26, Laurenz Albe wrote:
What do you think of the patch I just posted on this thread to
remove ownership automatically when the default is dropped, as Michael
suggested? I think that would make things much more intuitive from
the user's perspective.
I think a better overall fix is that that when creating the generated
column (or altering a column to make it generated) we should look for
existing an existing sequence and take ownership of that (update
pg_depend records), before deciding to create a new sequence.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Apr 26, 2019 at 11:55:34AM -0400, Alvaro Herrera wrote:
On 2019-Apr-26, Laurenz Albe wrote:
I think a better overall fix is that that when creating the generated
column (or altering a column to make it generated) we should look for
existing an existing sequence and take ownership of that (update
pg_depend records), before deciding to create a new sequence.
What that be actually right? The current value of the sequence would
be the one from the previous use, and max/min values may not be the
defaults associated with identity columns, which is ranged in
[-2^31,2^31-1] by default, but the sequence you may attempt (or not)
to attach to could have completely different properties. It seems to
me that it is much better to start afresh and not enforce the sequence
into something that the user may perhaps not want to use.
My 2c.
--
Michael
On 2019-04-26 15:37, Laurenz Albe wrote:
What do you think of the patch I just posted on this thread to
remove ownership automatically when the default is dropped, as Michael
suggested? I think that would make things much more intuitive from
the user's perspective.
I think that adds more nonstandard behavior on top of an already
confusing and obsolescent feature, so I can't get too excited about it.
A more forward-looking fix would be your other idea of having
getOwnedSequence() only deal with identity sequences (not serial
sequences). See attached patch for a draft.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Untangle-some-stuff-about-identity-sequences.patchtext/plain; charset=UTF-8; name=0001-Untangle-some-stuff-about-identity-sequences.patch; x-mac-creator=0; x-mac-type=0Download
From b0a9c3402847f846d6d133cb9eced56ea9634a30 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 27 Apr 2019 14:12:03 +0200
Subject: [PATCH] Untangle some stuff about identity sequences
XXX draft XXX
---
src/backend/catalog/pg_depend.c | 26 +++++++++++++++++++-------
src/backend/commands/tablecmds.c | 2 +-
src/backend/parser/parse_utilcmd.c | 12 +++++-------
src/backend/rewrite/rewriteHandler.c | 2 +-
src/include/catalog/dependency.h | 2 +-
5 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f7caedcc02..63c94cfbe6 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -707,8 +707,8 @@ sequenceIsOwned(Oid seqId, char deptype, Oid *tableId, int32 *colId)
* Collect a list of OIDs of all sequences owned by the specified relation,
* and column if specified.
*/
-List *
-getOwnedSequences(Oid relid, AttrNumber attnum)
+static List *
+getOwnedSequences_internal(Oid relid, AttrNumber attnum, char deptype)
{
List *result = NIL;
Relation depRel;
@@ -750,7 +750,8 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
(deprec->deptype == DEPENDENCY_AUTO || deprec->deptype == DEPENDENCY_INTERNAL) &&
get_rel_relkind(deprec->objid) == RELKIND_SEQUENCE)
{
- result = lappend_oid(result, deprec->objid);
+ if (!deptype || deprec->deptype == deptype)
+ result = lappend_oid(result, deprec->objid);
}
}
@@ -761,18 +762,29 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
return result;
}
+List *
+getOwnedSequences(Oid relid, AttrNumber attnum)
+{
+ return getOwnedSequences_internal(relid, attnum, 0);
+}
+
/*
- * Get owned sequence, error if not exactly one.
+ * Get owned identity sequence, error if not exactly one.
*/
Oid
-getOwnedSequence(Oid relid, AttrNumber attnum)
+getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
{
- List *seqlist = getOwnedSequences(relid, attnum);
+ List *seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL);
if (list_length(seqlist) > 1)
elog(ERROR, "more than one owned sequence found");
else if (list_length(seqlist) < 1)
- elog(ERROR, "no owned sequence found");
+ {
+ if (missing_ok)
+ return InvalidOid;
+ else
+ elog(ERROR, "no owned sequence found");
+ }
return linitial_oid(seqlist);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 14fcad9034..53dc72f7d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6607,7 +6607,7 @@ ATExecDropIdentity(Relation rel, const char *colName, bool missing_ok, LOCKMODE
table_close(attrelation, RowExclusiveLock);
/* drop the internal sequence */
- seqid = getOwnedSequence(RelationGetRelid(rel), attnum);
+ seqid = getIdentitySequence(RelationGetRelid(rel), attnum, false);
deleteDependencyRecordsForClass(RelationRelationId, seqid,
RelationRelationId, DEPENDENCY_INTERNAL);
CommandCounterIncrement();
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 4564c0ae81..a3c5b005d5 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1083,7 +1083,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
* find sequence owned by old column; extract sequence parameters;
* build new create sequence command
*/
- seq_relid = getOwnedSequence(RelationGetRelid(relation), attribute->attnum);
+ seq_relid = getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false);
seq_options = sequence_options(seq_relid);
generateSerialExtraStmts(cxt, def,
InvalidOid, seq_options, true,
@@ -3165,7 +3165,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
if (attnum != InvalidAttrNumber &&
TupleDescAttr(tupdesc, attnum - 1)->attidentity)
{
- Oid seq_relid = getOwnedSequence(relid, attnum);
+ Oid seq_relid = getIdentitySequence(relid, attnum, false);
Oid typeOid = typenameTypeId(pstate, def->typeName);
AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt);
@@ -3216,7 +3216,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
ListCell *lc;
List *newseqopts = NIL;
List *newdef = NIL;
- List *seqlist;
AttrNumber attnum;
/*
@@ -3237,14 +3236,13 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
if (attnum)
{
- seqlist = getOwnedSequences(relid, attnum);
- if (seqlist)
+ Oid seq_relid = getIdentitySequence(relid, attnum, true);
+
+ if (seq_relid)
{
AlterSeqStmt *seqstmt;
- Oid seq_relid;
seqstmt = makeNode(AlterSeqStmt);
- seq_relid = linitial_oid(seqlist);
seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
get_rel_name(seq_relid), -1);
seqstmt->options = newseqopts;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 39080776b0..fce361fc6a 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1130,7 +1130,7 @@ build_column_default(Relation rel, int attrno)
{
NextValueExpr *nve = makeNode(NextValueExpr);
- nve->seqid = getOwnedSequence(RelationGetRelid(rel), attrno);
+ nve->seqid = getIdentitySequence(RelationGetRelid(rel), attrno, false);
nve->typeId = att_tup->atttypid;
return (Node *) nve;
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 57545b70d8..495a591934 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -209,7 +209,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 Oid getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok);
extern Oid get_constraint_index(Oid constraintId);
--
2.21.0
On Sat, 2019-04-27 at 14:16 +0200, Peter Eisentraut wrote:
On 2019-04-26 15:37, Laurenz Albe wrote:
What do you think of the patch I just posted on this thread to
remove ownership automatically when the default is dropped, as Michael
suggested? I think that would make things much more intuitive from
the user's perspective.I think that adds more nonstandard behavior on top of an already
confusing and obsolescent feature, so I can't get too excited about it.A more forward-looking fix would be your other idea of having
getOwnedSequence() only deal with identity sequences (not serial
sequences). See attached patch for a draft.
That looks good to me.
I agree that slapping on black magic that appropriates a pre-existing
owned sequence seems out of proportion.
I still think thatthat there is merit to Michael's idea of removing
sequence "ownership" (which is just a dependency) when the DEFAULT
on the column is dropped, but this approach is possibly cleaner.
Yours,
Laurenz Albe
On 2019-04-29 18:28, Laurenz Albe wrote:
I still think thatthat there is merit to Michael's idea of removing
sequence "ownership" (which is just a dependency) when the DEFAULT
on the column is dropped, but this approach is possibly cleaner.
I think the proper way to address this would be to create some kind of
dependency between the sequence and the default.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
On 2019-04-29 18:28, Laurenz Albe wrote:
I still think thatthat there is merit to Michael's idea of removing
sequence "ownership" (which is just a dependency) when the DEFAULT
on the column is dropped, but this approach is possibly cleaner.I think the proper way to address this would be to create some kind of
dependency between the sequence and the default.
That is certainly true. But that's hard to retrofit into existing databases,
so it would probably be a modification that is not backpatchable.
Yours,
Laurenz Albe
On Fri, May 03, 2019 at 08:14:35AM +0200, Laurenz Albe wrote:
On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
I think the proper way to address this would be to create some kind of
dependency between the sequence and the default.That is certainly true. But that's hard to retrofit into existing databases,
so it would probably be a modification that is not backpatchable.
And this is basically already the dependency which exists between the
sequence and the relation created with the serial column. So what's
the advantage of adding more dependencies if we already have what we
need? I still think that we should be more careful to drop the
dependency between the sequence and the relation's column if dropping
the default using it. If a DDL defines first a sequence, and then a
default expression using nextval() on a column, then no serial-related
dependency exist.
--
Michael
On Tue, 2019-05-07 at 13:06 +0900, Michael Paquier wrote:
On Fri, May 03, 2019 at 08:14:35AM +0200, Laurenz Albe wrote:
On Thu, 2019-05-02 at 22:43 +0200, Peter Eisentraut wrote:
I think the proper way to address this would be to create some kind of
dependency between the sequence and the default.That is certainly true. But that's hard to retrofit into existing databases,
so it would probably be a modification that is not backpatchable.And this is basically already the dependency which exists between the
sequence and the relation created with the serial column. So what's
the advantage of adding more dependencies if we already have what we
need? I still think that we should be more careful to drop the
dependency between the sequence and the relation's column if dropping
the default using it. If a DDL defines first a sequence, and then a
default expression using nextval() on a column, then no serial-related
I believe we should have both:
- Identity columns should only use sequences with an INTERNAL dependency,
as in Peter's patch.
- When a column default is dropped, remove all dependencies between the
column and sequences.
In the spirit of moving this along, I have attached a patch which is
Peter's patch from above with a regression test.
Yours,
Laurenz Albe
Attachments:
0002-Untangle-some-stuff-about-identity-sequences.patchtext/x-patch; charset=UTF-8; name=0002-Untangle-some-stuff-about-identity-sequences.patchDownload
From 4280a5251320d2051ada7aa1888ba20a610efa94 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Wed, 8 May 2019 16:42:10 +0200
Subject: [PATCH] XXX Draft with regression tests XXX
---
src/backend/catalog/pg_depend.c | 26 +++++++++++++++++++-------
src/backend/commands/tablecmds.c | 2 +-
src/backend/parser/parse_utilcmd.c | 12 +++++-------
src/backend/rewrite/rewriteHandler.c | 2 +-
src/include/catalog/dependency.h | 2 +-
src/test/regress/expected/identity.out | 5 +++++
src/test/regress/sql/identity.sql | 7 +++++++
7 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f7caedcc02..63c94cfbe6 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -707,8 +707,8 @@ sequenceIsOwned(Oid seqId, char deptype, Oid *tableId, int32 *colId)
* Collect a list of OIDs of all sequences owned by the specified relation,
* and column if specified.
*/
-List *
-getOwnedSequences(Oid relid, AttrNumber attnum)
+static List *
+getOwnedSequences_internal(Oid relid, AttrNumber attnum, char deptype)
{
List *result = NIL;
Relation depRel;
@@ -750,7 +750,8 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
(deprec->deptype == DEPENDENCY_AUTO || deprec->deptype == DEPENDENCY_INTERNAL) &&
get_rel_relkind(deprec->objid) == RELKIND_SEQUENCE)
{
- result = lappend_oid(result, deprec->objid);
+ if (!deptype || deprec->deptype == deptype)
+ result = lappend_oid(result, deprec->objid);
}
}
@@ -761,18 +762,29 @@ getOwnedSequences(Oid relid, AttrNumber attnum)
return result;
}
+List *
+getOwnedSequences(Oid relid, AttrNumber attnum)
+{
+ return getOwnedSequences_internal(relid, attnum, 0);
+}
+
/*
- * Get owned sequence, error if not exactly one.
+ * Get owned identity sequence, error if not exactly one.
*/
Oid
-getOwnedSequence(Oid relid, AttrNumber attnum)
+getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok)
{
- List *seqlist = getOwnedSequences(relid, attnum);
+ List *seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL);
if (list_length(seqlist) > 1)
elog(ERROR, "more than one owned sequence found");
else if (list_length(seqlist) < 1)
- elog(ERROR, "no owned sequence found");
+ {
+ if (missing_ok)
+ return InvalidOid;
+ else
+ elog(ERROR, "no owned sequence found");
+ }
return linitial_oid(seqlist);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e4743d110..8240fec578 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6607,7 +6607,7 @@ ATExecDropIdentity(Relation rel, const char *colName, bool missing_ok, LOCKMODE
table_close(attrelation, RowExclusiveLock);
/* drop the internal sequence */
- seqid = getOwnedSequence(RelationGetRelid(rel), attnum);
+ seqid = getIdentitySequence(RelationGetRelid(rel), attnum, false);
deleteDependencyRecordsForClass(RelationRelationId, seqid,
RelationRelationId, DEPENDENCY_INTERNAL);
CommandCounterIncrement();
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 4564c0ae81..a3c5b005d5 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1083,7 +1083,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
* find sequence owned by old column; extract sequence parameters;
* build new create sequence command
*/
- seq_relid = getOwnedSequence(RelationGetRelid(relation), attribute->attnum);
+ seq_relid = getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false);
seq_options = sequence_options(seq_relid);
generateSerialExtraStmts(cxt, def,
InvalidOid, seq_options, true,
@@ -3165,7 +3165,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
if (attnum != InvalidAttrNumber &&
TupleDescAttr(tupdesc, attnum - 1)->attidentity)
{
- Oid seq_relid = getOwnedSequence(relid, attnum);
+ Oid seq_relid = getIdentitySequence(relid, attnum, false);
Oid typeOid = typenameTypeId(pstate, def->typeName);
AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt);
@@ -3216,7 +3216,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
ListCell *lc;
List *newseqopts = NIL;
List *newdef = NIL;
- List *seqlist;
AttrNumber attnum;
/*
@@ -3237,14 +3236,13 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
if (attnum)
{
- seqlist = getOwnedSequences(relid, attnum);
- if (seqlist)
+ Oid seq_relid = getIdentitySequence(relid, attnum, true);
+
+ if (seq_relid)
{
AlterSeqStmt *seqstmt;
- Oid seq_relid;
seqstmt = makeNode(AlterSeqStmt);
- seq_relid = linitial_oid(seqlist);
seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
get_rel_name(seq_relid), -1);
seqstmt->options = newseqopts;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 39080776b0..fce361fc6a 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1130,7 +1130,7 @@ build_column_default(Relation rel, int attrno)
{
NextValueExpr *nve = makeNode(NextValueExpr);
- nve->seqid = getOwnedSequence(RelationGetRelid(rel), attrno);
+ nve->seqid = getIdentitySequence(RelationGetRelid(rel), attrno, false);
nve->typeId = att_tup->atttypid;
return (Node *) nve;
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 57545b70d8..495a591934 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -209,7 +209,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 Oid getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok);
extern Oid get_constraint_index(Oid constraintId);
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 2286519b0c..88ed93b309 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -399,3 +399,8 @@ 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 itest15(id serial);
+ALTER TABLE itest15 ALTER id DROP DEFAULT;
+ALTER TABLE itest15 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY;
+INSERT INTO itest15 (id) VALUES (DEFAULT);
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 8dcfdf3dc1..af8610fd0b 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -254,3 +254,10 @@ 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 itest15(id serial);
+ALTER TABLE itest15 ALTER id DROP DEFAULT;
+ALTER TABLE itest15 ALTER id ADD GENERATED BY DEFAULT AS IDENTITY;
+INSERT INTO itest15 (id) VALUES (DEFAULT);
--
2.20.1
On 2019-05-08 16:49, Laurenz Albe wrote:
I believe we should have both:
- Identity columns should only use sequences with an INTERNAL dependency,
as in Peter's patch.
I have committed this.
- When a column default is dropped, remove all dependencies between the
column and sequences.
There is no proposed patch for this, AFAICT.
So I have closed this commit fest item for now.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote:
On 2019-05-08 16:49, Laurenz Albe wrote:
I believe we should have both:
- Identity columns should only use sequences with an INTERNAL dependency,
as in Peter's patch.I have committed this.
Thanks!
- When a column default is dropped, remove all dependencies between the
column and sequences.There is no proposed patch for this, AFAICT.
There was one in
/messages/by-id/3916586ef7f33948235fe60f54a3750046f5d940.camel@cybertec.at
So I have closed this commit fest item for now.
That's fine with me.
Yours,
Laurenz Albe
Peter Eisentraut wrote:
On 2019-05-08 16:49, Laurenz Albe wrote:
I believe we should have both:
- Identity columns should only use sequences with an INTERNAL dependency,
as in Peter's patch.I have committed this.
Since this is a bug fix, shouldn't it be backpatched?
Yours,
Laurenz Albe
On 2019-08-05 13:30, Laurenz Albe wrote:
Peter Eisentraut wrote:
On 2019-05-08 16:49, Laurenz Albe wrote:
I believe we should have both:
- Identity columns should only use sequences with an INTERNAL dependency,
as in Peter's patch.I have committed this.
Since this is a bug fix, shouldn't it be backpatched?
In cases where the workaround is "don't do that then", I'm inclined to
leave it alone.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services