Subject: Summary table trigger example race condition

From: "Jim C. Nasby" <[email-masked]>
Date: Thu, 5 Jan 2006 15:36:45 -0600

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      [email-masked]
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461
Index: doc/src/sgml/plpgsql.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/plpgsql.sgml,v
retrieving revision 1.83
diff -u -r1.83 plpgsql.sgml
--- doc/src/sgml/plpgsql.sgml	29 Dec 2005 04:02:32 -0000	1.83
+++ doc/src/sgml/plpgsql.sgml	5 Jan 2006 21:36:14 -0000
@@ -3017,31 +3017,33 @@
 
         -- There might have been no row with this time_key (e.g new data!).
         IF (NOT FOUND) THEN
-            BEGIN
-                INSERT INTO sales_summary_bytime (
-                            time_key, 
-                            amount_sold, 
-                            units_sold, 
-                            amount_cost)
-                    VALUES ( 
-                            delta_time_key,
-                            delta_amount_sold,
-                            delta_units_sold,
-                            delta_amount_cost
-                           );
-            EXCEPTION
-                --
-                -- Catch race condition when two transactions are adding data
-                -- for a new time_key.
-                --
-                WHEN UNIQUE_VIOLATION THEN
-                    UPDATE sales_summary_bytime
-                        SET amount_sold = amount_sold + delta_amount_sold,
-                            units_sold = units_sold + delta_units_sold,
-                            amount_cost = amount_cost + delta_amount_cost
-                        WHERE time_key = delta_time_key;
-
-            END;
+            &lt;&lt;insert_update&gt;&gt;
+            LOOP
+                UPDATE sales_summary_bytime
+                    SET amount_sold = amount_sold + delta_amount_sold,
+                        units_sold = units_sold + delta_units_sold,
+                        amount_cost = amount_cost + delta_amount_cost
+                    WHERE time_key = delta_time_key;
+
+                EXIT insert_update WHEN found;    
+
+                BEGIN
+                    INSERT INTO sales_summary_bytime (
+                                time_key, 
+                                amount_sold, 
+                                units_sold, 
+                                amount_cost)
+                        VALUES ( 
+                                delta_time_key,
+                                delta_amount_sold,
+                                delta_units_sold,
+                                delta_amount_cost
+                               );
+                EXCEPTION
+                    WHEN UNIQUE_VIOLATION THEN
+                        -- do nothing
+                END;
+            END LOOP insert_update;
         END IF;
         RETURN NULL;
 
@@ -3051,6 +3053,16 @@
 CREATE TRIGGER maint_sales_summary_bytime
 AFTER INSERT OR UPDATE OR DELETE ON sales_fact
     FOR EACH ROW EXECUTE PROCEDURE maint_sales_summary_bytime();
+
+INSERT INTO sales_fact VALUES(1,1,1,10,3,15);
+INSERT INTO sales_fact VALUES(1,2,1,20,5,35);
+INSERT INTO sales_fact VALUES(2,2,1,40,15,135);
+INSERT INTO sales_fact VALUES(2,3,1,10,1,13);
+SELECT * FROM sales_summary_bytime;
+DELETE FROM sales_fact WHERE product_key = 1;
+SELECT * FROM sales_summary_bytime;
+UPDATE sales_fact SET units_sold = units_sold * 2;
+SELECT * FROM sales_summary_bytime;
 </programlisting>
    </example>

---

From: Mark Kirkwood <[email-masked]>
Date: Fri, 06 Jan 2006 14:00:34 +1300

> 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

---

From: "Jim C. Nasby" <[email-masked]>
Date: Thu, 5 Jan 2006 21:04:15 -0600

> 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      [email-masked]
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461
Index: doc/src/sgml/plpgsql.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/plpgsql.sgml,v
retrieving revision 1.83
diff -c -r1.83 plpgsql.sgml
*** doc/src/sgml/plpgsql.sgml	29 Dec 2005 04:02:32 -0000	1.83
--- doc/src/sgml/plpgsql.sgml	6 Jan 2006 03:03:11 -0000
***************
*** 3007,3022 ****
          END IF;

!         -- Update the summary row with the new values.
!         UPDATE sales_summary_bytime
!             SET amount_sold = amount_sold + delta_amount_sold,
!                 units_sold = units_sold + delta_units_sold,
!                 amount_cost = amount_cost + delta_amount_cost
!             WHERE time_key = delta_time_key;

-         -- There might have been no row with this time_key (e.g new data!).
-         IF (NOT FOUND) THEN
              BEGIN
                  INSERT INTO sales_summary_bytime (
                              time_key, 
--- 3007,3023 ----
          END IF;

!         -- Insert or update the summary row with the new values.
!         &lt;&lt;insert_update&gt;&gt;
!         LOOP
!             UPDATE sales_summary_bytime
!                 SET amount_sold = amount_sold + delta_amount_sold,
!                     units_sold = units_sold + delta_units_sold,
!                     amount_cost = amount_cost + delta_amount_cost
!                 WHERE time_key = delta_time_key;

              BEGIN
                  INSERT INTO sales_summary_bytime (
                              time_key, 
***************
*** 3029,3048 ****
                              delta_units_sold,
                              delta_amount_cost
                             );
              EXCEPTION
-                 --
-                 -- Catch race condition when two transactions are adding data
-                 -- for a new time_key.
-                 --
                  WHEN UNIQUE_VIOLATION THEN
!                     UPDATE sales_summary_bytime
!                         SET amount_sold = amount_sold + delta_amount_sold,
!                             units_sold = units_sold + delta_units_sold,
!                             amount_cost = amount_cost + delta_amount_cost
!                         WHERE time_key = delta_time_key;
! 
              END;
!         END IF;
          RETURN NULL;
  
      END;
--- 3030,3044 ----
                              delta_units_sold,
                              delta_amount_cost
                             );
+ 
+                 EXIT insert_update;
+ 
              EXCEPTION
                  WHEN UNIQUE_VIOLATION THEN
!                     -- do nothing
              END;
!         END LOOP insert_update;
! 
          RETURN NULL;
  
      END;
***************
*** 3051,3056 ****
--- 3047,3062 ----
  CREATE TRIGGER maint_sales_summary_bytime
  AFTER INSERT OR UPDATE OR DELETE ON sales_fact
      FOR EACH ROW EXECUTE PROCEDURE maint_sales_summary_bytime();
+ 
+ INSERT INTO sales_fact VALUES(1,1,1,10,3,15);
+ INSERT INTO sales_fact VALUES(1,2,1,20,5,35);
+ INSERT INTO sales_fact VALUES(2,2,1,40,15,135);
+ INSERT INTO sales_fact VALUES(2,3,1,10,1,13);
+ SELECT * FROM sales_summary_bytime;
+ DELETE FROM sales_fact WHERE product_key = 1;
+ SELECT * FROM sales_summary_bytime;
+ UPDATE sales_fact SET units_sold = units_sold * 2;
+ SELECT * FROM sales_summary_bytime;
  </programlisting>
     </example>

---

From: Mark Kirkwood <[email-masked]>
Date: Fri, 06 Jan 2006 16:46:26 +1300

> 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

---

From: Mark Kirkwood <[email-masked]>
Date: Sun, 08 Jan 2006 16:13:01 +1300

>> Anyway, updated patch attached.

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

---

From: "Jim C. Nasby" <[email-masked]>
Date: Tue, 10 Jan 2006 18:09:51 -0600

> UPDATE - i.e three DML statements, which are enough to handle the race 

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.

> 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      [email-masked]
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

---

From: Mark Kirkwood <[email-masked]>
Date: Wed, 11 Jan 2006 13:40:37 +1300

> What happens if someone deletes the row between the failed insert and

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

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

---

From: Mark Kirkwood <[email-masked]>
Date: Wed, 11 Jan 2006 15:18:49 +1300

> 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

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 11 Jan 2006 19:35:42 -0300

> 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.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 12 Jan 2006 21:58:54 +0100

Jim C. Nasby wrote:
> 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/

---

From: "Jim C. Nasby" <[email-masked]>
Date: Thu, 12 Jan 2006 15:58:06 -0600

> > and for unit testing the different pieces of code.

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      [email-masked]
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 4 Feb 2006 21:48:13 -0500 (EST)

Patch applied to HEAD and 8.1.X.  Thanks.

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

> Anyway, updated patch attached.

[ Attachment, skipping... ]

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

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

---

From: "Jim C. Nasby" <[email-masked]>
Date: Wed, 22 Feb 2006 18:50:32 -0600

> 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      [email-masked]
Pervasive Software      http://pervasive.com    work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf       cell: 512-569-9461

---

From: "Jim C. Nasby" <[email-masked]>
Date: Thu, 23 Feb 2006 11:59:10 -0600

> 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                [email-masked] 
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?"