Re: alter enum add value if not exists

Here is a patch for this feature, which should alleviate some of the
woes caused by adding labels not being transactional (and thus not
allowing for the catching of errors).

(Also available on the add_enum_ine branch at
<https://bitbucket.org/adunstan/pgdevel>)

cheers

andrew

On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Here is a patch for this feature, which should alleviate some of the woes
> caused by adding labels not being transactional (and thus not allowing for
> the catching of errors).

I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?

I don't recall the exact discussion, but was there something about
enum labels that made it impossible to make them transactional, or was
it just "lots of work, let's do that later instead" to get the feature
in? If the second, does anyone have plans to fix it? It is a quite
annoying limitation :(

That said, this functionality would be useful even *if* the enum label
addition was made transactional...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 08/23/2012 06:47 AM, Magnus Hagander wrote:
> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> Here is a patch for this feature, which should alleviate some of the woes
>> caused by adding labels not being transactional (and thus not allowing for
>> the catching of errors).
> I haven't actually checked the code in detail, but if it's not
> transactional, how does it actually prevent race conditions? Doesn't
> it at least have to do it's check *after* the enum is locked?

Well, you can't remove a label, and if the test succeeds it results in
your doing nothing, so my possibly naive thinking was that that wasn't
necessary. But I could easily be wrong :-)

>
> I don't recall the exact discussion, but was there something about
> enum labels that made it impossible to make them transactional, or was
> it just "lots of work, let's do that later instead" to get the feature
> in? If the second, does anyone have plans to fix it? It is a quite
> annoying limitation :(

I don't know of any plans to fix it.

>
> That said, this functionality would be useful even *if* the enum label
> addition was made transactional...
>

Right.

cheers

andrew

On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 08/23/2012 06:47 AM, Magnus Hagander wrote:
>>
>> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>>>
>>> Here is a patch for this feature, which should alleviate some of the woes
>>> caused by adding labels not being transactional (and thus not allowing
>>> for
>>> the catching of errors).
>>
>> I haven't actually checked the code in detail, but if it's not
>> transactional, how does it actually prevent race conditions? Doesn't
>> it at least have to do it's check *after* the enum is locked?
>
>
>
> Well, you can't remove a label, and if the test succeeds it results in your
> doing nothing, so my possibly naive thinking was that that wasn't necessary.
> But I could easily be wrong :-)

Ah, good point.
But still: what if:

Session A checks if the label is present, it's not.
Session B checks if the label is present, it's not.
Session A locks the enum, and adds the label, then releases lock.
Session B locks the enum, and tries to add it -- and you still get a failure.

It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?

Or am I missing something? (I probably am :D - I still haven't looked
at it in detail)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 08/23/2012 07:39 AM, Magnus Hagander wrote:
> On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 08/23/2012 06:47 AM, Magnus Hagander wrote:
>>> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)net>
>>> wrote:
>>>> Here is a patch for this feature, which should alleviate some of the woes
>>>> caused by adding labels not being transactional (and thus not allowing
>>>> for
>>>> the catching of errors).
>>> I haven't actually checked the code in detail, but if it's not
>>> transactional, how does it actually prevent race conditions? Doesn't
>>> it at least have to do it's check *after* the enum is locked?
>>
>>
>> Well, you can't remove a label, and if the test succeeds it results in your
>> doing nothing, so my possibly naive thinking was that that wasn't necessary.
>> But I could easily be wrong :-)
> Ah, good point.
> But still: what if:
>
> Session A checks if the label is present, it's not.
> Session B checks if the label is present, it's not.
> Session A locks the enum, and adds the label, then releases lock.
> Session B locks the enum, and tries to add it -- and you still get a failure.
>
> It doesn't break, of course ,since it's protected by the unique index.
> But aren't you at risk of getting the very error message you're trying
> to avoid?
>
> Or am I missing something? (I probably am :D - I still haven't looked
> at it in detail)
>

Yeah, looking further this was probably a thinko on my part. Thanks for
noticing. I've moved the test down so it's done right after the lock is
acquired. Revised patch attached.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 08/23/2012 07:39 AM, Magnus Hagander wrote:
>> It doesn't break, of course ,since it's protected by the unique index.
>> But aren't you at risk of getting the very error message you're trying
>> to avoid?

> Yeah, looking further this was probably a thinko on my part. Thanks for
> noticing. I've moved the test down so it's done right after the lock is
> acquired. Revised patch attached.

This patch looks sane as far as it goes. It strikes me though that if
we're going to invent an opt_if_not_exists production in the grammar,
there are a lot of other places where it should be used too, for
consistency if nothing else.

However, it would be reasonable to do that mop-up as a separate
commit. If you prefer, commit what you've got and then I'll see
about the other thing.

regards, tom lane

On 09/20/2012 06:34 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 08/23/2012 07:39 AM, Magnus Hagander wrote:
>>> It doesn't break, of course ,since it's protected by the unique index.
>>> But aren't you at risk of getting the very error message you're trying
>>> to avoid?
>> Yeah, looking further this was probably a thinko on my part. Thanks for
>> noticing. I've moved the test down so it's done right after the lock is
>> acquired. Revised patch attached.
> This patch looks sane as far as it goes. It strikes me though that if
> we're going to invent an opt_if_not_exists production in the grammar,
> there are a lot of other places where it should be used too, for
> consistency if nothing else.
>
> However, it would be reasonable to do that mop-up as a separate
> commit. If you prefer, commit what you've got and then I'll see
> about the other thing.
>
>

The enum piece is now committed.

