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