Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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 https://www.postgresql.org/message-id/3916586ef7f33948235fe60f54a3750046f5d940.camel%40cybertec.at > So I have closed this commit fest item for now. That's fine with me. Yours, Laurenz Albe
Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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
From 4280a5251320d2051ada7aa1888ba20a610efa94 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
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 =
Re: Identity columns should own only one sequence
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 signature.asc Description: PGP signature
Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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
Re: Identity columns should own only one sequence
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
From b0a9c3402847f846d6d133cb9eced56ea9634a30 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
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;
RelationdepRel;
@@ -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,
Re: Identity columns should own only one sequence
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 signature.asc Description: PGP signature
Re: Identity columns should own only one sequence
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Identity columns should own only one sequence
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 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 > > 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. 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
Re: Identity columns should own only one sequence
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 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 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
Re: Identity columns should own only one sequence
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
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(, 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, >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([0],
+Anum_pg_depend_refclassid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(RelationRelationId));
+ ScanKeyInit([1],
+Anum_pg_depend_refobjid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(relid));
+ ScanKeyInit([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
Re: Identity columns should own only one sequence
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 signature.asc Description: PGP signature
Re: Identity columns should own only one sequence
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
From 7f7bae5315b7770f1327a80eb192bb098ee9df89 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
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
Re: Identity columns should own only one sequence
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 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. 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
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
From ab536da87fa8ffc70469d3dbdaf3e1b84b0ef793 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
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, >t_self, tuple);
--
2.20.1
