Re: [PATCHES] Summary table trigger example race condition

http://www.postgresql.org/docs/current/static/plpgsql-trigger.html
example 36-4 has a race condition in the code that checks to see if a
row exists. It should use the code from example 36-1. This patch fixes
that. It also adds some commands to show what the summary table output
looks like. Unfortunately gamke html is bombing with some kind of
library error, so I can't verify that I didn't break the sgml.

BTW, should this have gone to -docs instead?
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

Jim C. Nasby wrote:
> http://www.postgresql.org/docs/current/static/plpgsql-trigger.html
> example 36-4 has a race condition in the code that checks to see if a
> row exists. It should use the code from example 36-1. This patch fixes
> that. It also adds some commands to show what the summary table output
> looks like. Unfortunately gamke html is bombing with some kind of
> library error, so I can't verify that I didn't break the sgml.
>
> BTW, should this have gone to -docs instead?

Your SGML builds fine for me.

However, I think the actual change is not quite right - after running
the INSERT, DELETE, UPDATE sequence at the end I see:

ware=# SELECT * FROM sales_summary_bytime;
time_key | amount_sold | units_sold | amount_cost
----------+-------------+------------+-------------
1 | 30.00 | 13 | 50.00
2 | 90.00 | 47 | 283.00
(2 rows)

ware=# select * from sales_fact;
time_key | product_key | store_key | amount_sold | units_sold |
amount_cost
----------+-------------+-----------+-------------+------------+-------------
1 | 2 | 1 | 20.00 | 10 |
35.00
2 | 2 | 1 | 40.00 | 30 |
135.00
2 | 3 | 1 | 10.00 | 2 |
13.00
(3 rows)

i.e - sales_summary_bytime and sales_fact do not agree with each other
any more! I suspect that the loop does the update even if the insert is
successful (so double counts).

BTW - Nice to see someone reading this... :-)

Best wishes

Mark

On Fri, Jan 06, 2006 at 02:00:34PM +1300, Mark Kirkwood wrote:
> However, I think the actual change is not quite right - after running

DOH! It would be good if doc/src had a better mechanism for handling
code; one that would allow for writing the code natively (so you don't
have to worry about translating < into &lt; and > into &gt;) and for
unit testing the different pieces of code.

Anyway, updated patch attached.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

Jim C. Nasby wrote:
> On Fri, Jan 06, 2006 at 02:00:34PM +1300, Mark Kirkwood wrote:
>
>>However, I think the actual change is not quite right - after running
>
>
> DOH! It would be good if doc/src had a better mechanism for handling
> code; one that would allow for writing the code natively (so you don't
> have to worry about translating < into &lt; and > into &gt;) and for
> unit testing the different pieces of code.
>

Yes it would - I usually build the SGML -> HTML, then cut the code out
of a browser session to test - the pain is waiting for the docs to build.

> Anyway, updated patch attached.
>

This one is good!

Cheers

Mark

Mark Kirkwood wrote:
> Jim C. Nasby wrote:
>
>> On Fri, Jan 06, 2006 at 02:00:34PM +1300, Mark Kirkwood wrote:
>>
>>> However, I think the actual change is not quite right - after running
>>
>>
>>
>> DOH! It would be good if doc/src had a better mechanism for handling
>> code; one that would allow for writing the code natively (so you don't
>> have to worry about translating < into &lt; and > into &gt;) and for
>> unit testing the different pieces of code.
>>
>
> Yes it would - I usually build the SGML -> HTML, then cut the code out
> of a browser session to test - the pain is waiting for the docs to build.
>
>> Anyway, updated patch attached.
>>
>
> This one is good!
>

After re-examining the original code, it looks like it was not actually
vulnerable to a race condition! (it does the UPDATE, then if not found
will do an INSERT, and handle unique violation with a repeat of the same
UPDATE - i.e three DML statements, which are enough to handle the race
in this case).

However Jim's change handles the race needing only two DML statements in
a loop, which seems much more elegant! In addition it provides a nice
example of the 'merge' style code shown in e.g 36-1.

Cheers

Mark

On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote:
> After re-examining the original code, it looks like it was not actually
> vulnerable to a race condition! (it does the UPDATE, then if not found
> will do an INSERT, and handle unique violation with a repeat of the same
> UPDATE - i.e three DML statements, which are enough to handle the race
> in this case).

What happens if someone deletes the row between the failed insert and
the second update? :)

AFAICT, example 36-1 is the only way to handle this without creating a
race condition.

> However Jim's change handles the race needing only two DML statements in
> a loop, which seems much more elegant! In addition it provides a nice
> example of the 'merge' style code shown in e.g 36-1.

What's SOP here... should I ping someone to let them know this patch
should be committed now that those who care are happy with it?
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

Jim C. Nasby wrote:
> On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote:
>
>>After re-examining the original code, it looks like it was not actually
>>vulnerable to a race condition! (it does the UPDATE, then if not found
>>will do an INSERT, and handle unique violation with a repeat of the same
>>UPDATE - i.e three DML statements, which are enough to handle the race
>>in this case).
>
>
> What happens if someone deletes the row between the failed insert and
> the second update? :)
>

