Date: Wed, 7 Oct 2020 23:47:07 +0300
From: Maksim Kita <kitaetoya@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Allow deleting enum value
Message-ID: <20201007204707.GA210713@yetti>
References: <20201007142656.GA181015@yetti>
 <2151250.1602084214@sss.pgh.pa.us>
 <2191224.1602086595@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="qDbXVdCdHGoSgWSk"
Content-Disposition: inline
In-Reply-To: <2191224.1602086595@sss.pgh.pa.us>
Precedence: bulk


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

I like the idea, during prototype I added additional column and modified
enum_in method. But enum_in is called in contexts that can be important
for user (like comparisons).

Example:

postgres=# CREATE TYPE test_enum AS enum ('1', '2', '3');
CREATE TYPE

postgres=# CREATE TABLE test_table ( value test_enum );
postgres=# INSERT INTO test_table VALUES ('1'), ('2'), ('3');
INSERT 0 3

postgres=# ALTER TYPE test_enum DELETE VALUE '2';
ALTER TYPE

postgres=# INSERT INTO test_table VALUES ('2');
ERROR:  enum value is dropped test_enum: "2"
LINE 1: INSERT INTO test_table VALUES ('2');

postgres=# SELECT * FROM test_table WHERE value = '2';
ERROR:  enum value is dropped test_enum: "2"
LINE 1: SELECT * FROM test_table WHERE value = '2';

postgres=# UPDATE test_table SET value = '3' WHERE value = '2';
ERROR:  enum value is dropped test_enum: "2"
LINE 1: UPDATE test_table SET value = '3' WHERE value = '2';

Probably we need to make more specific change for enum type to prevent
using of dropped column in context of insert or update (where we
creating dropped enum value), but not in others.

Is that possible ? What places should I look into ? Thanks.

Best regards,
Maksim Kita

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

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

---
 src/backend/catalog/pg_enum.c   | 69 +++++++++++++++++++++++++++++++++
 src/backend/commands/typecmds.c |  8 +++-
 src/backend/parser/gram.y       | 11 ++++++
 src/backend/utils/adt/enum.c    | 10 +++++
 src/include/catalog/pg_enum.h   |  2 +
 5 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 27e4100a6f..907db93cf4 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -133,6 +133,7 @@ EnumValuesCreate(Oid enumTypeOid, List *vals)
 		values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(elemno + 1);
 		namestrcpy(&enumlabel, lab);
 		values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
+		values[Anum_pg_enum_isdropped - 1] = false;
 
 		tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
 
@@ -485,6 +486,7 @@ restart:
 	values[Anum_pg_enum_enumsortorder - 1] = Float4GetDatum(newelemorder);
 	namestrcpy(&enumlabel, newVal);
 	values[Anum_pg_enum_enumlabel - 1] = NameGetDatum(&enumlabel);
+	values[Anum_pg_enum_isdropped - 1] = false;
 	enum_tup = heap_form_tuple(RelationGetDescr(pg_enum), values, nulls);
 	CatalogTupleInsert(pg_enum, enum_tup);
 	heap_freetuple(enum_tup);
@@ -583,6 +585,73 @@ 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)));
+
+	/* OK, make a writable copy of old tuple */
+	enum_tup = heap_copytuple(old_tup);
+	en = (Form_pg_enum) GETSTRUCT(enum_tup);
+
+	ReleaseCatCacheList(list);
+
+	/* Update the pg_enum entry */
+	en->isdropped = true;
+	CatalogTupleUpdate(pg_enum, &enum_tup->t_self, enum_tup);
+	heap_freetuple(enum_tup);
+
+	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/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 5ead794e34..397c02fa69 100644
--- a/src/backend/utils/adt/enum.c
+++ b/src/backend/utils/adt/enum.c
@@ -113,6 +113,7 @@ enum_in(PG_FUNCTION_ARGS)
 	Oid			enumtypoid = PG_GETARG_OID(1);
 	Oid			enumoid;
 	HeapTuple	tup;
+	Form_pg_enum en;
 
 	/* must check length to prevent Assert failure within SearchSysCache */
 	if (strlen(name) >= NAMEDATALEN)
@@ -132,6 +133,15 @@ enum_in(PG_FUNCTION_ARGS)
 						format_type_be(enumtypoid),
 						name)));
 
+	en = (Form_pg_enum) GETSTRUCT(tup);
+
+	if (en->isdropped)
+		ereport(ERROR,
+			(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+				errmsg("enum value is dropped %s: \"%s\"",
+						format_type_be(enumtypoid),
+						name)));
+
 	/* check it's safe to use in SQL */
 	check_safe_enum_use(tup);
 
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index b28d441ba7..6a7b00ffbe 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -34,6 +34,7 @@ CATALOG(pg_enum,3501,EnumRelationId)
 	Oid			enumtypid;		/* OID of owning enum type */
 	float4		enumsortorder;	/* sort position of this enum value */
 	NameData	enumlabel;		/* text representation of enum value */
+	bool 		isdropped;		/* is enum value dropped */
 } FormData_pg_enum;
 
 /* ----------------
@@ -53,6 +54,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


--qDbXVdCdHGoSgWSk--