On Sun, Apr 28, 2024 at 12:29 PM Alexander Lakhin <exclus...@gmail.com> wrote:
> 27.04.2024 18:00, Alexander Lakhin wrote: > > > > Please look also at another script, which produces the same error: > > I've discovered yet another problematic case: > CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text) > PARTITION BY LIST (a); > CREATE TABLE tbl2 (b text, a int NOT NULL); > ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT; > > INSERT INTO tbl2 DEFAULT VALUES; > ERROR: no owned sequence found > > Though it works with tbl2(a int NOT NULL, b text)... > Take a look at this too, please. > Thanks Alexander for the report. PFA patch which fixes all the three problems. I had not fixed getIdentitySequence() to fetch identity sequence associated with the partition because I thought it would be better to fail with an error when it's not used correctly. But these bugs show 1. the error is misleading and unwanted 2. there are more places where adding that logic to getIdentitySequence() makes sense. Fixed the function in these patches. Now callers like transformAlterTableStmt have to be careful not to call the function on a partition. I have examined all the callers of getIdentitySequence() and they seem to be fine. The code related to SetIdentity, DropIdentity is not called for partitions, errors for which are thrown elsewhere earlier. -- Best Wishes, Ashutosh Bapat
From bce2e8d573040901b18c0c9f6884f0fd44f50724 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> Date: Tue, 30 Apr 2024 15:32:34 +0530 Subject: [PATCH] Fix assorted bugs related to identity column in partitioned tables When changing data type of a column of a partitioned table craft ALTER SEQUENCE command only once. Partitions do not have identity sequences of their own and thus do not need ALTER SEQUENCE command for each partition. Fix getIdentitySequence() to fetch the identity sequence associated with the top level partitioned table when Relation of a partition is passed to it. While doing so translate the attribute number of the partition into the attribute number of the partitioned table. Author: Ashutosh Bapat Discussion: https://www.postgresql.org/message-id/3b8a9dc1-bbc7-0ef5-6863-c432afac7...@gmail.com --- src/backend/catalog/pg_depend.c | 31 ++++++++++++++- src/backend/commands/tablecmds.c | 2 +- src/backend/parser/parse_utilcmd.c | 52 +++++++++++++++----------- src/backend/rewrite/rewriteHandler.c | 19 +--------- src/include/catalog/dependency.h | 2 +- src/test/regress/expected/identity.out | 32 +++++++++++++++- src/test/regress/sql/identity.sql | 11 +++++- 7 files changed, 101 insertions(+), 48 deletions(-) diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index f85a898de8..5366f7820c 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -23,10 +23,12 @@ #include "catalog/pg_constraint.h" #include "catalog/pg_depend.h" #include "catalog/pg_extension.h" +#include "catalog/partition.h" #include "commands/extension.h" #include "miscadmin.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" +#include "utils/syscache.h" #include "utils/rel.h" @@ -941,10 +943,35 @@ getOwnedSequences(Oid relid) * Get owned identity sequence, error if not exactly one. */ Oid -getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok) +getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok) { - List *seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL); + Oid relid; + List *seqlist; + /* + * The identity sequence is associated with the topmost partitioned table, + * which might have column order different than the given partition. + */ + if (RelationGetForm(rel)->relispartition) + { + List *ancestors = + get_partition_ancestors(RelationGetRelid(rel)); + HeapTuple ctup = SearchSysCacheAttNum(RelationGetRelid(rel), attnum); + const char *attname = NameStr(((Form_pg_attribute) GETSTRUCT(ctup))->attname); + HeapTuple ptup; + + relid = llast_oid(ancestors); + ptup = SearchSysCacheAttName(relid, attname); + attnum = ((Form_pg_attribute) GETSTRUCT(ptup))->attnum; + + ReleaseSysCache(ctup); + ReleaseSysCache(ptup); + list_free(ancestors); + } + else + relid = RelationGetRelid(rel); + + seqlist = getOwnedSequences_internal(relid, attnum, DEPENDENCY_INTERNAL); if (list_length(seqlist) > 1) elog(ERROR, "more than one owned sequence found"); else if (seqlist == NIL) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3556240c8e..9d32b2c495 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8516,7 +8516,7 @@ ATExecDropIdentity(Relation rel, const char *colName, bool missing_ok, LOCKMODE if (!recursing) { /* drop the internal sequence */ - seqid = getIdentitySequence(RelationGetRelid(rel), attnum, false); + seqid = getIdentitySequence(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 fef084f5d5..5e6bd58c3a 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1126,7 +1126,7 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla * find sequence owned by old column; extract sequence parameters; * build new create sequence command */ - seq_relid = getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false); + seq_relid = getIdentitySequence(relation, attribute->attnum, false); seq_options = sequence_options(seq_relid); generateSerialExtraStmts(cxt, def, InvalidOid, seq_options, @@ -3706,28 +3706,36 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, /* * For identity column, create ALTER SEQUENCE command to - * change the data type of the sequence. + * change the data type of the sequence. Identity sequence + * is associated with the top level partitioned table. + * Hence ignore partitions. */ - attnum = get_attnum(relid, cmd->name); - if (attnum == InvalidAttrNumber) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_COLUMN), - errmsg("column \"%s\" of relation \"%s\" does not exist", - cmd->name, RelationGetRelationName(rel)))); - - if (attnum > 0 && - TupleDescAttr(tupdesc, attnum - 1)->attidentity) + if (!RelationGetForm(rel)->relispartition) { - Oid seq_relid = getIdentitySequence(relid, attnum, false); - Oid typeOid = typenameTypeId(pstate, def->typeName); - AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt); - - altseqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)), - get_rel_name(seq_relid), - -1); - altseqstmt->options = list_make1(makeDefElem("as", (Node *) makeTypeNameFromOid(typeOid, -1), -1)); - altseqstmt->for_identity = true; - cxt.blist = lappend(cxt.blist, altseqstmt); + attnum = get_attnum(relid, cmd->name); + if (attnum == InvalidAttrNumber) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_COLUMN), + errmsg("column \"%s\" of relation \"%s\" does not exist", + cmd->name, RelationGetRelationName(rel)))); + + if (attnum > 0 && + TupleDescAttr(tupdesc, attnum - 1)->attidentity) + { + Oid seq_relid = getIdentitySequence(rel, attnum, false); + Oid typeOid = typenameTypeId(pstate, def->typeName); + AlterSeqStmt *altseqstmt = makeNode(AlterSeqStmt); + + altseqstmt->sequence + = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)), + get_rel_name(seq_relid), + -1); + altseqstmt->options = list_make1(makeDefElem("as", + (Node *) makeTypeNameFromOid(typeOid, -1), + -1)); + altseqstmt->for_identity = true; + cxt.blist = lappend(cxt.blist, altseqstmt); + } } newcmds = lappend(newcmds, cmd); @@ -3793,7 +3801,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, errmsg("column \"%s\" of relation \"%s\" does not exist", cmd->name, RelationGetRelationName(rel)))); - seq_relid = getIdentitySequence(relid, attnum, true); + seq_relid = getIdentitySequence(rel, attnum, true); if (seq_relid) { diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 9fd05b15e7..8a29fbbc46 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -24,7 +24,6 @@ #include "access/sysattr.h" #include "access/table.h" #include "catalog/dependency.h" -#include "catalog/partition.h" #include "commands/trigger.h" #include "executor/executor.h" #include "foreign/fdwapi.h" @@ -1233,24 +1232,8 @@ build_column_default(Relation rel, int attrno) if (att_tup->attidentity) { NextValueExpr *nve = makeNode(NextValueExpr); - Oid reloid; - /* - * The identity sequence is associated with the topmost partitioned - * table. - */ - if (rel->rd_rel->relispartition) - { - List *ancestors = - get_partition_ancestors(RelationGetRelid(rel)); - - reloid = llast_oid(ancestors); - list_free(ancestors); - } - else - reloid = RelationGetRelid(rel); - - nve->seqid = getIdentitySequence(reloid, attrno, false); + nve->seqid = getIdentitySequence(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 ec654010d4..f861d5c8a5 100644 --- a/src/include/catalog/dependency.h +++ b/src/include/catalog/dependency.h @@ -170,7 +170,7 @@ extern List *getAutoExtensionsOfObject(Oid classId, Oid objectId); extern bool sequenceIsOwned(Oid seqId, char deptype, Oid *tableId, int32 *colId); extern List *getOwnedSequences(Oid relid); -extern Oid getIdentitySequence(Oid relid, AttrNumber attnum, bool missing_ok); +extern Oid getIdentitySequence(Relation rel, AttrNumber attnum, bool missing_ok); extern Oid get_index_constraint(Oid indexId); diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out index f357b9b63b..4f67e75918 100644 --- a/src/test/regress/expected/identity.out +++ b/src/test/regress/expected/identity.out @@ -619,14 +619,17 @@ CREATE TABLE pitest1_p1 PARTITION OF pitest1 FOR VALUES FROM ('2016-07-01') TO ( INSERT into pitest1(f1, f2) VALUES ('2016-07-2', 'from pitest1'); INSERT into pitest1_p1 (f1, f2) VALUES ('2016-07-3', 'from pitest1_p1'); -- attached partition -CREATE TABLE pitest1_p2 (f1 date NOT NULL, f2 text, f3 bigint); -INSERT INTO pitest1_p2 VALUES ('2016-08-2', 'before attaching', 100); +CREATE TABLE pitest1_p2 (f3 bigint, f2 text, f1 date NOT NULL); +INSERT INTO pitest1_p2 (f1, f2, f3) VALUES ('2016-08-2', 'before attaching', 100); ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); -- requires NOT NULL constraint ERROR: column "f3" in child table must be marked NOT NULL ALTER TABLE pitest1_p2 ALTER COLUMN f3 SET NOT NULL; ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); INSERT INTO pitest1_p2 (f1, f2) VALUES ('2016-08-3', 'from pitest1_p2'); INSERT INTO pitest1 (f1, f2) VALUES ('2016-08-4', 'from pitest1'); +-- LIKE INCLUDING on partition +CREATE TABLE pitest1_p1_like(LIKE pitest1_p1 INCLUDING IDENTITY); +INSERT into pitest1_p1_like(f1, f2) VALUES ('2016-07-2', 'from pitest1_p1_like'); SELECT tableoid::regclass, f1, f2, f3 FROM pitest1; tableoid | f1 | f2 | f3 ------------+------------+------------------+----- @@ -637,6 +640,31 @@ SELECT tableoid::regclass, f1, f2, f3 FROM pitest1; pitest1_p2 | 08-04-2016 | from pitest1 | 4 (5 rows) +SELECT tableoid::regclass, f1, f2, f3 FROM pitest1_p1_like; + tableoid | f1 | f2 | f3 +-----------------+------------+----------------------+---- + pitest1_p1_like | 07-02-2016 | from pitest1_p1_like | 1 +(1 row) + +ALTER TABLE pitest1 ALTER COLUMN f3 SET DATA TYPE bigint; +SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1; + tableoid | f1 | f2 | f3 | pg_typeof +------------+------------+------------------+-----+----------- + pitest1_p1 | 07-02-2016 | from pitest1 | 1 | bigint + pitest1_p1 | 07-03-2016 | from pitest1_p1 | 2 | bigint + pitest1_p2 | 08-02-2016 | before attaching | 100 | bigint + pitest1_p2 | 08-03-2016 | from pitest1_p2 | 3 | bigint + pitest1_p2 | 08-04-2016 | from pitest1 | 4 | bigint +(5 rows) + +SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1_p2; + tableoid | f1 | f2 | f3 | pg_typeof +------------+------------+------------------+-----+----------- + pitest1_p2 | 08-02-2016 | before attaching | 100 | bigint + pitest1_p2 | 08-03-2016 | from pitest1_p2 | 3 | bigint + pitest1_p2 | 08-04-2016 | from pitest1 | 4 | bigint +(3 rows) + -- add identity column CREATE TABLE pitest2 (f1 date NOT NULL, f2 text) PARTITION BY RANGE (f1); CREATE TABLE pitest2_p1 PARTITION OF pitest2 FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql index 7b0800226c..c921074058 100644 --- a/src/test/regress/sql/identity.sql +++ b/src/test/regress/sql/identity.sql @@ -358,14 +358,21 @@ CREATE TABLE pitest1_p1 PARTITION OF pitest1 FOR VALUES FROM ('2016-07-01') TO ( INSERT into pitest1(f1, f2) VALUES ('2016-07-2', 'from pitest1'); INSERT into pitest1_p1 (f1, f2) VALUES ('2016-07-3', 'from pitest1_p1'); -- attached partition -CREATE TABLE pitest1_p2 (f1 date NOT NULL, f2 text, f3 bigint); -INSERT INTO pitest1_p2 VALUES ('2016-08-2', 'before attaching', 100); +CREATE TABLE pitest1_p2 (f3 bigint, f2 text, f1 date NOT NULL); +INSERT INTO pitest1_p2 (f1, f2, f3) VALUES ('2016-08-2', 'before attaching', 100); ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); -- requires NOT NULL constraint ALTER TABLE pitest1_p2 ALTER COLUMN f3 SET NOT NULL; ALTER TABLE pitest1 ATTACH PARTITION pitest1_p2 FOR VALUES FROM ('2016-08-01') TO ('2016-09-01'); INSERT INTO pitest1_p2 (f1, f2) VALUES ('2016-08-3', 'from pitest1_p2'); INSERT INTO pitest1 (f1, f2) VALUES ('2016-08-4', 'from pitest1'); +-- LIKE INCLUDING on partition +CREATE TABLE pitest1_p1_like(LIKE pitest1_p1 INCLUDING IDENTITY); +INSERT into pitest1_p1_like(f1, f2) VALUES ('2016-07-2', 'from pitest1_p1_like'); SELECT tableoid::regclass, f1, f2, f3 FROM pitest1; +SELECT tableoid::regclass, f1, f2, f3 FROM pitest1_p1_like; +ALTER TABLE pitest1 ALTER COLUMN f3 SET DATA TYPE bigint; +SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1; +SELECT tableoid::regclass, f1, f2, f3, pg_typeof(f3) FROM pitest1_p2; -- add identity column CREATE TABLE pitest2 (f1 date NOT NULL, f2 text) PARTITION BY RANGE (f1); -- 2.34.1
