From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: BUG #14825: enum type: unsafe use?
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: balazs@obiserver.hu, pgsql-bugs@postgresql.org,
 pgsql-hackers@postgresql.org
References: <20170922185904.1448.16585@wrigleys.postgresql.org>
 <15165.1506116768@sss.pgh.pa.us>
 <3cece400-af56-7145-2e65-f044de1b0b68@2ndQuadrant.com>
 <28968.1506136792@sss.pgh.pa.us>
 <f29e2d7e-7d71-b3b7-2fed-d52bddd8ca61@2ndQuadrant.com>
 <24380.1506179805@sss.pgh.pa.us> <16272.1506189656@sss.pgh.pa.us>
 <928f5974-151f-0273-0221-fac213e7624f@2ndQuadrant.com>
 <19137.1506196325@sss.pgh.pa.us>
Message-ID: <4942e2c0-3302-f496-d4c1-a967f77d0143@2ndQuadrant.com>
Date: Sat, 23 Sep 2017 17:45:46 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <19137.1506196325@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------1542897F09D0D318E466EE70"
Content-Language: en-MW
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org

This is a multi-part message in MIME format.
--------------1542897F09D0D318E466EE70
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


On 09/23/2017 03:52 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@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


--------------1542897F09D0D318E466EE70
Content-Type: text/x-patch;
 name="fix_enum_wip.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix_enum_wip.patch"

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 */

--------------1542897F09D0D318E466EE70
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--------------1542897F09D0D318E466EE70--