I agree cleaning this up would be a good idea.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> The enum piece is now committed.

BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
options we have issue a NOTICE when skipping the CREATE action. Is
there a reason this shouldn't do the same?

regards, tom lane

On 09/22/2012 05:39 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> The enum piece is now committed.
> BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
> options we have issue a NOTICE when skipping the CREATE action. Is
> there a reason this shouldn't do the same?
>
>

Not really, I guess we should for the sake of consistency, although TBH
I find it just useless noise and rather wish we hadn't started the trend
when we did the first DROP IF NOT EXISTS stuff.

I'll add it.

cheers

andrew

On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
>
> On 09/22/2012 05:39 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> The enum piece is now committed.
>> BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
>> options we have issue a NOTICE when skipping the CREATE action. Is
>> there a reason this shouldn't do the same?
>>
>>
>
> Not really, I guess we should for the sake of consistency, although TBH
> I find it just useless noise and rather wish we hadn't started the
> trend when we did the first DROP IF NOT EXISTS stuff.
Time for a GUC

existence_notice = none | exists | not_exists | all

?

Cheers,
Hannu Krosing

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 09/22/2012 05:39 PM, Tom Lane wrote:
>> BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
>> options we have issue a NOTICE when skipping the CREATE action. Is
>> there a reason this shouldn't do the same?

> I'll add it.

I'm on it already.

regards, tom lane

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
>> Not really, I guess we should for the sake of consistency, although TBH
>> I find it just useless noise and rather wish we hadn't started the
>> trend when we did the first DROP IF NOT EXISTS stuff.

> Time for a GUC
> existence_notice = none | exists | not_exists | all

Not another one :-( ... isn't client_min_messages good enough?

We sort of had this discussion before w.r.t. the notices about creating
primary key indexes etc. I wonder whether we should make a formal
effort to split NOTICE message level into, say, NOTICE and NOVICE
levels, where the latter contains all the "training wheels" stuff that
experienced users would really rather not see. Or maybe just redefine
NOTICE as meaning novice-oriented messages, and push anything that
doesn't seem to fit that categorization into another existing message
level?

regards, tom lane

I wrote:
> ... It strikes me though that if
> we're going to invent an opt_if_not_exists production in the grammar,
> there are a lot of other places where it should be used too, for
> consistency if nothing else.

BTW, I tried to do this and realized that it doesn't work, because IF
is not a reserved word. The only way that opt_if_not_exists isn't
ambiguous is if it must appear before something that's not an
identifier, which is to say it works in ALTER TYPE ADD VALUE ... Sconst
and nowhere else. Otherwise you have to spell it out with duplicate
productions so that bison doesn't have to make a shift/reduce decision
till it's seen the whole phrase.

If we're ever forced to make IF reserved for other reasons, we could
clean up a lot of both IF EXISTS and IF NOT EXISTS productions.

There are other ways we could refactor the productions involved to
reduce duplication; for instance I think that we could make it work for
CREATE TABLE IF NOT EXISTS by defining a nonterminal that expands to
either "qualified_name" or "IF NOT EXISTS qualified_name". But that
seems ugly enough to not be much of an improvement, not least because
the nonterminal would need to return two separate pieces of info,
and that's not terribly easy in bison.

regards, tom lane

On 09/22/2012 07:05 PM, Tom Lane wrote:
> I wrote:
>> ... It strikes me though that if
>> we're going to invent an opt_if_not_exists production in the grammar,
>> there are a lot of other places where it should be used too, for
>> consistency if nothing else.
> BTW, I tried to do this and realized that it doesn't work, because IF
> is not a reserved word. The only way that opt_if_not_exists isn't
> ambiguous is if it must appear before something that's not an
> identifier, which is to say it works in ALTER TYPE ADD VALUE ... Sconst
> and nowhere else. Otherwise you have to spell it out with duplicate
> productions so that bison doesn't have to make a shift/reduce decision
> till it's seen the whole phrase.
>
> If we're ever forced to make IF reserved for other reasons, we could
> clean up a lot of both IF EXISTS and IF NOT EXISTS productions.
>
> There are other ways we could refactor the productions involved to
> reduce duplication; for instance I think that we could make it work for
> CREATE TABLE IF NOT EXISTS by defining a nonterminal that expands to
> either "qualified_name" or "IF NOT EXISTS qualified_name". But that
> seems ugly enough to not be much of an improvement, not least because
> the nonterminal would need to return two separate pieces of info,
> and that's not terribly easy in bison.
>
>

:-(

I remember running into this when I did the DINE stuff. I was actually
pleasantly surprised that it worked with the enum command.

cheers

andrew

On Sat, Sep 22, 2012 at 06:08:19PM -0400, Tom Lane wrote:
> Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> > On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
> >> Not really, I guess we should for the sake of consistency, although TBH
> >> I find it just useless noise and rather wish we hadn't started the
> >> trend when we did the first DROP IF NOT EXISTS stuff.
>
> > Time for a GUC
> > existence_notice = none | exists | not_exists | all
>
> Not another one :-( ... isn't client_min_messages good enough?
>
> We sort of had this discussion before w.r.t. the notices about creating
> primary key indexes etc. I wonder whether we should make a formal
> effort to split NOTICE message level into, say, NOTICE and NOVICE
> levels, where the latter contains all the "training wheels" stuff that
> experienced users would really rather not see. Or maybe just redefine
> NOTICE as meaning novice-oriented messages, and push anything that
> doesn't seem to fit that categorization into another existing message
> level?

I have always wanted a "novice" level, so we could warn about things
like unjoined tables.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +