On 09/23/2017 03:52 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.duns...@2ndquadrant.com> writes:
>> On 09/23/2017 02:00 PM, Tom Lane wrote:
>>> So I'm back to not being sure about the path forward. Maybe it would be
>>> all right to say "the value added by ADD VALUE can't be used in the same
>>> transaction, period". That's still a step forward compared to the pre-v10
>>> prohibition on doing it at all. I don't remember if there were use-cases
>>> where we really needed the exception for new-in-transaction types.
>> Well, my idea was to have the test run like this:
>> * is the value an old one? Test txnid of tuple. If yes it's ok
>> * is the value one created by ALTER TYPE ADD VALUE? Test
>> blacklist. If no, it's ok.
>> * is the enum a new one? Test whitelist. If yes, it's ok.
>> * anything else is not ok.
> My point is that if you do 1 and 3, you don't need 2. Or if you do
> 2 and 3, you don't need 1. But in most cases, testing the tuple
> hint bits is cheap, so you don't really want that option.
>
> In any case, what I'm worried about is the amount of bookkeeping
> overhead added by keeping a whitelist of enum-types-created-in-
> current-transaction. That's less than trivial, especially since
> you have to account correctly for subtransactions. And there are
> common use-cases where that table will become large.
>
>> If we just did the blacklist and stuck with our current heuristic test
>> for enum being created in the current transaction, we'd still probably
>> avoid 99% of the problems, including specifically the one that gave rise
>> to the bug report.
> True. But I'm not sure whether the heuristic test is adding anything
> meaningful if we use a blacklist first. The case where it could help
> is
>
> begin;
> create type t as enum();
> alter type t add value 'v';
> -- do something with 'v'
> commit;
>
> That perhaps is worth something, but if somebody is trying to build a new
> enum type in pieces like that, doesn't it seem fairly likely that they
> might throw in an ALTER OWNER or GRANT as well? My feeling is that the
> lesson we need to learn is that the heuristic test isn't good enough.
>
>
OK, I think I'm convinced. Here's is the WIP code I put together for the
blacklist. I'm was looking for a place to put the init call, but since
it's possibly not going anywhere I stopped :-) . My initial thought
about substransactions was that we should ignore them for this purpose
(That's why I used TopTransactionContext for the table).
I agree the heuristic test isn't good enough, and if we can get a 100%
accurate test for the newness of the enum type then the blacklist would
be redundant.
w.r.t. table size - how large? I confess I haven't seen any systems with
more than a few hundred enum types. But even a million or two shouldn't
consume a huge amount of memory, should it?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index fe61d4d..52c1271 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -28,6 +28,8 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
@@ -38,6 +40,9 @@ Oid binary_upgrade_next_pg_enum_oid = InvalidOid;
static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
static int sort_order_cmp(const void *p1, const void *p2);
+/* hash table of values added in current transaction by AddEnumLabel */
+
+static HTAB *enum_blacklist = NULL;
/*
* EnumValuesCreate
@@ -460,8 +465,44 @@ restart:
heap_freetuple(enum_tup);
heap_close(pg_enum, RowExclusiveLock);
+
+ /* set up blacklist hash if required */
+ if (enum_blacklist == NULL)
+ {
+ HASHCTL hash_ctl;
+ memset(&hash_ctl, 0, sizeof(hash_ctl));
+ hash_ctl.keysize = sizeof(Oid);
+ hash_ctl.entrysize = sizeof(Oid);
+ hash_ctl.hcxt = CurTransactionContext;
+ enum_blacklist = hash_create("Enum blacklist for current transaction",
+ 32,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+ }
+
+ /* and add the new value to the blacklist */
+
+ (void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL);
}
+bool
+EnumBlacklisted(Oid enum_id)
+{
+ bool found;
+
+ if (enum_blacklist == NULL)
+ return false;
+
+ (void) hash_search(enum_blacklist, &enum_id, HASH_FIND, &found);
+ return found;
+}
+
+void
+InitEnumBlacklist(void)
+{
+ enum_blacklist = NULL;
+}
/*
* RenameEnumLabel
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 973397c..a7ba3d0 100644
--- a/src/backend/utils/adt/enum.c
+++ b/src/backend/utils/adt/enum.c
@@ -76,6 +76,10 @@ check_safe_enum_use(HeapTuple enumval_tup)
TransactionIdDidCommit(xmin))
return;
+ /* Check if the enum value is blacklisted. If not, it's safe */
+ if (! EnumBlacklisted(HeapTupleGetOid(enumval_tup)))
+ return;
+
/* It is a new enum value, so check to see if the whole enum is new */
en = (Form_pg_enum) GETSTRUCT(enumval_tup);
enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));
diff --git a/src/include/catalog/pg_enum.h b/src/include/catalog/pg_enum.h
index 5938ba5..f8a492b 100644
--- a/src/include/catalog/pg_enum.h
+++ b/src/include/catalog/pg_enum.h
@@ -69,5 +69,7 @@ extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
bool skipIfExists);
extern void RenameEnumLabel(Oid enumTypeOid,
const char *oldVal, const char *newVal);
+extern bool EnumBlacklisted(Oid enum_id);
+extern void InitEnumBlacklist(void);
#endif /* PG_ENUM_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
