On Sat, May 4, 2024 at 11:29 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I think we intentionally did not bother with preventing this,
> on the grounds that if you were silly enough to name a column
> that way then you deserve any ensuing problems.
Fair enough.
> If we were going to expend any code on the scenario, I'd prefer
> to make it be checks in column addition/renaming that disallow
> naming a column this way.
Is there any interest in making this change? The attached patch could
use some cleanup, but seems to accomplish what's described. It's
definitely more involved than the previous one and may not be worth the
effort. If you feel that it's worth it I can clean it up, otherwise
I'll drop it.
Thanks,
Joe Koshakow
From 936a9e3509867574633882f5c1ec714d2f2604ec Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
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.<attnum>........". 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
