Multiple page store bugfixes.

h2/src/main/org/h2/index/PageBtree.java

@@ -234,9 +234,9 @@ public abstract class PageBtree extends Page {
     abstract SearchRow remove(SearchRow row) throws SQLException;
 
     /**
-     * Free up all child pages.
+     * Free this page and all child pages.
      */
-    abstract void freeChildren() throws SQLException;
+    abstract void freeRecursive() throws SQLException;
 
     /**
      * Ensure all rows are read in memory.

h2/src/main/org/h2/index/PageBtreeIndex.java

@@ -229,7 +229,7 @@ public class PageBtreeIndex extends PageIndex {
             trace.debug("remove");
         }
         removeAllRows();
-        store.free(rootPageId, true);
+        store.free(rootPageId);
         store.removeMeta(this, session);
     }
 
@@ -246,8 +246,7 @@ public class PageBtreeIndex extends PageIndex {
 
     private void removeAllRows() throws SQLException {
         PageBtree root = getPage(rootPageId);
-        store.logUndo(root, root.data);
-        root.freeChildren();
+        root.freeRecursive();
         root = PageBtreeLeaf.create(this, rootPageId, PageBtree.ROOT);
         store.removeRecord(rootPageId);
         store.update(root);

h2/src/main/org/h2/index/PageBtreeLeaf.java

@@ -222,8 +222,9 @@ public class PageBtreeLeaf extends PageBtree {
         return null;
     }
 
-    void freeChildren() {
-        // nothing to do
+    void freeRecursive() throws SQLException {
+        index.getPageStore().logUndo(this, data);
+        index.getPageStore().free(getPos());
     }
 
     int getRowCount() {
@@ -240,7 +241,6 @@ public class PageBtreeLeaf extends PageBtree {
 
     public void write(DataPage buff) throws SQLException {
         write();
-        index.getPageStore().checkUndo(getPos());
         index.getPageStore().writePage(getPos(), data);
     }
 
@@ -340,7 +340,7 @@ public class PageBtreeLeaf extends PageBtree {
             PageBtreeNode p = (PageBtreeNode) store.getPage(parentPageId);
             p.moveChild(getPos(), newPos);
         }
-        store.free(getPos(), true);
+        store.free(getPos());
     }
 
 }

h2/src/main/org/h2/index/PageBtreeNode.java

@@ -318,7 +318,7 @@ public class PageBtreeNode extends PageBtree {
             return null;
         } else if (last == row) {
             // this child is now empty
-            index.getPageStore().free(page.getPos(), true);
+            index.getPageStore().free(page.getPos());
             if (entryCount < 1) {
                 // no more children - this page is empty as well
                 return row;
@@ -390,7 +390,6 @@ public class PageBtreeNode extends PageBtree {
     public void write(DataPage buff) throws SQLException {
         check();
         write();
-        index.getPageStore().checkUndo(getPos());
         index.getPageStore().writePage(getPos(), data);
     }
 
@@ -421,12 +420,10 @@ public class PageBtreeNode extends PageBtree {
         written = true;
     }
 
-    void freeChildren() throws SQLException {
-        for (int i = 0; i <= entryCount; i++) {
-            int childPageId = childPageIds[i];
-            PageBtree child = index.getPage(childPageId);
-            index.getPageStore().free(childPageId, false);
-            child.freeChildren();
+    void freeRecursive() throws SQLException {
+        index.getPageStore().logUndo(this, data);
+        for (int childPageId : childPageIds) {
+            index.getPage(childPageId).freeRecursive();
         }
     }
 
@@ -542,12 +539,12 @@ public class PageBtreeNode extends PageBtree {
             PageBtreeNode p = (PageBtreeNode) store.getPage(parentPageId);
             p.moveChild(getPos(), newPos);
         }
-        for (int i = 0; i < childPageIds.length; i++) {
-            PageBtree p = (PageBtree) store.getPage(childPageIds[i]);
+        for (int childPageId : childPageIds) {
+            PageBtree p = index.getPage(childPageId);
             p.setParentPageId(newPos);
             store.update(p);
         }
-        store.free(getPos(), true);
+        store.free(getPos());
     }
 
     /**

h2/src/main/org/h2/index/PageData.java

@@ -198,9 +198,9 @@ abstract class PageData extends Page {
     abstract boolean remove(long key) throws SQLException;
 
     /**
-     * Free up all child pages.
+     * Free this page and all child pages.
      */
-    abstract void freeChildren() throws SQLException;
+    abstract void freeRecursive() throws SQLException;
 
     /**
      * Get the row for the given key.

h2/src/main/org/h2/index/PageDataIndex.java

@@ -12,6 +12,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import org.h2.constant.ErrorCode;
 import org.h2.engine.Constants;
 import org.h2.engine.Session;
 import org.h2.log.UndoLogRecord;
@@ -188,7 +189,11 @@ public class PageDataIndex extends PageIndex implements RowIndex {
      * @return the page
      */
     PageDataOverflow getPageOverflow(int id) throws SQLException {
-        return (PageDataOverflow) store.getPage(id);
+        Page p = store.getPage(id);
+        if (p instanceof PageDataOverflow) {
+            return (PageDataOverflow) p;
+        }
+        throw Message.getSQLException(ErrorCode.FILE_CORRUPTED_1, p.toString());
     }
 
     /**
@@ -317,7 +322,7 @@ public class PageDataIndex extends PageIndex implements RowIndex {
             trace.debug(this + " remove");
         }
         removeAllRows();
-        store.free(rootPageId, true);
+        store.free(rootPageId);
         store.removeMeta(this, session);
     }
 
@@ -338,8 +343,7 @@ public class PageDataIndex extends PageIndex implements RowIndex {
 
     private void removeAllRows() throws SQLException {
         PageData root = getPage(rootPageId, 0);
-        store.logUndo(root, root.data);
-        root.freeChildren();
+        root.freeRecursive();
         root = PageDataLeaf.create(this, rootPageId, PageData.ROOT);
         store.removeRecord(rootPageId);
         store.update(root);

h2/src/main/org/h2/index/PageDataLeaf.java

@@ -143,6 +143,14 @@ public class PageDataLeaf extends PageData {
         return size;
     }
 
+    private int findInsertionPoint(long key) throws SQLException {
+        int x = find(key);
+        if (x < keys.length && keys[x] == key) {
+            throw index.getDuplicateKeyException();
+        }
+        return x;
+    }
+
     int addRowTry(Row row) throws SQLException {
         index.getPageStore().logUndo(this, data);
         int rowLength = getRowLength(row);
@@ -150,7 +158,7 @@ public class PageDataLeaf extends PageData {
         int last = entryCount == 0 ? pageSize : offsets[entryCount - 1];
         int keyOffsetPairLen = 2 + data.getVarLongLen(row.getKey());
         if (entryCount > 0 && last - rowLength < start + keyOffsetPairLen) {
-            int x = find(row.getKey());
+            int x = findInsertionPoint(row.getKey());
             if (entryCount > 1) {
                 if (entryCount < 5) {
                     // required, otherwise the index doesn't work correctly
@@ -177,10 +185,7 @@ public class PageDataLeaf extends PageData {
             x = 0;
         } else {
             readAllRows();
-            x = find(row.getKey());
-            if (x < keys.length && keys[x] == row.getKey()) {
-                throw index.getDuplicateKeyException();
-            }
+            x = findInsertionPoint(row.getKey());
             System.arraycopy(offsets, 0, newOffsets, 0, x);
             System.arraycopy(keys, 0, newKeys, 0, x);
             System.arraycopy(rows, 0, newRows, 0, x);
@@ -262,7 +267,7 @@ public class PageDataLeaf extends PageData {
         if (entryCount < 0) {
             Message.throwInternalError();
         }
-        freeChildren();
+        freeOverflow();
         firstOverflowPageId = 0;
         overflowRowSize = 0;
         rowRef = null;
@@ -320,10 +325,6 @@ public class PageDataLeaf extends PageData {
                 int next = firstOverflowPageId;
                 do {
                     PageDataOverflow page = index.getPageOverflow(next);
-if (page == null) {
-    page = index.getPageOverflow(next);
-    System.out.println("stop!");
-}
                     next = page.readInto(buff);
                 } while (next != 0);
                 overflowRowSize = pageSize + buff.length();
@@ -394,7 +395,7 @@ if (page == null) {
         }
         index.getPageStore().logUndo(this, data);
         if (entryCount == 1) {
-            freeChildren();
+            freeRecursive();
             return true;
         }
         removeRow(i);
@@ -402,13 +403,18 @@ if (page == null) {
         return false;
     }
 
-    void freeChildren() throws SQLException {
+    void freeRecursive() throws SQLException {
+        index.getPageStore().logUndo(this, data);
+        index.getPageStore().free(getPos());
+        freeOverflow();
+    }
+
+    void freeOverflow() throws SQLException {
         if (firstOverflowPageId != 0) {
-            PageStore store = index.getPageStore();
             int next = firstOverflowPageId;
             do {
                 PageDataOverflow page = index.getPageOverflow(next);
-                store.free(next, false);
+                page.free();
                 next = page.getNextOverflow();
             } while (next != 0);
         }
@@ -433,7 +439,6 @@ if (page == null) {
 
     public void write(DataPage buff) throws SQLException {
         write();
-        index.getPageStore().checkUndo(getPos());
         index.getPageStore().writePage(getPos(), data);
         data.truncate(index.getPageStore().getPageSize());
     }
@@ -516,7 +521,7 @@ if (page == null) {
         p2.write();
         p2.data.truncate(index.getPageStore().getPageSize());
         store.update(p2);
-        store.free(getPos(), true);
+        store.free(getPos());
         if (parentPageId == ROOT) {
             index.setRootPageId(session, newPos);
         } else {

h2/src/main/org/h2/index/PageDataNode.java

@@ -259,7 +259,7 @@ public class PageDataNode extends PageData {
             return false;
         }
         // this child is now empty
-        index.getPageStore().free(page.getPos(), true);
+        index.getPageStore().free(page.getPos());
         if (entryCount < 1) {
             // no more children - this page is empty as well
             return true;
@@ -269,12 +269,11 @@ public class PageDataNode extends PageData {
         return false;
     }
 
-    void freeChildren() throws SQLException {
-        for (int i = 0; i <= entryCount; i++) {
-            int childPageId = childPageIds[i];
-            PageData child = index.getPage(childPageId, getPos());
-            index.getPageStore().free(childPageId, false);
-            child.freeChildren();
+    void freeRecursive() throws SQLException {
+        index.getPageStore().logUndo(this, data);
+        index.getPageStore().free(getPos());
+        for (int childPageId : childPageIds) {
+            index.getPage(childPageId, getPos()).freeRecursive();
         }
     }
 
@@ -326,7 +325,6 @@ public class PageDataNode extends PageData {
 
     public void write(DataPage buff) throws SQLException {
         write();
-        index.getPageStore().checkUndo(getPos());
         index.getPageStore().writePage(getPos(), data);
     }
 
@@ -409,7 +407,7 @@ public class PageDataNode extends PageData {
             p.setParentPageId(newPos);
             store.update(p);
         }
-        store.free(getPos(), true);
+        store.free(getPos());
     }
 
     /**

h2/src/main/org/h2/index/PageDataOverflow.java

@@ -180,7 +180,6 @@ public class PageDataOverflow extends Page {
 
     public void write(DataPage buff) throws SQLException {
         write();
-        store.checkUndo(getPos());
         store.writePage(getPos(), data);
     }
 
@@ -236,7 +235,7 @@ public class PageDataOverflow extends Page {
             p1.setOverflow(getPos(), newPos);
         }
         store.update(p);
-        store.free(getPos(), true);
+        store.free(getPos());
     }
 
     private void setNext(int old, int nextPage) throws SQLException {
@@ -248,4 +247,9 @@ public class PageDataOverflow extends Page {
         data.setInt(START_NEXT_OVERFLOW, nextPage);
     }
 
+    void free() throws SQLException {
+        store.logUndo(this, data);
+        store.free(getPos());
+    }
+
 }

h2/src/main/org/h2/store/PageStore.java

@@ -393,6 +393,7 @@ public class PageStore implements CacheWriter {
                 break;
             }
         }
+        writeIndexRowCounts();
         writeBack();
         // truncate the log
         recoveryRunning = true;
@@ -867,13 +868,22 @@ public class PageStore implements CacheWriter {
         fileLength = newLength;
     }
 
+    /**
+     * Add a page to the free list. The undo log entry must have been written.
+     *
+     * @param pageId the page id
+     */
+    public void free(int pageId) throws SQLException {
+        free(pageId, true);
+    }
+
     /**
      * Add a page to the free list.
      *
      * @param pageId the page id
      * @param undo if the undo record must have been written
      */
-    public void free(int pageId, boolean undo) throws SQLException {
+    void free(int pageId, boolean undo) throws SQLException {
         if (trace.isDebugEnabled()) {
             // trace.debug("freePage " + pageId);
         }
@@ -1036,9 +1046,9 @@ public class PageStore implements CacheWriter {
             openIndex.close(systemSession);
         }
         allocatePage(PAGE_ID_META_ROOT);
+        writeIndexRowCounts();
         recoveryRunning = false;
         reservedPages = null;
-        writeIndexRowCounts();
         writeBack();
         // clear the cache because it contains pages with closed indexes
         cache.clear();
@@ -1548,16 +1558,4 @@ public class PageStore implements CacheWriter {
         return true;
     }
 
-    /**
-     * Check if the undo entry for the given page has been written.
-     *
-     * @param pageId the page id
-     */
-    public void checkUndo(int pageId) throws SQLException {
-        if (SysProperties.CHECK && !recoveryRunning) {
-            // ensure the undo entry is already written
-            // log.addUndo(pageId, null);
-        }
-    }
-
 }

h2/src/test/org/h2/test/unit/TestPageStore.java

@@ -36,6 +36,7 @@ public class TestPageStore extends TestBase implements DatabaseEventListener {
     }
 
     public void test() throws Exception {
+        testDuplicateKey();
         testUpdateOverflow();
         testTruncateReconnect();
         testReverseIndex();
@@ -56,6 +57,22 @@ public class TestPageStore extends TestBase implements DatabaseEventListener {
         testFuzzOperations();
     }
 
+    private void testDuplicateKey() throws SQLException {
+        deleteDb("pageStore");
+        Connection conn;
+        conn = getConnection("pageStore;PAGE_STORE=TRUE");
+        Statement stat = conn.createStatement();
+        stat.execute("create table test(id int primary key, name varchar)");
+        stat.execute("insert into test values(0, space(3000))");
+        try {
+            stat.execute("insert into test values(0, space(3000))");
+        } catch (SQLException e) {
+            // ignore
+        }
+        stat.execute("select * from test");
+        conn.close();
+    }
+
     private void testTruncateReconnect() throws SQLException {
         if (config.memory) {
             return;