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: BUG #14825: enum type: unsafe use?
Date: Sat, 23 Sep 2017 17:45:46 -0400
Message-ID: <4942e2c0-3302-f496-d4c1-a967f77d0143@2ndQuadrant.com> (raw)
In-Reply-To: <19137.1506196325@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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-bugs>
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugsAttachments:fix_enum_wip.patch (text/x-patch, 2.7K) downloaddiff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.cindex 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.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..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 */
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: BUG #14825: enum type: unsafe use?
In-Reply-To: <4942e2c0-3302-f496-d4c1-a967f77d0143@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