Message-ID: <48B87164.4050802@soe.ucsc.edu>
Date: Fri, 29 Aug 2008 15:00:04 -0700
From: Karl Schnaitter <karlsch@soe.ucsc.edu>
User-Agent: Thunderbird 2.0.0.16 (Macintosh/20080707)
MIME-Version: 1.0
To: pgsql-patches@postgresql.org
Subject: fixing bug in combocid.c
Content-Type: multipart/mixed; boundary="------------070000090602080501090105"

This is a multi-part message in MIME format.
--------------070000090602080501090105
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

This patch is for a bug in backend/utils/time/combocid.c.

In HeapTupleHeaderAdjustCmax, the code makes the incorrect assumption 
that the raw command id corresponds to cmin, when the raw command id can 
in fact be a combo cid. It needs to be fixed to call 
HeapTupleHeaderGetCmin to get cmin. That's all there is to my patch, but 
read on for more details.

One thing that can happen is that multiple subtransactions may delete a 
tuple and each abort. Each subtransaction (except the first one) will 
store a combo cid that uses another combo cid as cmin. Then when cmin is 
accessed by a later operation, HeapTupleHeaderGetCmin will return the 
previous combo cid. In unlucky situations, the bogus cmin will be so 
high that it looks like the tuple was inserted in the future.

I have pasted an example below to show how things can go wrong. The 
second SELECT leaves out some tuples because their stored cmin is 
actually a large combo cid. The third SELECT gives correct results 
because the inserting transaction committed, so the command id is 
ignored. The script file with these commands in in the attachment, and I 
think it could make for a valuable regression test.

Side note: Another idea for a fix would be to have tqual.c change a 
combocid to a simple cmin when it does a time qual check and sees that 
the deleting subtransaction aborted. This is no good because it requires 
a write lock to change two header fields atomically, namely the 
HEAP_COMBOCID hint and t_cid. There are probably other issues, but this 
seems like a show-stopper.

The one-line patch is in the attachment. It gives correct behavior on my 
test case. This is my first patch; I hope I did it right!

Thanks!
Karl Schnaitter

test=# CREATE TABLE tab(a INT);
CREATE TABLE
test=# BEGIN;
BEGIN
test=# INSERT INTO tab VALUES(1);
INSERT 0 1
test=# INSERT INTO tab VALUES(2);
INSERT 0 1
test=# INSERT INTO tab VALUES(3);
INSERT 0 1
test=# SAVEPOINT s; DELETE FROM tab; ROLLBACK TO SAVEPOINT s;
SAVEPOINT
DELETE 3
ROLLBACK
test=# SELECT a FROM tab;
 a
---
 1
 2
 3
(3 rows)

test=# SAVEPOINT s; DELETE FROM tab; ROLLBACK TO SAVEPOINT s;
SAVEPOINT
DELETE 3
ROLLBACK
test=# SAVEPOINT s; DELETE FROM tab; ROLLBACK TO SAVEPOINT s;
SAVEPOINT
DELETE 3
ROLLBACK
test=# SAVEPOINT s; DELETE FROM tab; ROLLBACK TO SAVEPOINT s;
SAVEPOINT
DELETE 3
ROLLBACK
test=# SELECT a FROM tab;
 a
---
 1
(1 row)

test=# COMMIT;
COMMIT
test=# SELECT a FROM tab;
 a
---
 1
 2
 3
(3 rows)

test=# DROP TABLE tab;
DROP TABLE


--------------070000090602080501090105
Content-Type: application/x-gzip;
 name="combocid.tgz"
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="combocid.tgz"

H4sIAL1tuEgAA+2VW1PaQBSAeYVfcXxxgBFIAomtTNvBEDVTLg6ktm/OkiyaMRe6uxns9M/3
bDAibWmto3Zs93vZZM/97ObET+NZ6odBq/R0aMi+pq1Wy9xYbyjpumZamoUYJU1v73fMEphP
mNMtGReEAZSuCIu4f7lV73fyopBifSH4xfkXD80FEb8o8yHIflidzrbz1w1TL85f75gWnr9l
mkYJnqWJ//n51+t1aLY481sz4l/RJGhlIox4S4QxXd8Jv5my8KJ8xELoZRdgvAa9fWBaB3oH
DE17VWk0Gvfx8p2DdudA01YO6pvId9BNc0+3dMg3AMrhHKo7VZEtGm/FeZjM05jwK9iFE6d3
ev5p6I7O7fFw6Hqe06/B7q40KXuMJJz4IkwTN3C5nTFGE7GxWz2hZOFli4jiQ0DZMRWf4jCR
gWq1mvTytbKDruw0jkkSuEHZRzG8gR/tJmR5q5Xbd9E8z6Pux+QabVDJlu1Yq0lne5DLc3XU
DXneMlQXLKPdvLtFM/C58TKaYRd2j9OEv/2h/KNszn9BuWjyz9Hjxsjn//b/v260jZv5b1p6
O5//HUtT8/85sCdOz3PA6x0OHBBkViXgjjz8CCuHzrE7whWHj80oERTEJaMUWLrksAzFJQTh
fE7lCAH5/Vbc0dSZeNJ8LD3BWW/wwZlWdXS2RWRsF7Vrq9B9GlERJhdAoggTuA2Pb/7drFbD
Ai/yTW5SlZMYBThTKtPemXM6xhjAu9B3Bg6WfDQZD2XELkzGg8Fhz34PmMJdzTyBjxTo9YL6
YjMDkQIXMouZzAC7AGQuKENhFMl/YGWKUWwPyDpO7s5jX3BvTpcQp0z6yhJMOZ1DkBeaJvyh
yT6vmaxlmjcgr/7dT+td/YLuqdyfjE/X91ANfIVCoVAoFAqFQqFQKBQKhUKhUCgUCoXiz/kG
Jkw4kQAoAAA=
--------------070000090602080501090105--