Thread

1. remove upsert example from docs

   Merlin Moncure <mmoncure@gmail.com> — 2010-08-05T17:44:52Z

   Attached is a patch to remove the upsert example from the pl/pgsql
   documentation.  It has a serious bug (see:
   http://www.spinics.net/lists/pgsql/msg112560.html) which is nontrivial
   to fix.  IMNSHO, our code examples should encourage good practices and
   style.
   
   The 'correct' way to do race free upsert is to take a table lock first
   -- you don't have to loop or open a subtransaction.  A high
   concurrency version is nice but is more of a special case solution (it
   looks like concurrent MERGE might render the issue moot anyways).
   
   merlin
   

2. Re: remove upsert example from docs

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-08-05T18:03:25Z

   Merlin Moncure <mmoncure@gmail.com> wrote:
    
   > Attached is a patch to remove the upsert example from the pl/pgsql
   > documentation.  It has a serious bug (see:
   > http://www.spinics.net/lists/pgsql/msg112560.html) which is
   > nontrivial to fix.  IMNSHO, our code examples should encourage
   > good practices and style.
   > 
   > The 'correct' way to do race free upsert is to take a table lock
   > first -- you don't have to loop or open a subtransaction.  A high
   > concurrency version is nice but is more of a special case solution
   > (it looks like concurrent MERGE might render the issue moot
   > anyways).
    
   Of course, this can be done safely without a table lock if either or
   both of the concurrency patches (one by Florian, one by Dan and
   myself) get committed, so maybe we should wait to see whether either
   of them makes it before adjusting the docs on this point -- at least
   for 9.1.  Taking a broken example out of 9.0 and back branches might
   make sense....
    
   -Kevin
   
   

3. Re: remove upsert example from docs

   Greg Sabino Mullane <greg@turnstep.com> — 2010-08-05T18:06:36Z

   -----BEGIN PGP SIGNED MESSAGE-----
   Hash: RIPEMD160
   
   
   > Attached is a patch to remove the upsert example from the pl/pgsql
   > documentation.  It has a serious bug (see:
   > http://www.spinics.net/lists/pgsql/msg112560.html) which is nontrivial
   > to fix.  IMNSHO, our code examples should encourage good practices and
   > style.
   
   No, removing is a bad idea, as it's referenced from here to the North 
   Pole and back. Better would simply be a warning about the non uniqueness 
   of the unique constraint message.
   
   > The 'correct' way to do race free upsert is to take a table lock first
   > -- you don't have to loop or open a subtransaction.  A high
   > concurrency version is nice but is more of a special case solution (it
   > looks like concurrent MERGE might render the issue moot anyways).
   
   I think anything doing table locks should be the "special case solution" 
   as production systems generally avoid full table locks like the plague.
   The existing solution works fine as long as we explain that caveat (which 
   is a little bit of a corner case, else we'd have heard more complaints 
   before now).
   
   - -- 
   Greg Sabino Mullane greg@turnstep.com
   End Point Corporation http://www.endpoint.com/
   PGP Key: 0x14964AC8 201008051402
   http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
   -----BEGIN PGP SIGNATURE-----
   
   iEYEAREDAAYFAkxa/XgACgkQvJuQZxSWSsjTbACfcjrsBVXCOGUb6foARfNIztSo
   AswAn0bNttP8XOs/2tw6jFsSa0cZkq7e
   =HUcq
   -----END PGP SIGNATURE-----
   
   
   
   

4. Re: remove upsert example from docs

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-05T18:09:22Z

   Merlin Moncure <mmoncure@gmail.com> writes:
   > Attached is a patch to remove the upsert example from the pl/pgsql
   > documentation.  It has a serious bug (see:
   > http://www.spinics.net/lists/pgsql/msg112560.html) which is nontrivial
   > to fix.  IMNSHO, our code examples should encourage good practices and
   > style.
   
   I was not persuaded that there's a real bug in practice.  IMO, his
   problem was a broken trigger not broken upsert logic.  Even if we
   conclude this is unsafe, simply removing the example is of no help to
   anyone.  A more useful response would be to supply a correct example.
   
   			regards, tom lane
   
   

5. Re: remove upsert example from docs

   Andrew Dunstan <andrew@dunslane.net> — 2010-08-05T18:24:40Z

   
   On 08/05/2010 02:09 PM, Tom Lane wrote:
   > Merlin Moncure<mmoncure@gmail.com>  writes:
   >> Attached is a patch to remove the upsert example from the pl/pgsql
   >> documentation.  It has a serious bug (see:
   >> http://www.spinics.net/lists/pgsql/msg112560.html) which is nontrivial
   >> to fix.  IMNSHO, our code examples should encourage good practices and
   >> style.
   > I was not persuaded that there's a real bug in practice.  IMO, his
   > problem was a broken trigger not broken upsert logic.  Even if we
   > conclude this is unsafe, simply removing the example is of no help to
   > anyone.  A more useful response would be to supply a correct example.
   >
   > 			
   
   Yeah, that's how it struck me just now. Maybe we should document that 
   the inserts had better not fire a trigger that could cause an uncaught 
   uniqueness violation exception. You could also possibly usefully prevent 
   infinite looping in such cases by using a limited loop rather an 
   unlimited loop.
   
   cheers
   
   andrew
   
   

6. Re: remove upsert example from docs

   Merlin Moncure <mmoncure@gmail.com> — 2010-08-05T18:44:50Z

   On Thu, Aug 5, 2010 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Merlin Moncure <mmoncure@gmail.com> writes:
   >> Attached is a patch to remove the upsert example from the pl/pgsql
   >> documentation.  It has a serious bug (see:
   >> http://www.spinics.net/lists/pgsql/msg112560.html) which is nontrivial
   >> to fix.  IMNSHO, our code examples should encourage good practices and
   >> style.
   >
   > I was not persuaded that there's a real bug in practice.  IMO, his
   > problem was a broken trigger not broken upsert logic.  Even if we
   > conclude this is unsafe, simply removing the example is of no help to
   > anyone.
   
   Well, the error handler is assuming that the unique_volation is coming
   from the insert made within the loop.  This is obviously not a safe
   assumption in an infinite loop context.  It should be double checking
   where the error was being thrown from -- but the only way I can think
   of to do that is to check sqlerrm.  Or you arguing that if you're
   doing this, all dependent triggers must not throw unique violations up
   the exception chain?
   
   Looping N times and punting is meh: since you have to now check in the
   app, why have this mechanism at all?
   
   > A more useful response would be to supply a correct example.
   Agree: I'd go further I would argue to supply both the 'safe' and
   'high concurrency (with caveat)' way.  I'm not saying the example is
   necessarily bad, just that it's maybe not a good thing to be pointing
   as a learning example without qualifications.  Then you get a lesson
   both on upsert methods and defensive error handling (barring
   objection, I'll provide that).
   
   merlin
   
   

7. Re: remove upsert example from docs

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-05T18:53:28Z

   Merlin Moncure <mmoncure@gmail.com> writes:
   > On Thu, Aug 5, 2010 at 2:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> I was not persuaded that there's a real bug in practice. IMO, his
   >> problem was a broken trigger not broken upsert logic. Even if we
   >> conclude this is unsafe, simply removing the example is of no help to
   >> anyone.
   
   > Well, the error handler is assuming that the unique_volation is coming
   > from the insert made within the loop.  This is obviously not a safe
   > assumption in an infinite loop context.
   
   Well, that's a fair point.  Perhaps we should just add a note that if
   there are any triggers that do additional inserts/updates, the exception
   catcher had better check which table the unique_violation is being
   reported for.
   
   >> A more useful response would be to supply a correct example.
   
   > Agree: I'd go further I would argue to supply both the 'safe' and
   > 'high concurrency (with caveat)' way.  I'm not saying the example is
   > necessarily bad, just that it's maybe not a good thing to be pointing
   > as a learning example without qualifications.  Then you get a lesson
   > both on upsert methods and defensive error handling (barring
   > objection, I'll provide that).
   
   Have at it.
   
   			regards, tom lane
   
   

8. Re: remove upsert example from docs

   Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-08-07T09:42:58Z

   On 8/5/2010 9:44 PM, Merlin Moncure wrote:
   > On Thu, Aug 5, 2010 at 2:09 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
   >> I was not persuaded that there's a real bug in practice.  IMO, his
   >> problem was a broken trigger not broken upsert logic.  Even if we
   >> conclude this is unsafe, simply removing the example is of no help to
   >> anyone.
   >
   > Well, the error handler is assuming that the unique_volation is coming
   > from the insert made within the loop.  This is obviously not a safe
   > assumption in an infinite loop context.  It should be double checking
   > where the error was being thrown from -- but the only way I can think
   > of to do that is to check sqlerrm.
   
   Yeah, this is a known problem with our exception system.  If there was 
   an easy and reliable way of knowing where the exception came from, I'm 
   sure the example would include that.
   
   > Or you arguing that if you're
   > doing this, all dependent triggers must not throw unique violations up
   > the exception chain?
   
   If he isn't, I am.  I'm pretty sure you can break every example in the 
   docs with a trigger (or a rule) you haven't thought through.
   
   >> A more useful response would be to supply a correct example.
   > Agree: I'd go further I would argue to supply both the 'safe' and
   > 'high concurrency (with caveat)' way.  I'm not saying the example is
   > necessarily bad, just that it's maybe not a good thing to be pointing
   > as a learning example without qualifications.  Then you get a lesson
   > both on upsert methods and defensive error handling (barring
   > objection, I'll provide that).
   
   The problem with the "safe" way is that it's not safe if called in a 
   transaction with isolation level set to SERIALIZABLE.
   
   
   Regards,
   Marko Tiikkaja
   
   

9. Re: remove upsert example from docs

   Bruce Momjian <bruce@momjian.us> — 2011-02-17T18:37:01Z

   Marko Tiikkaja wrote:
   > On 8/5/2010 9:44 PM, Merlin Moncure wrote:
   > > On Thu, Aug 5, 2010 at 2:09 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
   > >> I was not persuaded that there's a real bug in practice.  IMO, his
   > >> problem was a broken trigger not broken upsert logic.  Even if we
   > >> conclude this is unsafe, simply removing the example is of no help to
   > >> anyone.
   > >
   > > Well, the error handler is assuming that the unique_volation is coming
   > > from the insert made within the loop.  This is obviously not a safe
   > > assumption in an infinite loop context.  It should be double checking
   > > where the error was being thrown from -- but the only way I can think
   > > of to do that is to check sqlerrm.
   > 
   > Yeah, this is a known problem with our exception system.  If there was 
   > an easy and reliable way of knowing where the exception came from, I'm 
   > sure the example would include that.
   > 
   > > Or you arguing that if you're
   > > doing this, all dependent triggers must not throw unique violations up
   > > the exception chain?
   > 
   > If he isn't, I am.  I'm pretty sure you can break every example in the 
   > docs with a trigger (or a rule) you haven't thought through.
   > 
   > >> A more useful response would be to supply a correct example.
   > > Agree: I'd go further I would argue to supply both the 'safe' and
   > > 'high concurrency (with caveat)' way.  I'm not saying the example is
   > > necessarily bad, just that it's maybe not a good thing to be pointing
   > > as a learning example without qualifications.  Then you get a lesson
   > > both on upsert methods and defensive error handling (barring
   > > objection, I'll provide that).
   > 
   > The problem with the "safe" way is that it's not safe if called in a 
   > transaction with isolation level set to SERIALIZABLE.
   
   Good analysis.  Documentation patch attached and applied.
   
   -- 
     Bruce Momjian  <bruce@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +
   

10. Re: remove upsert example from docs

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-17T18:47:18Z

    On 2011-02-17 8:37 PM +0200, Bruce Momjian wrote:
    > Marko Tiikkaja wrote:
    >> The problem with the "safe" way is that it's not safe if called in a
    >> transaction with isolation level set to SERIALIZABLE.
    >
    > Good analysis.  Documentation patch attached and applied.
    
    The "safe way" I was referring to above was the LOCK TABLE method, not 
    the one described in the documentation, so the remark about READ 
    COMMITTED in the patch should be removed.  The first part looks fine though.
    
    
    Regards,
    Marko Tiikkaja
    
    

11. Re: remove upsert example from docs

    Bruce Momjian <bruce@momjian.us> — 2011-02-17T19:24:47Z

    Marko Tiikkaja wrote:
    > On 2011-02-17 8:37 PM +0200, Bruce Momjian wrote:
    > > Marko Tiikkaja wrote:
    > >> The problem with the "safe" way is that it's not safe if called in a
    > >> transaction with isolation level set to SERIALIZABLE.
    > >
    > > Good analysis.  Documentation patch attached and applied.
    > 
    > The "safe way" I was referring to above was the LOCK TABLE method, not 
    > the one described in the documentation, so the remark about READ 
    > COMMITTED in the patch should be removed.  The first part looks fine though.
    
    OK, sentence removed.  Thanks.
    
    -- 
      Bruce Momjian  <bruce@momjian.us>        http://momjian.us
      EnterpriseDB                             http://enterprisedb.com
    
      + It's impossible for everything to be true. +