Re: BUG #14825: enum type: unsafe use?

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