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

On 09/23/2017 02:00 PM, Tom Lane wrote:
> I wrote:
>> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
>>> I see what you're saying, but my idea was slightly different. We would
>>> only add to the hashtable I had in mind at the bottom AddEnumLabel().
>>> Any other value, whether added in the current transaction or not, should
>>> be safe, AIUI.
>> Oh, I see: a list of enum values we need to blacklist, not whitelist.
>> Yes, that's a significantly better idea than mine, because in common
>> use-cases that would be empty or have a very small number of entries.
> Oh, wait a minute ... that's not where the problem is, really. We can
> already tell reliably whether an enum value was created in the current
> transaction: the is-it-committed check in check_safe_enum_use is
> perfectly safe for that AFAICS. The hard part of this is the part about
> "was the enum type created in the current transaction?". We could make
> that reliable with the table I proposed of enum types created in the
> current transaction, but the possible performance drag is a concern.
>
> What I understand you to be proposing is to blacklist individual
> enum values added by ALTER ADD VALUE, but *not* values created
> aboriginally by CREATE TYPE AS ENUM. (The latter are surely safe,
> because the type must disappear if they do.) However, that would
> require dropping the second part of the current documentation promise:
>
> If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
> an enum type) is executed inside a transaction block, the new value cannot
> be used until after the transaction has been committed, except in the case
> that the enum type itself was created earlier in the same transaction.
>
> We'd have to just say "it can't be used till the transaction has been
> committed", full stop. Otherwise we're right back into the problem of
> needing to detect whether the enum type is new in transaction.
>
>>> Maybe we should also keep a cache of whitelisted enums
>>> created in the current transaction.
>> What for?
> I now realize that what you probably meant here was to track enum *types*,
> not values, created in the current transaction. But if we're doing that
> then we don't really need the per-enum-value-blacklist part of it.
>
> 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.

I think that would let us keep our promise.

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.

Am I missing something?

cheers

andrew

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