From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: [BUGS] 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>
 <4942e2c0-3302-f496-d4c1-a967f77d0143@2ndQuadrant.com>
 <25662.1506204381@sss.pgh.pa.us>
Message-ID: <9479302a-09b8-4264-f9c4-514b65f18547@2ndQuadrant.com>
Date: Sun, 24 Sep 2017 16:19:37 -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: <25662.1506204381@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------7EA76BD6D3EC435163BA1FDE"
Content-Language: en-MW
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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


On 09/23/2017 06:06 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> 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).
> For the blacklist, I agree we could just ignore subtransactions: all
> subtransaction levels are equally uncommitted for this purpose, and
> leaving entries from failed subtransactions in place seems like a
> non-issue, since they'd never be referenced again.  (Well, barring OID
> wraparound and an enum-value-OID collision while the transaction runs,
> but I think we can ignore that as having probability epsilon.)
>
> But you need to actually put the table in TopTransactionContext, not
> CurTransactionContext ;-).  Also, I don't think you need an init call
> so much as an end-of-transaction cleanup call.  Maybe call it
> AtEOXactEnum(), for consistency with other functions called in the
> same area.
>
>> 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?
> Dynahash tables are self-expanding, so I don't see a need to stress about
> that too much.  Anything in 10-100 seems reasonable for initial size.
>


OK, here's the finished patch. It has a pretty small footprint all
things considered, and I think it guarantees that nothing that could be
done in this area in 9.6 will be forbidden. That's probably enough to
get us to 10 without having to revert the whole thing, ISTM, and we can
leave any further refinement to the next release.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--------------7EA76BD6D3EC435163BA1FDE
Content-Type: text/x-patch;
 name="fix-enums-3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix-enums-3.patch"

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 93dca7a..1d6f774 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -32,6 +32,7 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "catalog/pg_enum.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
 #include "commands/tablecmds.h"
@@ -2126,6 +2127,7 @@ CommitTransaction(void)
 	smgrDoPendingDeletes(true);
 
 	AtCommit_Notify();
+	AtEOXact_Enum();
 	AtEOXact_GUC(true, 1);
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
@@ -2405,6 +2407,7 @@ PrepareTransaction(void)
 
 	/* PREPARE acts the same as COMMIT as far as GUC is concerned */
 	AtEOXact_GUC(true, 1);
+	AtEOXact_Enum();
 	AtEOXact_SPI(true);
 	AtEOXact_on_commit_actions(true);
 	AtEOXact_Namespace(true, false);
@@ -2606,6 +2609,7 @@ AbortTransaction(void)
 							 false, true);
 		smgrDoPendingDeletes(false);
 
+		AtEOXact_Enum();
 		AtEOXact_GUC(false, 1);
 		AtEOXact_SPI(false);
 		AtEOXact_on_commit_actions(false);
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index fe61d4d..3056f68 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,8 @@ 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 the current transaction by AddEnumLabel */
+static HTAB *enum_blacklist = NULL;
 
 /*
  * EnumValuesCreate
@@ -460,8 +464,49 @@ restart:
 	heap_freetuple(enum_tup);
 
 	heap_close(pg_enum, RowExclusiveLock);
+
+	/* Set up the 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 = TopTransactionContext;
+		enum_blacklist = hash_create("Enum blacklist for current transaction",
+						   32,
+						   &hash_ctl,
+						   HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	}
+
+	/* Add the new value to the blacklist */
+	(void) hash_search(enum_blacklist, &newOid, HASH_ENTER, NULL);
 }
 
+/* Test if the enum is on the blacklist */
+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;
+}
+
+/*
+ * Clean up the blacklist hash at the end of the transaction. The memory will
+ * have been deallocated, so all we need to do is set the pointer back to
+ * NULL for the next transaction.
+ */
+void
+AtEOXact_Enum(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..dff3d2f 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 AtEOXact_Enum(void);
 
 #endif							/* PG_ENUM_H */
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 0e60304..96345e4 100644
--- a/src/test/regress/expected/enum.out
+++ b/src/test/regress/expected/enum.out
@@ -648,6 +648,29 @@ SELECT enum_range(null::bogus);
 (1 row)
 
 ROLLBACK;
+-- check that we don't forbid anything except use of a new value on an
+-- existing enum
+BEGIN;
+CREATE ROLE newowner;
+CREATE TYPE bogus AS ENUM('good','bad','ugly');
+ALTER TYPE bogus OWNER TO newowner;
+select enum_range(null::bogus);
+   enum_range    
+-----------------
+ {good,bad,ugly}
+(1 row)
+
+ROLLBACK;
+BEGIN;
+CREATE ROLE newowner;
+ALTER TYPE rainbow OWNER TO newowner;
+select enum_range(null::rainbow);
+                enum_range                 
+-------------------------------------------
+ {crimson,orange,yellow,green,blue,purple}
+(1 row)
+
+ROLLBACK;
 --
 -- Cleanup
 --
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index d7e8714..2129f64 100644
--- a/src/test/regress/sql/enum.sql
+++ b/src/test/regress/sql/enum.sql
@@ -311,6 +311,22 @@ ALTER TYPE bogus ADD VALUE 'ugly';
 SELECT enum_range(null::bogus);
 ROLLBACK;
 
+-- check that we don't forbid anything except use of a new value on an
+-- existing enum
+
+BEGIN;
+CREATE ROLE newowner;
+CREATE TYPE bogus AS ENUM('good','bad','ugly');
+ALTER TYPE bogus OWNER TO newowner;
+select enum_range(null::bogus);
+ROLLBACK;
+
+BEGIN;
+CREATE ROLE newowner;
+ALTER TYPE rainbow OWNER TO newowner;
+select enum_range(null::rainbow);
+ROLLBACK;
+
 --
 -- Cleanup
 --

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


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

--------------7EA76BD6D3EC435163BA1FDE--