Repository: diennea/herddb (java)

Issue:
Divide by zero on new page flush
Bug report.

I got a divide by zero during a new page flush.

```
java.sql.SQLException: java.util.concurrent.CompletionException: java.lang.ArithmeticException: / by zero
        at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
        at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)
        at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:645)
        at java.base/java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:478)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.ArithmeticException: / by zero
        at herddb.core.PageSet$DataPageMetaData.<init>(PageSet.java:58)
        at herddb.core.PageSet.pageCreated(PageSet.java:139)
        at herddb.core.TableManager.flushNewPage(TableManager.java:815)
        at herddb.core.TableManager.flushNewPageForUnload(TableManager.java:763)
        at herddb.core.TableManager.lambda$unload$2(TableManager.java:711)
        at java.base/java.util.concurrent.ConcurrentHashMap.computeIfPresent(ConcurrentHashMap.java:1822)
        at herddb.core.TableManager.unload(TableManager.java:704)
        at herddb.core.TableManager.allocateLivePage(TableManager.java:630)
        at herddb.core.TableManager.applyUpdate(TableManager.java:1728)
        at herddb.core.TableManager.onTransactionCommit(TableManager.java:1413)
        at herddb.core.TableSpaceManager.apply(TableSpaceManager.java:361)
        at herddb.core.TableSpaceManager.lambda$commitTransaction$16(TableSpaceManager.java:1692)
        at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
        ... 5 more
```

It occurred on a fresh standalone installation of HerdDB.

Pull request:
Correct empty pages flush, closes #423
Fix arithmetic divide by 0 found in #423 

Empty new pages flushed:
* will simply be discarded during page unload
* will be discarded during checkpoint ONLY if aren't the current active one

 - [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)


Patch:
diff --git a/herddb-core/src/main/java/herddb/core/PageSet.java b/herddb-core/src/main/java/herddb/core/PageSet.java
index ad3f0a64..b0f5c4ee 100644
--- a/herddb-core/src/main/java/herddb/core/PageSet.java
+++ b/herddb-core/src/main/java/herddb/core/PageSet.java
@@ -19,6 +19,9 @@
  */
 package herddb.core;
 
+import herddb.model.Record;
+import herddb.utils.ExtendedDataInputStream;
+import herddb.utils.ExtendedDataOutputStream;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.HashMap;
@@ -29,10 +32,6 @@ import java.util.concurrent.atomic.LongAdder;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
-import herddb.model.Record;
-import herddb.utils.ExtendedDataInputStream;
-import herddb.utils.ExtendedDataOutputStream;
-
 /**
  * Status of the set of pages of a Table
  *
@@ -59,7 +58,6 @@ public final class PageSet {
             this.dirt = new LongAdder();
         }
 
-
         private DataPageMetaData(long size, long avgRecordSize, long dirt) {
             super();
             this.size = size;
diff --git a/herddb-core/src/main/java/herddb/core/TableManager.java b/herddb-core/src/main/java/herddb/core/TableManager.java
index f040c32f..11b3485b 100644
--- a/herddb-core/src/main/java/herddb/core/TableManager.java
+++ b/herddb-core/src/main/java/herddb/core/TableManager.java
@@ -727,6 +727,12 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
         );
     }
 
+    private static enum FlushNewPageResult {
+        FLUSHED,
+        ALREADY_FLUSHED,
+        EMPTY_FLUSH;
+    }
+
     /**
      * Remove the page from {@link #newPages}, set it as "not writable" and write it
      * to disk
@@ -739,39 +745,51 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
      * @param spareDataPage old spare data to fit in the new page if possible
      */
     private void flushNewPageForCheckpoint(DataPage page, DataPage spareDataPage) {
-        final boolean flushed = flushNewPage(page, spareDataPage);
+        final FlushNewPageResult flush = flushNewPage(page, spareDataPage);
+        switch (flush) {
+            case FLUSHED:
 
-        if (flushed) {
-            /*
-             * Replace the page in memory with his immutable version (faster modification checks). We can
-             * replace the page with the immutable one from memory running during a checkpoint and no other
-             * page write could happen (thus our page copy is fully equivalent to really flushed one).
-             *
-             * We replace the page only if we have actually flushed it.. if it was flushed by another thread
-             * it was flushed due to unload request (no concurrent checkpoints) and then removed from pages
-             * (or the thread is going to remove it). We don't want to keep knowledge of a page not know
-             * anymore to page replacement policy (It can't be remove again)
-             *
-             * For similar reason we replace the page only if there actually is a page in the first place. If
-             * a concurrent thread flushed and removed the page we don't want to add it again.
-             */
-            pages.computeIfPresent(page.pageId, (i,p) -> p.toImmutable());
-        } else {
-            LOGGER.log(Level.INFO, "New page {0} already flushed in a concurrent thread", page.pageId);
-        }
+                /*
+                 * Replace the page in memory with his immutable version (faster modification checks). We can
+                 * replace the page with the immutable one from memory running during a checkpoint and no other
+                 * page write could happen (thus our page copy is fully equivalent to really flushed one).
+                 *
+                 * We replace the page only if we have actually flushed it.. if it was flushed by another thread
+                 * it was flushed due to unload request (no concurrent checkpoints) and then removed from pages
+                 * (or the thread is going to remove it). We don't want to keep knowledge of a page not know
+                 * anymore to page replacement policy (It can't be remove again)
+                 *
+                 * For similar reason we replace the page only if there actually is a page in the first place. If
+                 * a concurrent thread flushed and removed the page we don't want to add it again.
+                 */
+                pages.computeIfPresent(page.pageId, (i,p) -> p.toImmutable());
+                return;
+
+            case ALREADY_FLUSHED:
+                /* Already flushed (and possibly unloaded) by another thread */
+                LOGGER.log(Level.INFO, "New page {0} already flushed in a concurrent thread", page.pageId);
+                return;
+
+            case EMPTY_FLUSH:
+                /* Attempted and empty flush. The page has been dropped and we must remove it from page knowledge */
+                pageReplacementPolicy.remove(page);
+                pages.remove(page.pageId);
+                return;
 
+            default:
+                throw new IllegalArgumentException("Unknown new page flush result: " + flush);
+        }
     }
 
     /**
-     * Remove the page from {@link #newPages}, set it as "unloaded" and write it
-     * to disk
+     * Remove the page from {@link #newPages}, set it as "unloaded" and write it to disk
      *
      * @param page new page to flush
-     * @return {@code true} if the page has been flushed, {@code false} if
-     * already flushed by another thread
+     * @return {@code true} if the page has been flushed, {@code false} if not flushed by any other means
+     *         (concurrent flush or empty page flush)
      */
     private boolean flushNewPageForUnload(DataPage page) {
-        return flushNewPage(page, null);
+        return FlushNewPageResult.FLUSHED == flushNewPage(page, null);
     }
 
     /**
@@ -786,7 +804,7 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
      * @return {@code false} if no flush has been done because the page isn't writable anymore,
      *         {@code true} otherwise
      */
