Allow deleting enumerated values from an existing enumerated data type
Greetings, everyone!
I would like to offer my patch on the problem of removing values from enums
It adds support for expression ALTER TYPE <enum_name> DROP VALUE
<value_name>
Added:
1. expression in grammar
2. function to drop enum values
3. regression tests
4. documentation
Attachments:
enum_drop_value.patchtext/x-patch; charset=US-ASCII; name=enum_drop_value.patchDownload
Subject: [PATCH] Add DROP VALUE for ALTER TYPE with enum
---
Index: doc/src/sgml/datatype.sgml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
--- a/doc/src/sgml/datatype.sgml (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/doc/src/sgml/datatype.sgml (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -3294,10 +3294,10 @@
<para>
Although enum types are primarily intended for static sets of values,
- there is support for adding new values to an existing enum type, and for
- renaming values (see <xref linkend="sql-altertype"/>). Existing values
- cannot be removed from an enum type, nor can the sort ordering of such
- values be changed, short of dropping and re-creating the enum type.
+ there is support for adding new values to an existing enum type, and
+ also for dropping and renaming values (see <xref linkend="sql-altertype"/>).
+ The sorting order of such values cannot be changed, except for deleting
+ and re-creating the enum type.
</para>
<para>
Index: doc/src/sgml/ref/alter_type.sgml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
--- a/doc/src/sgml/ref/alter_type.sgml (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/doc/src/sgml/ref/alter_type.sgml (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -30,6 +30,7 @@
ALTER TYPE <replaceable class="parameter">name</replaceable> <replaceable class="parameter">action</replaceable> [, ... ]
ALTER TYPE <replaceable class="parameter">name</replaceable> ADD VALUE [ IF NOT EXISTS ] <replaceable class="parameter">new_enum_value</replaceable> [ { BEFORE | AFTER } <replaceable class="parameter">neighbor_enum_value</replaceable> ]
ALTER TYPE <replaceable class="parameter">name</replaceable> RENAME VALUE <replaceable class="parameter">existing_enum_value</replaceable> TO <replaceable class="parameter">new_enum_value</replaceable>
+ALTER TYPE <replaceable class="parameter">name</replaceable> DROP VALUE <replaceable class="parameter">existing_enum_value</replaceable>
ALTER TYPE <replaceable class="parameter">name</replaceable> SET ( <replaceable class="parameter">property</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
<phrase>where <replaceable class="parameter">action</replaceable> is one of:</phrase>
@@ -145,6 +146,15 @@
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><literal>DROP VALUE</literal></term>
+ <listitem>
+ <para>
+ This form drops a value of an enum type.
+ </para>
+ </listitem>
+ </varlistentry>
<varlistentry>
<term>
@@ -462,6 +472,13 @@
ALTER TYPE colors RENAME VALUE 'purple' TO 'mauve';
</programlisting>
</para>
+
+ <para>
+ To drop an enum value:
+<programlisting>
+ALTER TYPE colors DROP VALUE 'red';
+</programlisting>
+ </para>
<para>
To create binary I/O functions for an existing base type:
Index: src/backend/catalog/pg_enum.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
--- a/src/backend/catalog/pg_enum.c (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/backend/catalog/pg_enum.c (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -612,6 +612,62 @@
table_close(pg_enum, RowExclusiveLock);
}
+void
+DropEnumLabel(Oid enumTypeOid, const char *oldVal)
+{
+ Relation pg_enum;
+ HeapTuple enum_tup;
+ Form_pg_enum en;
+ CatCList *list;
+ int nelems;
+ HeapTuple old_tup;
+ int i;
+
+
+ /*
+ * 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;
+
+ /*
+ * Locate the element to rename and check if the new label is already in
+ * use. (The unique index on pg_enum would catch that anyway, but we
+ * prefer a friendlier error message.)
+ */
+ old_tup = NULL;
+ for (i = 0; i < nelems; i++)
+ {
+ enum_tup = &(list->members[i]->tuple);
+ en = (Form_pg_enum) GETSTRUCT(enum_tup);
+ if (strcmp(NameStr(en->enumlabel), oldVal) == 0)
+ {
+ old_tup = enum_tup;
+ }
+ }
+
+ ReleaseCatCacheList(list);
+
+ if (!old_tup)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not an existing enum label",
+ oldVal)));
+
+ CatalogTupleDelete(pg_enum, &old_tup->t_self);
+ table_close(pg_enum, RowExclusiveLock);
+}
+
/*
* Test if the given enum value is in the table of uncommitted enums.
Index: src/backend/commands/typecmds.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
--- a/src/backend/commands/typecmds.c (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/backend/commands/typecmds.c (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -1280,8 +1280,16 @@
if (stmt->oldVal)
{
- /* Rename an existing label */
- RenameEnumLabel(enum_type_oid, stmt->oldVal, stmt->newVal);
+ if(stmt->newVal)
+ {
+ /* Rename an existing label */
+ RenameEnumLabel(enum_type_oid, stmt->oldVal, stmt->newVal);
+ }
+ else
+ {
+ /* Delete an existing label */
+ DropEnumLabel(enum_type_oid, stmt->oldVal);
+ }
}
else
{
Index: src/backend/parser/gram.y
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
--- a/src/backend/parser/gram.y (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/backend/parser/gram.y (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -6359,7 +6359,7 @@
ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
-
+
n->typeName = $3;
n->oldVal = NULL;
n->newVal = $7;
@@ -6404,6 +6404,18 @@
n->skipIfNewValExists = false;
$$ = (Node *) n;
}
+ | ALTER TYPE_P any_name DROP 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; }
Index: src/include/catalog/pg_enum.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
--- a/src/include/catalog/pg_enum.h (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/include/catalog/pg_enum.h (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -57,6 +57,7 @@
bool skipIfExists);
extern void RenameEnumLabel(Oid enumTypeOid,
const char *oldVal, const char *newVal);
+extern void DropEnumLabel(Oid enumTypeOid, const char *oldVal);
extern bool EnumUncommitted(Oid enum_id);
extern Size EstimateUncommittedEnumsSpace(void);
extern void SerializeUncommittedEnums(void *space, Size size);
Index: src/test/regress/expected/enum.out
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
--- a/src/test/regress/expected/enum.out (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/test/regress/expected/enum.out (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -605,6 +605,28 @@
-- check that renaming to an existent value fails
ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
ERROR: enum label "green" already exists
+-- check dropping a value
+ALTER TYPE rainbow DROP VALUE 'purple';
+ALTER TYPE rainbow DROP VALUE 'blue';
+ALTER TYPE rainbow DROP VALUE 'green';
+ALTER TYPE rainbow DROP VALUE 'yellow';
+ALTER TYPE rainbow DROP VALUE 'orange';
+ALTER TYPE rainbow DROP VALUE 'crimson';
+SELECT enum_range(NULL::rainbow);
+ enum_range
+------------
+ {}
+(1 row)
+
+ALTER TYPE rainbow DROP VALUE 'purple';
+ERROR: "purple" is not an existing enum label
+ALTER TYPE rainbow ADD VALUE 'purple';
+SELECT enum_range(NULL::rainbow);
+ enum_range
+------------
+ {purple}
+(1 row)
+
--
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
Index: src/test/regress/sql/enum.sql
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
--- a/src/test/regress/sql/enum.sql (revision 1f9e3a9be539f912babd3ad58d01a4ce6aa0b85b)
+++ b/src/test/regress/sql/enum.sql (revision 2984db4be6d2a4afc37ba15049c6365b880b6609)
@@ -276,6 +276,20 @@
-- check that renaming to an existent value fails
ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
+-- check dropping a value
+ALTER TYPE rainbow DROP VALUE 'purple';
+ALTER TYPE rainbow DROP VALUE 'blue';
+ALTER TYPE rainbow DROP VALUE 'green';
+ALTER TYPE rainbow DROP VALUE 'yellow';
+ALTER TYPE rainbow DROP VALUE 'orange';
+ALTER TYPE rainbow DROP VALUE 'crimson';
+SELECT enum_range(NULL::rainbow);
+
+ALTER TYPE rainbow DROP VALUE 'purple';
+
+ALTER TYPE rainbow ADD VALUE 'purple';
+SELECT enum_range(NULL::rainbow);
+
--
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
On 9/28/23 14:13, Данил Столповских wrote:
Greetings, everyone!
I would like to offer my patch on the problem of removing values from enumsIt adds support for expression ALTER TYPE <enum_name> DROP VALUE
<value_name>Added:
1. expression in grammar
2. function to drop enum values
3. regression tests
4. documentation
Thanks for this patch that a lot of people want.
However, it does not seem to address the issue of how to handle the
dropped value being in the high key of an index. Until we solve that
problem (and maybe others), this kind of patch is insufficient to add
the feature.
--
Vik Fearing
=?UTF-8?B?0JTQsNC90LjQuyDQodGC0L7Qu9C/0L7QstGB0LrQuNGF?= <danil.stolpovskikh@gmail.com> writes:
I would like to offer my patch on the problem of removing values from enums
It adds support for expression ALTER TYPE <enum_name> DROP VALUE
<value_name>
This does not fix any of the hard problems that caused us not to
have such a feature to begin with. Notably, what happens to
stored data of the enum type if it is a now-deleted value?
regards, tom lane
On 2023-09-28 Th 10:28, Tom Lane wrote:
=?UTF-8?B?0JTQsNC90LjQuyDQodGC0L7Qu9C/0L7QstGB0LrQuNGF?= <danil.stolpovskikh@gmail.com> writes:
I would like to offer my patch on the problem of removing values from enums
It adds support for expression ALTER TYPE <enum_name> DROP VALUE
<value_name>This does not fix any of the hard problems that caused us not to
have such a feature to begin with. Notably, what happens to
stored data of the enum type if it is a now-deleted value?
I wonder if we could have a boolean flag in pg_enum, indicating that
setting an enum to that value was forbidden. That wouldn't delete the
value but it wouldn't show up in enum_range and friends. We'd have to
teach pg_dump and pg_upgrade to deal with it, but that shouldn't be too
hard.
Perhaps the command could be something like
ALTER TYPE enum_name DISABLE value;
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
I wonder if we could have a boolean flag in pg_enum, indicating that
setting an enum to that value was forbidden.
Yeah, but that still offers no coherent solution to the problem of
what happens if there's a table that already contains such a value.
It doesn't seem terribly useful to forbid new entries if you can't
get rid of old ones.
Admittedly, a DISABLE flag would at least offer a chance at a
race-condition-free scan to verify that no such values remain
in tables. But as somebody already mentioned upthread, that
wouldn't guarantee that the value doesn't appear in non-leaf
index pages. So basically you could never get rid of the pg_enum
row, short of a full dump and restore.
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.
regards, tom lane
On 9/28/23 20:46, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I wonder if we could have a boolean flag in pg_enum, indicating that
setting an enum to that value was forbidden.Yeah, but that still offers no coherent solution to the problem of
what happens if there's a table that already contains such a value.
It doesn't seem terribly useful to forbid new entries if you can't
get rid of old ones.Admittedly, a DISABLE flag would at least offer a chance at a
race-condition-free scan to verify that no such values remain
in tables. But as somebody already mentioned upthread, that
wouldn't guarantee that the value doesn't appear in non-leaf
index pages. So basically you could never get rid of the pg_enum
row, short of a full dump and restore.We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.
This issue comes up regularly (although far from often). Do we want to
put some comments right where would-be implementors would be sure to see it?
Attached is an example of what I mean. Documentation is intentionally
omitted.
--
Vik Fearing
Attachments:
dropenum.difftext/x-patch; charset=UTF-8; name=dropenum.diffDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7d2032885e..f8d70cdaa0 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6404,6 +6404,29 @@ AlterEnumStmt:
n->skipIfNewValExists = false;
$$ = (Node *) n;
}
+ | ALTER TYPE_P any_name DROP VALUE_P Sconst
+ {
+ /*
+ * The following problems must be solved before this can be
+ * implemented:
+ *
+ * - There can be no data of this type using this value in
+ * any table
+ *
+ * - The value may not appear in any non-leaf page of a
+ * btree (and similar issues with other index types)
+ *
+ * - Concurrent sessions must not be able to insert this
+ * value while the preceding conditions are being checked
+ *
+ * - Possibly more...
+ */
+
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("dropping an enum value is not yet implemented"),
+ parser_errposition(@4)));
+ }
;
opt_if_not_exists: IF_P NOT EXISTS { $$ = true; }
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 01159688e5..105ae7a2f9 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -605,6 +605,11 @@ ERROR: "red" is not an existing enum label
-- check that renaming to an existent value fails
ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
ERROR: enum label "green" already exists
+-- We still can't drop an enum value
+ALTER TYPE rainbow DROP VALUE 'green';
+ERROR: dropping an enum value is not yet implemented
+LINE 1: ALTER TYPE rainbow DROP VALUE 'green';
+ ^
--
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 93171379f2..a98df40ed8 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -276,6 +276,9 @@ ALTER TYPE rainbow RENAME VALUE 'red' TO 'crimson';
-- check that renaming to an existent value fails
ALTER TYPE rainbow RENAME VALUE 'blue' TO 'green';
+-- We still can't drop an enum value
+ALTER TYPE rainbow DROP VALUE 'green';
+
--
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
Vik Fearing <vik@postgresfriends.org> writes:
On 9/28/23 20:46, Tom Lane wrote:
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.
This issue comes up regularly (although far from often). Do we want to
put some comments right where would-be implementors would be sure to see it?
Perhaps. I'd be kind of inclined to leave the "yet" out of "not yet
implemented" in the error message, as that wording sounds like we just
haven't got round to it.
regards, tom lane
On 2023-09-28 Th 14:46, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I wonder if we could have a boolean flag in pg_enum, indicating that
setting an enum to that value was forbidden.Yeah, but that still offers no coherent solution to the problem of
what happens if there's a table that already contains such a value.
It doesn't seem terribly useful to forbid new entries if you can't
get rid of old ones.Admittedly, a DISABLE flag would at least offer a chance at a
race-condition-free scan to verify that no such values remain
in tables. But as somebody already mentioned upthread, that
wouldn't guarantee that the value doesn't appear in non-leaf
index pages. So basically you could never get rid of the pg_enum
row, short of a full dump and restore.
or a reindex, I think, although getting the timing right would be messy.
I agree the non-leaf index pages are rather pesky in dealing with this.
I guess the alternative would be to create a new enum with the
to-be-deleted value missing, and then alter the column type to the new
enum type. For massive tables that would be painful.
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.
That's quite true, and I accept my part in this history. But I'm not
sure we were correct back then.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 9/29/23 03:17, Tom Lane wrote:
Vik Fearing <vik@postgresfriends.org> writes:
On 9/28/23 20:46, Tom Lane wrote:
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.This issue comes up regularly (although far from often). Do we want to
put some comments right where would-be implementors would be sure to see it?Perhaps. I'd be kind of inclined to leave the "yet" out of "not yet
implemented" in the error message, as that wording sounds like we just
haven't got round to it.
I see your point, but should we be dissuading people who might want to
work on solving those problems? I intentionally did not document that
this syntax exists so the only people seeing the message are those who
just try it, and those wanting to write a patch like Danil did.
No one except you has said anything about this patch. I think it would
be good to commit it, wordsmithing aside.
--
Vik Fearing
Vik Fearing <vik@postgresfriends.org> writes:
On 9/29/23 03:17, Tom Lane wrote:
Vik Fearing <vik@postgresfriends.org> writes:
On 9/28/23 20:46, Tom Lane wrote:
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.This issue comes up regularly (although far from often). Do we want to
put some comments right where would-be implementors would be sure to see it?Perhaps. I'd be kind of inclined to leave the "yet" out of "not yet
implemented" in the error message, as that wording sounds like we just
haven't got round to it.I see your point, but should we be dissuading people who might want to
work on solving those problems? I intentionally did not document that
this syntax exists so the only people seeing the message are those who
just try it, and those wanting to write a patch like Danil did.No one except you has said anything about this patch. I think it would
be good to commit it, wordsmithing aside.
FWIW I'm +1 on this patch, and with Tom on dropping the "yet". To me it
makes it sound like we intend to implement it soon (fsvo).
- ilmari
Andrew Dunstan <andrew@dunslane.net> writes:
On 2023-09-28 Th 14:46, Tom Lane wrote:
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.
That's quite true, and I accept my part in this history. But I'm not
sure we were correct back then.
I think it was the right decision at the time, given that the
alternative was to not add the enum feature at all. The question
is whether we're now prepared to do additional work to support DROP
VALUE. But the tradeoff still looks pretty grim, because the
problems haven't gotten any easier.
I've been trying to convince myself that there'd be some value in
your idea about a DISABLE flag, but I feel like there's something
missing there. The easiest implementation would be to have
enum_in() reject disabled values, while still allowing enum_out()
to print them. But that doesn't seem to lead to nice results:
* You couldn't do, say,
SELECT * FROM my_table WHERE enum_col = 'disabled_value'
to look for rows that you need to clean up. I guess this'd work:
SELECT * FROM my_table WHERE enum_col::text = 'disabled_value'
but it's un-obvious and could not use an index on enum_col.
* If any of the disabled values remain, dump/restore would fail.
Maybe you'd want that to be sure you got rid of them, but it sounds
like a foot-gun. ("What do you mean, our only backup doesn't
restore?") Probably people would wish for two different behaviors:
either don't list disabled values at all in the dumped CREATE TYPE,
or do list them but disable them only after loading data. The latter
approach will still have problems in data-only restores, but there are
comparable hazards with things like foreign keys. (pg_upgrade would
need still a third behavior, perhaps.)
On the whole this is still a long way from a clean easy-to-use DROP
facility, and it adds a lot of complexity of its own for pg_dump.
So I'm not sure we want to build it.
regards, tom lane
On 10/2/23 20:07, Dagfinn Ilmari Mannsåker wrote:
Vik Fearing <vik@postgresfriends.org> writes:
No one except you has said anything about this patch. I think it would
be good to commit it, wordsmithing aside.FWIW I'm +1 on this patch,
Thanks.
and with Tom on dropping the "yet". To me it
makes it sound like we intend to implement it soon (fsvo).
I am not fundamentally opposed to it, nor to any other wordsmithing the
committer (probably Tom) wants to do. The main point of the patch is to
list at least some of the problems that need to be solved in a correct
implementation.
--
Vik Fearing
Vik Fearing <vik@postgresfriends.org> writes:
On 10/2/23 20:07, Dagfinn Ilmari Mannsåker wrote:
FWIW I'm +1 on this patch,
Thanks.
and with Tom on dropping the "yet". To me it
makes it sound like we intend to implement it soon (fsvo).
I am not fundamentally opposed to it, nor to any other wordsmithing the
committer (probably Tom) wants to do. The main point of the patch is to
list at least some of the problems that need to be solved in a correct
implementation.
Pushed with a bit more work on the text.
I left out the regression test, as it seems like it'd add test cycles
to little purpose. It won't do anything to improve the odds that
someone finds this text.
regards, tom lane
On 10/3/23 17:44, Tom Lane wrote:
Vik Fearing <vik@postgresfriends.org> writes:
On 10/2/23 20:07, Dagfinn Ilmari Mannsåker wrote:
FWIW I'm +1 on this patch,
Thanks.
and with Tom on dropping the "yet". To me it
makes it sound like we intend to implement it soon (fsvo).I am not fundamentally opposed to it, nor to any other wordsmithing the
committer (probably Tom) wants to do. The main point of the patch is to
list at least some of the problems that need to be solved in a correct
implementation.Pushed with a bit more work on the text.
I left out the regression test, as it seems like it'd add test cycles
to little purpose. It won't do anything to improve the odds that
someone finds this text.
Thanks!
--
Vik Fearing
On Tue, 3 Oct 2023 at 22:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
On 2023-09-28 Th 14:46, Tom Lane wrote:
We went through all these points years ago when the enum feature
was first developed, as I recall. Nobody thought that the ability
to remove an enum value was worth the amount of complexity it'd
entail.That's quite true, and I accept my part in this history. But I'm not
sure we were correct back then.I think it was the right decision at the time, given that the
alternative was to not add the enum feature at all. The question
is whether we're now prepared to do additional work to support DROP
VALUE. But the tradeoff still looks pretty grim, because the
problems haven't gotten any easier.I've been trying to convince myself that there'd be some value in
your idea about a DISABLE flag, but I feel like there's something
missing there. The easiest implementation would be to have
enum_in() reject disabled values, while still allowing enum_out()
to print them. But that doesn't seem to lead to nice results:[...]
On the whole this is still a long way from a clean easy-to-use DROP
facility, and it adds a lot of complexity of its own for pg_dump.
So I'm not sure we want to build it.
I don't quite get what the hard problem is that we haven't already
solved for other systems:
We already can add additional constraints to domains (e.g. VALUE::int
<> 4), which (according to docs) scan existing data columns for
violations. We already drop columns without rewriting the table to
remove the column's data, and reject new data insertions for those
still-in-the-catalogs-but-inaccessible columns.
So, if a user wants to drop an enum value, why couldn't we "just" use
the DOMAIN facilities and 1.) add a constraint WHERE value NOT IN
(deleted_values), and after validation of that constraint 2.) mark the
enum value as deleted like we do with table column's pg_attribute
entries?
The only real issue that I can think of is making sure that concurrent
backends don't modify this data, but that shouldn't be very different
from the other locks we already have to take in e.g. ALTER TYPE ...
DROP ATTRIBUTE.
Kind regards,
Matthias van de Meent
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
I don't quite get what the hard problem is that we haven't already
solved for other systems:
We already can add additional constraints to domains (e.g. VALUE::int
<> 4), which (according to docs) scan existing data columns for
violations.
That's "solved" only for rather small values of "solved". While the
code does try to look for violations of the new constraint, there is
no interlock against race conditions (ie, concurrent insertions of
a conflicting value). It doesn't check temp tables belonging to
other backends, because it can't. And IIRC it doesn't look for
instances in metadata, such as stored views.
The reason we've considered that Good Enough(TM) for domain
constraints is that if something does sneak through those loopholes,
nothing terribly surprising happens. You've got a value there that
shouldn't be there, but that's mostly your own fault, and the
system continues to behave sanely. Also you don't have any problem
introspecting what you've got, because such values will still print
normally. Plus, we can't really guarantee that users won't get
into such a state in other ways, for example if their constraint
isn't really immutable.
The problem with dropping an enum value is that surprising things
might very well happen, because removal of the pg_enum row risks
comparisons failing if they involve the dropped value. Thus for
example you might find yourself with a broken index that fails
all insertion and search attempts, even if you'd carefully removed
every user-visible instance of the doomed value: an instance of
it high up in the index tree will break most index accesses.
Even for values in user-visible places like views, the fact that
enum_out will fail doesn't make it any easier to figure out what
is wrong.
We might be able to get to a place where the surprise factor is
low enough to tolerate, but the domain-constraint precedent isn't
good enough for that IMO.
Andrew's idea of DISABLE rather than full DROP is one way of
ameliorating these problems: comparisons would still work, and
we can still print a value that perhaps shouldn't have been there.
But it's not without other problems.
We already drop columns without rewriting the table to
remove the column's data, and reject new data insertions for those
still-in-the-catalogs-but-inaccessible columns.
Those cases don't seem to have a lot of connection to the enum problem.
The only real issue that I can think of is making sure that concurrent
backends don't modify this data, but that shouldn't be very different
from the other locks we already have to take in e.g. ALTER TYPE ...
DROP ATTRIBUTE.
I'd bet a good deal of money that those cases aren't too bulletproof.
We do not lock types simply because somebody has a value of the type
in flight somewhere in a query, and the cost of doing so would be
quite discouraging I fear. On the whole, I'd rather accept the
idea that the DROP might not be completely watertight; but then
we have to work out the details of coping with orphaned values
in an acceptable way.
regards, tom lane