remove upsert example from docs
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
Attachments:
rm_upsert.patchtext/x-patch; charset=US-ASCII; name=rm_upsert.patchDownload+0-39
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
-----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-----
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
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
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
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
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
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. +
Attachments:
/rtmp/upsert.difftext/x-diffDownload+7-7
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
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. +