From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: balazs@obiserver.hu
Cc: pgsql-bugs@postgresql.org
Cc: pgsql-hackers@postgresql.org
Subject: Re: [BUGS] BUG #14825: enum type: unsafe use?
Date: Sun, 24 Sep 2017 16:19:37 -0400
Message-ID: <9479302a-09b8-4264-f9c4-514b65f18547@2ndQuadrant.com> (raw)
In-Reply-To: <25662.1506204381@sss.pgh.pa.us>
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:fix-enums-3.patch (text/x-patch, 5.5K) downloaddiff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex 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.cindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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
--
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andrew.dunstan@2ndquadrant.com, tgl@sss.pgh.pa.us, balazs@obiserver.hu, pgsql-hackers@postgresql.org
Subject: Re: [BUGS] BUG #14825: enum type: unsafe use?
In-Reply-To: <9479302a-09b8-4264-f9c4-514b65f18547@2ndQuadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox