Thread: 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@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Attachment
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 < and > into >) and for unit testing the different pieces of code. Anyway, updated patch attached. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Attachment
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 < and > into >) 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 < and > into >) 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@pervasive.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.
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 < and > into >) and for > unit testing the different pieces of code. > > Anyway, updated patch attached. > -- > Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.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@candle.pha.pa.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 < and > into >) 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@pervasive.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@decibel.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?"