Thread: 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
Attachment
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.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 HaganderMe: 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@dunslane.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@dunslane.net> wrote: > > On 08/23/2012 06:47 AM, Magnus Hagander wrote: >> >> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.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 HaganderMe: 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@dunslane.net> wrote: >> On 08/23/2012 06:47 AM, Magnus Hagander wrote: >>> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.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
Attachment
Andrew Dunstan <andrew@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@2ndQuadrant.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@2ndQuadrant.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +