Date: Wed, 7 Oct 2020 17:26:56 +0300
From: Maksim Kita <kitaetoya@gmail.com>
To: pgsql-hackers@lists.postgresql.org
Subject: Allow deleting enum value
Message-ID: <20201007142656.GA181015@yetti>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline
Precedence: bulk


--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

There is a question related to TODO task with name "Allow deleting enumerated values from an existing enumerated data type".
I made simple changes in parser and implement RemoveEnumLabel function, but as I understand if enum value that we want to 
delete is in use by some tables, we have to prevent deletion to avoid inconsistency.

Could you please provide some references where similar functionality was implemented  ? Thanks.

Attached basic patch without handling dependencies. 

Example of problem that I mention using basic implementation:

postgres=# CREATE TYPE enum_test as ENUM ('1', '2', '3');
CREATE TYPE
postgres=# CREATE TYPE test_enum as ENUM ('1', '2', '3');
CREATE TYPE
postgres=# CREATE TABLE test_table (value test_enum);
CREATE TABLE
postgres=# INSERT INTO test_table VALUES ('1'), ('2');
INSERT 0 2
postgres=# ALTER TYPE test_enum DELETE VALUE '2';
ALTER TYPE
postgres=# SELECT enum_range(NULL::test_enum);
 enum_range
------------
 {1,3}
(1 row)

postgres=# SELECT * FROM test_table;
ERROR:  invalid internal value for enum: 16396

Best regards,
Maksim Kita

--mYCpIKhGyMATD0i+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Allow-alter-type-delete-value-in-enum.patch"

From 2ae09254584340b7700e31680d752822a782be75 Mon Sep 17 00:00:00 2001
From: Maksim Kita <kitaetoya@gmail.com>
Date: Wed, 7 Oct 2020 17:18:01 +0300
Subject: [PATCH] Allow alter type delete value in enum

---
 src/backend/catalog/pg_enum.c   | 60 +++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c |  8 +++--
 src/backend/parser/gram.y       | 11 ++++++
 src/include/catalog/pg_enum.h   |  1 +
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 27e4100a6f..c547dfd22c 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -583,6 +583,66 @@ RenameEnumLabel(Oid enumTypeOid,
 	table_close(pg_enum, RowExclusiveLock);
 }
 
+extern void RemoveEnumLabel(Oid enumTypeOid, const char *val)
+{
+	Relation	pg_enum;
+	HeapTuple	enum_tup;
+	Form_pg_enum en;
+	CatCList   *list;
+	int			nelems;
+	HeapTuple	old_tup;
+	int			i;
+
+	/* check length of new label is ok */
+	if (strlen(val) > (NAMEDATALEN - 1))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_NAME),
+				 errmsg("invalid enum label \"%s\"", val),
+				 errdetail("Labels must be %d characters or less.",
+						   NAMEDATALEN - 1)));
+
+	/*
+	 * Acquire a lock on the enum type, which we won't release until commit.
+	 * This ensures that two backends aren't concurrently modifying the same
+	 * enum type.  Since we are not changing the type's sort order, this is
+	 * probably not really necessary, but there seems no reason not to take
+	 * the lock to be sure.
+	 */
+	LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+
+	pg_enum = table_open(EnumRelationId, RowExclusiveLock);
+
+	/* Get the list of existing members of the enum */
+	list = SearchSysCacheList1(ENUMTYPOIDNAME,
+							   ObjectIdGetDatum(enumTypeOid));
+	nelems = list->n_members;
+
+	/*
+	 * Check if the label is in use.  (The unique index on pg_enum would catch that anyway, but we
+	 * prefer a friendlier error message.)
+	 */
+	for (i = 0; i < nelems; i++)
+	{
+		enum_tup = &(list->members[i]->tuple);
+		en = (Form_pg_enum) GETSTRUCT(enum_tup);
+		if (strcmp(NameStr(en->enumlabel), val) == 0)
+		{
+			old_tup = enum_tup;
+			break;
+		}
+	}
+	if (!old_tup)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("\"%s\" is not an existing enum label",
+						val)));
+
+	ReleaseCatCacheList(list);
+
+	CatalogTupleDelete(pg_enum, &old_tup->t_self);
+
+	table_close(pg_enum, RowExclusiveLock);
+}
 
 /*
  * Test if the given enum value is on the blacklist
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 483bb65ddc..b9b3e18a94 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1232,13 +1232,17 @@ AlterEnum(AlterEnumStmt *stmt)
 
 	ReleaseSysCache(tup);
 
-	if (stmt->oldVal)
+	if (stmt->oldVal && stmt->newVal)
 	{
 		/* Rename an existing label */
 		RenameEnumLabel(enum_type_oid, stmt->oldVal, stmt->newVal);
 	}
-	else
+	else if (stmt->oldVal)
 	{
+		/* Delete an existing label */
+		RemoveEnumLabel(enum_type_oid, stmt->oldVal);
+	}
+	else {
 		/* Add a new label */
 		AddEnumLabel(enum_type_oid, stmt->newVal,
 					 stmt->newValNeighbor, stmt->newValIsAfter,
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0d101d8171..82dd5147fa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5792,6 +5792,17 @@ AlterEnumStmt:
 				n->skipIfNewValExists = false;
 				$$ = (Node *) n;
 			}
+		 | ALTER TYPE_P any_name DELETE_P VALUE_P Sconst
+		 	{
+				AlterEnumStmt *n = makeNode(AlterEnumStmt);
+				n->typeName = $3;
+				n->oldVal = $6;
+				n->newVal = NULL;
+				n->newValNeighbor = NULL;
+				n->newValIsAfter = false;
+				n->skipIfNewValExists = false;
+				$$ = (Node *) n;
+			 }
 		 ;
 
 opt_if_not_exists: IF_P NOT EXISTS              { $$ = true; }
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index b28d441ba7..3dd81cd892 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -53,6 +53,7 @@ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
 						 bool skipIfExists);
 extern void RenameEnumLabel(Oid enumTypeOid,
 							const char *oldVal, const char *newVal);
+extern void RemoveEnumLabel(Oid enumTypeOid, const char *val);
 extern bool EnumBlacklisted(Oid enum_id);
 extern Size EstimateEnumBlacklistSpace(void);
 extern void SerializeEnumBlacklist(void *space, Size size);
-- 
2.25.1


--mYCpIKhGyMATD0i+--