-    private boolean flushNewPage(DataPage page, DataPage spareDataPage) {
+    private FlushNewPageResult flushNewPage(DataPage page, DataPage spareDataPage) {
 
         if (page.immutable) {
             LOGGER.log(Level.SEVERE, "Attempt to flush an immutable page {0} as it was mutable", page.pageId);
@@ -797,7 +815,8 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
         try {
 
             if (!page.writable) {
-                return false;
+                LOGGER.log(Level.INFO, "Mutable page {0} already flushed in a concurrent thread", page.pageId);
+                return FlushNewPageResult.ALREADY_FLUSHED;
             }
 
         } finally {
@@ -813,9 +832,18 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
         try {
             if (!page.writable) {
                 LOGGER.log(Level.INFO, "Mutable page {0} already flushed in a concurrent thread", page.pageId);
-                return false;
+                return FlushNewPageResult.ALREADY_FLUSHED;
             }
 
+            /*
+             * If there isn't any data to flush we'll avoid flush at all and just remove the page. We avoid to
+             * copy data from spareDataPage to an empty page because it would just be an additional roundtrip
+             * for spare data with an additional PK write too (consider that you have just one empty page and
+             * an half full spareDataPage: why copy all data to the first and update PK records when you can
+             * just save spareDataPage directly?)
+             */
+            boolean drop = page.isEmpty();
+
             /*
              * NewPages removal technically could be done before current write lock but
              * doing it in lock permit to check safely if a page has been removed. Without
@@ -823,7 +851,9 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
              * concurrent thread or because it wasn't present in the first place.
              */
             /* Set the new page as a fully active page */
-            pageSet.pageCreated(page.pageId, page);
+            if (!drop) {
+                pageSet.pageCreated(page.pageId, page);
+            }
 
             /* Remove it from "new" pages */
             DataPage remove = newPages.remove(page.pageId);
@@ -835,6 +865,11 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
 
             page.writable = false;
 
+            if (drop) {
+                LOGGER.log(Level.INFO, "Deleted empty mutable page {0} instead of flushing it", page.pageId);
+                return FlushNewPageResult.EMPTY_FLUSH;
+            }
+
         } finally {
             lock.unlock();
         }
@@ -878,7 +913,7 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
                 new Object[]{table.name, page.pageId, page.size(), page.getUsedMemory()});
         dataStorageManager.writePage(tableSpaceUUID, table.uuid, page.pageId, page.getRecordsForFlush());
 
-        return true;
+        return FlushNewPageResult.FLUSHED;
     }
 
     /**
@@ -2608,6 +2643,13 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
             /* *** New pages handling *** */
             /* ************************** */
 
+            /*
+             * Retrieve the "current" new page. It can be held in memory because no writes are executed during
+             * a checkpoint and thus the page cannot change (nor be flushed due to an unload because it isn't
+             * known to page replacement policy)
+             */
+            final long lastKnownPageId = currentDirtyRecordsPage.get();
+
             /*
              * Flush dirty records (and remaining records from previous step).
              *
@@ -2618,7 +2660,8 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
              */
             long flushedNewPages = 0;
             for (DataPage dataPage : newPages.values()) {
-                if (!dataPage.isEmpty()) {
+                /* Flush every dirty page (but not the "current" dirty page if empty) */
+                if (lastKnownPageId != dataPage.pageId || !dataPage.isEmpty()) {
                     flushNewPageForCheckpoint(dataPage, buildingPage);
                     ++flushedNewPages;
                     flushedRecords += dataPage.size();
@@ -2694,7 +2737,7 @@ public final class TableManager implements AbstractTableManager, Page.Owner {
              */
             if (newPages.isEmpty()) {
                 /* Allocate live handles the correct policy load/unload of last dirty page */
-                allocateLivePage(currentDirtyRecordsPage.get());
+                allocateLivePage(lastKnownPageId);
             }
 
             checkPointRunning = false;


Test patch:
diff --git a/herddb-core/src/test/java/herddb/core/NewPageTest.java b/herddb-core/src/test/java/herddb/core/NewPageTest.java
new file mode 100644
index 00000000..67286d0e
--- /dev/null
+++ b/herddb-core/src/test/java/herddb/core/NewPageTest.java
@@ -0,0 +1,223 @@
+/*
+ Licensed to Diennea S.r.l. under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. Diennea S.r.l. licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied.  See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+ */
+package herddb.core;
+
+import static herddb.core.TestUtils.execute;
+import static herddb.model.TransactionContext.NO_TRANSACTION;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+
+import herddb.mem.MemoryCommitLogManager;
+import herddb.mem.MemoryDataStorageManager;
+import herddb.mem.MemoryMetadataStorageManager;
+import herddb.model.Record;
+import herddb.model.StatementEvaluationContext;
+import herddb.model.Table;
+import herddb.model.TransactionContext;
+import herddb.model.commands.CreateTableSpaceStatement;
+import herddb.model.commands.DeleteStatement;
+import herddb.model.commands.InsertStatement;
+import herddb.server.ServerConfiguration;
+import herddb.utils.Bytes;
+import java.util.Collections;
+import org.junit.Test;
+
+/**
+* Tests on <i>new</i> pages behaviours.
+*
+* @author diego.salvi
+*/
+public class NewPageTest {
+
+    /**
+     * Attempt to unload an empty new page from memory
+     */
+    @Test
+    public void unloadEmptyNewPage() throws Exception {
+        String nodeId = "localhost";
+
+        ServerConfiguration config1 = new ServerConfiguration();
+
+        /* Smaller pages to avoid too many record creations */
+        config1.set(ServerConfiguration.PROPERTY_MAX_LOGICAL_PAGE_SIZE, 10 * 1024);
+
+        try (DBManager manager = new DBManager("localhost",
+                new MemoryMetadataStorageManager(),
+                new MemoryDataStorageManager(),
+                new MemoryCommitLogManager(),
+                null, null, config1,
+                null);) {
+            manager.start();
+            CreateTableSpaceStatement st1 = new CreateTableSpaceStatement("tblspace1", Collections.singleton(nodeId), nodeId, 1, 0, 0);
+            manager.executeStatement(st1, StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(), NO_TRANSACTION);
+            manager.waitForTablespace("tblspace1", 10000);
+
+            execute(manager, "CREATE TABLE tblspace1.tsql (K1 string, S1 string, primary key(k1))",
+                    Collections.emptyList());
+
+            manager.checkpoint();
+
+            final TableManager tableManager = (TableManager) manager.getTableSpaceManager("tblspace1")
+                    .getTableManager("tsql");
+
+            final Table table = tableManager.getTable();
+
+            int records = 0;
+
+            /*
+             * Fill table until there are some loaded page (not only kept in memory and unknown to page
+             * replacement policy).
+             */
+            while (tableManager.getLoadedPages().size() < 2) {
+                Bytes key = Bytes.from_string("ins_key_" + records++);
+                assertEquals(1,
+                        manager.executeUpdate(new InsertStatement(table.tablespace, table.name, new Record(key, key)),
+                                StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(),
+                                TransactionContext.NO_TRANSACTION).getUpdateCount());
+            }
+
+            /* Checks that every page is still writable and filled */
+            for (DataPage page : tableManager.getLoadedPages()) {
+                assertTrue(page.writable);
+                assertFalse(page.immutable);
+
+                assertFalse(page.isEmpty());
+                assertNotEquals(0, page.getUsedMemory());
+            }
+
+            /* Fully remove all the data, existing (still mutable) pages should be empty now */
+            for (int i = 0; i < records; ++i) {
+                Bytes key = Bytes.from_string("ins_key_" + i);
+                assertEquals(1,
+                        manager.executeUpdate(new DeleteStatement(table.tablespace, table.name, key, null),
+                                StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(),
+                                TransactionContext.NO_TRANSACTION).getUpdateCount());
+            }
+
+            /* Checks that every page is still writable and empty */
+            for (DataPage page : tableManager.getLoadedPages()) {
+                assertTrue(page.writable);
+                assertFalse(page.immutable);
+
+                assertTrue(page.isEmpty());
+                assertEquals(0, page.getUsedMemory());
+            }
+
+            /* Expect that every empty new page remains */
+            assertEquals(2, tableManager.getLoadedPages().size());
+
+            /* Attempt to unload new pages known to page replacement policy */
+            boolean unloadedAny = false;
+            for (DataPage page : tableManager.getLoadedPages()) {
+                if (manager.getMemoryManager().getDataPageReplacementPolicy().remove(page)) {
+                    unloadedAny = true;
+                    tableManager.unload(page.pageId);
+                }
+            }
+
+            /* Ensure that some page was unloaded */
+            assertTrue(unloadedAny);
+        }
+    }
+
+
+    /**
+     * Attempt to executre a checkpoint with empty new pages
+     */
+    @Test
+    public void checkpointEmptyNewPage() throws Exception {
+        String nodeId = "localhost";
+
+        ServerConfiguration config1 = new ServerConfiguration();
+
+        /* Smaller pages to avoid too many record creations */
+        config1.set(ServerConfiguration.PROPERTY_MAX_LOGICAL_PAGE_SIZE, 10 * 1024);
+
+        try (DBManager manager = new DBManager("localhost",
+                new MemoryMetadataStorageManager(),
+                new MemoryDataStorageManager(),
+                new MemoryCommitLogManager(),
+                null, null, config1,
+                null);) {
+            manager.start();
+            CreateTableSpaceStatement st1 = new CreateTableSpaceStatement("tblspace1", Collections.singleton(nodeId), nodeId, 1, 0, 0);
+            manager.executeStatement(st1, StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(), NO_TRANSACTION);
+            manager.waitForTablespace("tblspace1", 10000);
+
+            execute(manager, "CREATE TABLE tblspace1.tsql (K1 string, S1 string, primary key(k1))",
+                    Collections.emptyList());
+
+            manager.checkpoint();
+
+            final TableManager tableManager = (TableManager) manager.getTableSpaceManager("tblspace1")
+                    .getTableManager("tsql");
+
+            final Table table = tableManager.getTable();
+
+            int records = 0;
+
+            /*
+             * Fill table until there are some loaded page (not only kept in memory and unknown to page
+             * replacement policy).
+             */
+            while (tableManager.getLoadedPages().size() < 2) {
+                Bytes key = Bytes.from_string("ins_key_" + records++);
+                assertEquals(1,
+                        manager.executeUpdate(new InsertStatement(table.tablespace, table.name, new Record(key, key)),
+                                StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(),
+                                TransactionContext.NO_TRANSACTION).getUpdateCount());
+            }
+
+            /* Checks that every page is still writable and filled */
+            for (DataPage page : tableManager.getLoadedPages()) {
+                assertTrue(page.writable);
+                assertFalse(page.immutable);
+
+                assertFalse(page.isEmpty());
+                assertNotEquals(0, page.getUsedMemory());
+            }
+
+            /* Fully remove all the data, existing (still mutable) pages should be empty now */
+            for (int i = 0; i < records; ++i) {
+                Bytes key = Bytes.from_string("ins_key_" + i);
+                assertEquals(1,
+                        manager.executeUpdate(new DeleteStatement(table.tablespace, table.name, key, null),
+                                StatementEvaluationContext.DEFAULT_EVALUATION_CONTEXT(),
+                                TransactionContext.NO_TRANSACTION).getUpdateCount());
+            }
+
+            /* Checks that every page is still writable and empty */
+            for (DataPage page : tableManager.getLoadedPages()) {
+                assertTrue(page.writable);
+                assertFalse(page.immutable);
+
+                assertTrue(page.isEmpty());
+                assertEquals(0, page.getUsedMemory());
+            }
+
+            manager.checkpoint();
+
+            /* Expect just one (empty) new page remaining */
+            assertEquals(1, tableManager.getLoadedPages().size());
+        }
+    }
+}