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