From 936a9e3509867574633882f5c1ec714d2f2604ec Mon Sep 17 00:00:00 2001 From: Joseph Koshakow Date: Sat, 4 May 2024 10:12:37 -0400 Subject: [PATCH] Prevent name conflicts when dropping a column Previously, dropped columns were always renamed to "........pg.dropped.........". In the rare scenario that a column with that name already exists, the column drop would fail with an error about violating the unique constraint on "pg_attribute_relid_attnam_index". This commit fixes that issue by preventing users from creating columns with a name that matches "........pg.dropped.\d+........". This is backwards incompatible. --- src/backend/catalog/heap.c | 57 ++++++++++++++++++++-- src/backend/commands/tablecmds.c | 2 + src/include/catalog/heap.h | 3 ++ src/test/regress/expected/alter_table.out | 7 +++ src/test/regress/expected/create_table.out | 3 ++ src/test/regress/sql/alter_table.sql | 6 +++ src/test/regress/sql/create_table.sql | 3 ++ 7 files changed, 77 insertions(+), 4 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 922ba79ac2..0a0afe833d 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -231,6 +231,9 @@ static const FormData_pg_attribute a6 = { static const FormData_pg_attribute *const SysAtt[] = {&a1, &a2, &a3, &a4, &a5, &a6}; +static const char *dropped_col_pre = "........pg.dropped."; +static const char *dropped_col_post = "........"; + /* * This function returns a Form_pg_attribute pointer for a system attribute. * Note that we elog if the presented attno is invalid, which would only @@ -468,10 +471,10 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, MaxHeapAttributeNumber))); /* - * first check for collision with system attribute names + * first check for collision with system attribute and reserved names * * Skip this for a view or type relation, since those don't have system - * attributes. + * attributes and cannot drop columns. */ if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE) { @@ -484,6 +487,11 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, (errcode(ERRCODE_DUPLICATE_COLUMN), errmsg("column name \"%s\" conflicts with a system column name", NameStr(attr->attname)))); + + if ((CHKATYPE_RESERVED_NAME & flags) == 0) + { + CheckAttributeReservedName(NameStr(attr->attname)); + } } } @@ -679,6 +687,47 @@ CheckAttributeType(const char *attname, } } +/* + * TODO: Add function description. + */ +void +CheckAttributeReservedName(const char *attname) +{ + size_t name_len, + pre_len, + post_len; + int i; + + name_len = strlen(attname); + pre_len = strlen(dropped_col_pre); + post_len = strlen(dropped_col_post); + + if (name_len < pre_len + post_len + 1) + { + return; + } + if (memcmp(attname, dropped_col_pre, pre_len) != 0) + { + return; + } + for (i = pre_len; i < name_len - post_len; i++) + { + if (!isdigit(attname[i])) + { + return; + } + } + if (memcmp(attname + (name_len - post_len), dropped_col_post, post_len) != 0) + { + return; + } + + ereport(ERROR, + (errcode(ERRCODE_RESERVED_NAME), + errmsg("column name \"%s\" conflicts with a reserved column name", + attname))); +} + /* * InsertPgAttributeTuples * Construct and insert a set of tuples in pg_attribute. @@ -1148,7 +1197,7 @@ heap_create_with_catalog(const char *relname, * hack to allow creating pg_statistic and cloning it during VACUUM FULL. */ CheckAttributeNamesTypes(tupdesc, relkind, - allow_system_table_mods ? CHKATYPE_ANYARRAY : 0); + (allow_system_table_mods ? CHKATYPE_ANYARRAY : 0) | (is_internal ? CHKATYPE_RESERVED_NAME : 0)); /* * This would fail later on anyway, if the relation already exists. But @@ -1705,7 +1754,7 @@ RemoveAttributeById(Oid relid, AttrNumber attnum) * Change the column name to something that isn't likely to conflict */ snprintf(newattname, sizeof(newattname), - "........pg.dropped.%d........", attnum); + "%s%d%s", dropped_col_pre, attnum, dropped_col_post); namestrcpy(&(attStruct->attname), newattname); /* Clear the missing value */ diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a79ac884f7..bd475f7f77 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7485,6 +7485,8 @@ check_for_column_name_collision(Relation rel, const char *colname, HeapTuple attTuple; int attnum; + CheckAttributeReservedName(colname); + /* * this test is deliberately not attisdropped-aware, since if one tries to * add a column matching a dropped column name, it's gonna fail anyway. diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index e446d49b3e..29dc80b12e 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -23,6 +23,7 @@ #define CHKATYPE_ANYARRAY 0x01 /* allow ANYARRAY */ #define CHKATYPE_ANYRECORD 0x02 /* allow RECORD and RECORD[] */ #define CHKATYPE_IS_PARTKEY 0x04 /* attname is part key # not column */ +#define CHKATYPE_RESERVED_NAME 0x04 /* allow reserved names */ typedef struct RawColumnDefault { @@ -148,6 +149,8 @@ extern void CheckAttributeType(const char *attname, List *containing_rowtypes, int flags); +extern void CheckAttributeReservedName(const char *attname); + /* pg_partitioned_table catalog manipulation functions */ extern void StorePartitionKey(Relation rel, char strategy, diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 7666c76238..0fc479ae41 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1554,6 +1554,13 @@ insert into atacc1(id, value) values (null, 0); ERROR: null value in column "id" of relation "atacc1" violates not-null constraint DETAIL: Failing row contains (null, 0). drop table atacc1; +-- test reserved column name +create table atacc1(a int); +alter table atacc1 add column "........pg.dropped.1........" int; +ERROR: column name "........pg.dropped.1........" conflicts with a reserved column name +alter table atacc1 rename column a to "........pg.dropped.1........"; +ERROR: column name "........pg.dropped.1........" conflicts with a reserved column name +drop table atacc1; -- test inheritance create table parent (a int, b int, c int); insert into parent values (1, 2, 3); diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 344d05233a..08e36668c6 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -139,6 +139,9 @@ ALTER TABLE remember_node_subid ALTER c TYPE bigint; SAVEPOINT q; DROP TABLE remember_node_subid; ROLLBACK TO q; COMMIT; DROP TABLE remember_node_subid; +-- Verify that tables can't be created with reserved column names. +CREATE TABLE reserved_name ("........pg.dropped.1........" int); +ERROR: column name "........pg.dropped.1........" conflicts with a reserved column name -- -- Partitioned tables -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 9df5a63bdf..fb3f7cd30c 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1097,6 +1097,12 @@ insert into atacc1(value) values (100); insert into atacc1(id, value) values (null, 0); drop table atacc1; +-- test reserved column name +create table atacc1(a int); +alter table atacc1 add column "........pg.dropped.1........" int; +alter table atacc1 rename column a to "........pg.dropped.1........"; +drop table atacc1; + -- test inheritance create table parent (a int, b int, c int); insert into parent values (1, 2, 3); diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 1fd4cbfa7e..b395c8d2fc 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -91,6 +91,9 @@ SAVEPOINT q; DROP TABLE remember_node_subid; ROLLBACK TO q; COMMIT; DROP TABLE remember_node_subid; +-- Verify that tables can't be created with reserved column names. +CREATE TABLE reserved_name ("........pg.dropped.1........" int); + -- -- Partitioned tables -- -- 2.34.1