Prev: pg_stat_user_functions' notion of user
Next: [HACKERS] Online backup cause boot failure, anyone know why?
From: "Kevin Grittner" on 5 Aug 2010 14:03
Merlin Moncure <mmoncure(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: "Greg Sabino Mullane" on 5 Aug 2010 14:06

-----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(a)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-----



--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 5 Aug 2010 14:09
Merlin Moncure <mmoncure(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Andrew Dunstan on 5 Aug 2010 14:24


On 08/05/2010 02:09 PM, Tom Lane wrote:
> Merlin Moncure<mmoncure(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Merlin Moncure on 5 Aug 2010 14:44
On Thu, Aug 5, 2010 at 2:09 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2
Prev: pg_stat_user_functions' notion of user
Next: [HACKERS] Online backup cause boot failure, anyone know why?