In this example the rows in the summary table never get deleted by
DELETE operations on that main one - the trigger just decrements the
various amounts - i.e DELETE becomes UPDATE, so no problem there.

> AFAICT, example 36-1 is the only way to handle this without creating a
> race condition.
>

For the general case indeed you are correct - a good reason for this
change :-). In addition, that fact that it is actually quite difficult
to be sure that any race condition is actually being handled makes
(another) good reason for using the most robust method in the 'official'
examples.

Best wishes

Mark

Mark Kirkwood wrote:
> Jim C. Nasby wrote:
>
>> On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote:
>>
>>
>>
>> What happens if someone deletes the row between the failed insert and
>> the second update? :)
>>
>
> In this example the rows in the summary table never get deleted by
> DELETE operations on that main one - the trigger just decrements the
> various amounts - i.e DELETE becomes UPDATE, so no problem there.
>

Sorry Jim, I just realized you probably meant someone directly deleting
rows in the summary table itself. Well yes, that would certainly fox it!

I guess I was implicitly considering a use case where the only direct
DML on the summary table would be some sort of ETL process, which would
probably lock out other changes (and re-create the summary table data
afterwards in all likelihood).

Cheers

Mark

Mark Kirkwood wrote:

> Yes it would - I usually build the SGML -> HTML, then cut the code out
> of a browser session to test - the pain is waiting for the docs to build.

FWIW, what I do is to build a cut-down version of postgres.sgml to
include only the file you want to check. I think there is a suggestion
somewhere telling you to add a DTD line to compile only that file.

Jim C. Nasby wrote:
> DOH! It would be good if doc/src had a better mechanism for handling
> code; one that would allow for writing the code natively (so you
> don't have to worry about translating < into &lt; and > into &gt;)
> and for unit testing the different pieces of code.

<![CDATA[

weird stuff

]]>

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Thu, Jan 12, 2006 at 09:58:54PM +0100, Peter Eisentraut wrote:
> Jim C. Nasby wrote:
> > DOH! It would be good if doc/src had a better mechanism for handling
> > code; one that would allow for writing the code natively (so you
> > don't have to worry about translating < into &lt; and > into &gt;)
> > and for unit testing the different pieces of code.
>
> <![CDATA[
>
> weird stuff
>
> ]]>

While that certainly takes care of < and >, it doesn't do anything for
allowing testing of code examples.

If instead we put code snippets into seperate files that could actually
be run via psql, we could actually test the code examples that we're
including in the docs. We could also easily capture the output from that
code to include in the docs.

If things go well, I'll soon be working on a database book, and my plan
is to use this technique to (hopefully) speed up production. If that
happens I'll post the code here for folks to look at.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

Patch applied to HEAD and 8.1.X. Thanks.

---------------------------------------------------------------------------

Jim C. Nasby wrote:
> On Fri, Jan 06, 2006 at 02:00:34PM +1300, Mark Kirkwood wrote:
> > However, I think the actual change is not quite right - after running
>
> DOH! It would be good if doc/src had a better mechanism for handling
> code; one that would allow for writing the code natively (so you don't
> have to worry about translating < into &lt; and > into &gt;) and for
> unit testing the different pieces of code.
>
> Anyway, updated patch attached.
> --
> Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
> Pervasive Software http://pervasive.com work: 512-231-6117
> vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, Jan 06, 2006 at 04:46:26PM +1300, Mark Kirkwood wrote:
> Jim C. Nasby wrote:
> >On Fri, Jan 06, 2006 at 02:00:34PM +1300, Mark Kirkwood wrote:
> >
> >>However, I think the actual change is not quite right - after running
> >
> >
> >DOH! It would be good if doc/src had a better mechanism for handling
> >code; one that would allow for writing the code natively (so you don't
> >have to worry about translating < into &lt; and > into &gt;) and for
> >unit testing the different pieces of code.
> >
>
> Yes it would - I usually build the SGML -> HTML, then cut the code out
> of a browser session to test - the pain is waiting for the docs to build.

Ok, here's some *Uber Ugly* code I hacked together to do something
similar for a potential book project. It's based loosely on docbook.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

On Wed, Feb 22, 2006 at 06:50:32PM -0600, Jim C. Nasby wrote:
> Ok, here's some *Uber Ugly* code I hacked together to do something
> similar for a potential book project. It's based loosely on docbook.

Ok, no, for real this time... it's at http://jim.nasby.net/sgmlcode/

Thanks to all that pointed it out...
--
Jim C. Nasby, Database Architect decibel(at)decibel(dot)org
Give your computer some brain candy! www.distributed.net Team #1828

Windows: "Where do you want to go today?"
Linux: "Where do you want to go tomorrow?"
FreeBSD: "Are you guys coming, or what?"