#747 Truncation handling of FBStatisticsManager

.gitignore

@@ -4,6 +4,7 @@ gen/
 build_*.bat
 .idea/
 build-local.properties
+local-test-logging.properties
 classes/
 *.iml
 *.eml

src/docs/asciidoc/release_notes.adoc

@@ -40,6 +40,11 @@ The following has been changed or fixed since Jaybird 5.0.1:
 +
 This bug did not affect connection property `serverBatchBufferSize` set through the JDBC URL or provided in a `Properties` object to `DriverManager`, it only affected the property set through implementations of `DatabaseConnectionProperties` (e.g. data sources from `org.firebirdsql.ds`).
 * New feature: add `MaintenanceManager.fixIcu()` for the Firebird 3.0 gfix/service repair action "`ICU`" to update or rebuild collations and indexes when the ICU version changed (https://github.com/FirebirdSQL/jaybird/issues/744[jaybird#744])
+* Fixed: The first call to `getTableStatistics()` of a `FBTableStatisticsManager` instance returned only a few or even no tables; if no tables were returned, subsequent calls would also return no tables (https://github.com/FirebirdSQL/jaybird/issues/747[jaybird#747])
++
+Truncation of the information response will now result in three retries (so, four attempts), increasing the buffer size for each retry.
+If after three retries, the buffer is still truncated, an `InfoTruncatedException` exception is thrown.
+Subsequent attempts to call `getTableStatistics()` may succeed as that will further increase the buffer size.
 
 [#jaybird-5-0-1-changelog]
 === Jaybird 5.0.1

src/main/org/firebirdsql/gds/ng/InfoProcessor.java

@@ -29,12 +29,11 @@
 import java.sql.SQLException;
 
 /**
- * Functional interface to process an information buffer (responses to p_info_*
- * requests) returning an object of type T.
+ * Functional interface to process an information buffer (responses to p_info_* requests) returning an object of type T.
  *
  * @param <T>
- *         Type of the result of the {@link #process(byte[])} method.
- * @author <a href="mailto:mrotteveel@users.sourceforge.net">Mark Rotteveel</a>
+ *         type of the result of the {@link #process(byte[])} method.
+ * @author Mark Rotteveel
  * @since 3.0
  */
 public interface InfoProcessor<T> {
@@ -44,17 +43,18 @@ public interface InfoProcessor<T> {
      *
      * @param infoResponse
      *         byte array containing the server response to an info-request.
-     * @return Processed response of type T (usually - but not required - a
-     *         newly created object).
+     * @return Processed response of type T (usually - but not required - a newly created object).
+     * @throws InfoTruncatedException
+     *         (optional) if {@code infoResponse} is truncated and this processor could not recover by itself
      * @throws SQLException
-     *         For errors during the infoResponse.
+     *         for errors during processing the infoResponse.
      */
     T process(byte[] infoResponse) throws SQLException;
 
     /**
      * Interface for information on a statement.
      *
-     * @author <a href="mailto:mrotteveel@users.sourceforge.net">Mark Rotteveel</a>
+     * @author Mark Rotteveel
      * @since 3.0
      */
     interface StatementInfo {

src/main/org/firebirdsql/gds/ng/InfoTruncatedException.java

@@ -0,0 +1,51 @@
+/*
+ * Firebird Open Source JDBC Driver
+ *
+ * Distributable under LGPL license.
+ * You may obtain a copy of the License at http://www.gnu.org/copyleft/lgpl.html
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * LGPL License for more details.
+ *
+ * This file was created by members of the firebird development team.
+ * All individual contributions remain the Copyright (C) of those
+ * individuals.  Contributors to this file are either listed here or
+ * can be obtained from a source control history command.
+ *
+ * All rights reserved.
+ */
+package org.firebirdsql.gds.ng;
+
+import java.sql.SQLNonTransientException;
+
+/**
+ * Thrown to indicate that an info request buffer was truncated ({@code isc_info_truncated}).
+ * <p>
+ * Implementations of {@link InfoProcessor} may throw this exception if they cannot recover themselves from truncation.
+ * The length of the truncated buffer is reported in {@link #length()}; this is the <em>actual</em> length, not the
+ * <em>requested</em> length.
+ * </p>
+ * <p>
+ * This is a subclass of {@link SQLNonTransientException} as retrying the operation without taking corrective action
+ * will likely result in the same error.
+ * </p>
+ *
+ * @author Mark Rotteveel
+ * @since 5.0.2
+ */
+public class InfoTruncatedException extends SQLNonTransientException {
+
+    private final int length;
+
+    public InfoTruncatedException(String message, int length) {
+        super(message);
+        this.length = length;
+    }
+
+    public final int length() {
+        return length;
+    }
+
+}

src/main/org/firebirdsql/management/FBTableStatisticsManager.java

@@ -21,9 +21,9 @@
 import org.firebirdsql.gds.VaxEncoding;
 import org.firebirdsql.gds.ng.FbDatabase;
 import org.firebirdsql.gds.ng.InfoProcessor;
+import org.firebirdsql.gds.ng.InfoTruncatedException;
 import org.firebirdsql.jdbc.FirebirdConnection;
 import org.firebirdsql.jdbc.SQLStateConstants;
-import org.firebirdsql.logging.Logger;
 import org.firebirdsql.logging.LoggerFactory;
 import org.firebirdsql.util.Volatile;
 
@@ -59,8 +59,15 @@
 @Volatile(reason = "Experimental")
 public final class FBTableStatisticsManager implements AutoCloseable {
 
+    private static final int MAX_RETRIES = 3;
+
     private Map<Integer, String> tableMapping = new HashMap<>();
     private FirebirdConnection connection;
+    /**
+     * Table slack is a number which is used to pad the table count used for calculating the buffer size in an attempt
+     * to prevent or fix truncation of the info request. It is incremented when a truncation is handled.
+     */
+    private int tableSlack;
 
     private FBTableStatisticsManager(FirebirdConnection connection) throws SQLException {
         if (connection.isClosed()) {
@@ -91,13 +98,44 @@ public static FBTableStatisticsManager of(Connection connection) throws SQLExcep
      * </p>
      *
      * @return map from table name to table statistics
+     * @throws InfoTruncatedException
+     *         if a truncated response is received, after retrying 3 times (total: 4 attempts) while increasing
+     *         the buffer size; it is possible that subsequent calls to this method may recover (as that will increase
+     *         the buffer size even more)
      * @throws SQLException
      *         if the connection is closed, or if obtaining the statistics failed due to a database access error
      */
     public Map<String, TableStatistics> getTableStatistics() throws SQLException {
         checkClosed();
         FbDatabase db = connection.getFbDatabase();
-        return db.getDatabaseInfo(getInfoItems(), bufferSize(tableMapping.size()), new TableStatisticsProcessor());
+        InfoTruncatedException lastTruncation;
+        TableStatisticsProcessor tableStatisticsProcessor = new TableStatisticsProcessor();
+        int attempt = 0;
+        do {
+            try {
+                return db.getDatabaseInfo(getInfoItems(), bufferSize(getTableCount()), tableStatisticsProcessor);
+            } catch (InfoTruncatedException e) {
+                /* Occurrence of truncation should be rare. It could occur if all tables have all statistics items, and
+                   new tables are added after the last updateMapping() call or statistics were previously requested by
+                   a different instance and this instance was created after tables have been dropped.
+                   Here, tableSlack is incremented to account for tables removed, while updateTableMapping() is called
+                   to account for new tables. */
+                tableSlack++;
+                updateTableMapping();
+                lastTruncation = e;
+            }
+        } while (attempt++ < MAX_RETRIES);
+        throw lastTruncation;
+    }
+
+    /**
+     * @return the actual table count (so excluding {@link #tableSlack}).
+     */
+    private int getTableCount() throws SQLException {
+        int size = tableMapping.size();
+        if (size != 0) return size;
+        updateTableMapping();
+        return tableMapping.size();
     }
 
     /**
@@ -136,23 +174,9 @@ private void updateTableMapping() throws SQLException {
         }
     }
 
-    private String getTableName(Integer tableId) throws SQLException {
-        String tableName = tableMapping.get(tableId);
-        if (tableName == null) {
-            // mapping empty or out of date (e.g. new table created since the last update)
-            updateTableMapping();
-            tableName = tableMapping.get(tableId);
-            if (tableName == null) {
-                // fallback
-                tableName = "UNKNOWN_TABLE_ID_" + tableId;
-            }
-        }
-        return tableName;
-    }
-
-    private static int bufferSize(int maxTables) {
+    private int bufferSize(int maxTables) {
         // NOTE: In the current implementation in Firebird, the limit is actually 1 + 8 * (3 + 65535)
-        long size = 1 + 8 * (3 + 6 * (long) maxTables);
+        long size = 1 + 8 * (3 + 6 * (long) (maxTables + tableSlack));
         if (size <= 0) {
             return Integer.MAX_VALUE;
         }
@@ -172,9 +196,19 @@ private static byte[] getInfoItems() {
         };
     }
 
+    /**
+     * Info processor to retrieve table statistics.
+     * <p>
+     * This is a stateful object and not thread-safe. It should not be shared, and not be reused or only reused in
+     * a small scope. For example, in the current implementation, it is shared for the retry attempts within
+     * {@link #getTableStatistics()}, and this is OK because doing so prevents unnecessary calls to
+     * {@link #updateTableMapping()} from this processor.
+     * </p>
+     */
     private final class TableStatisticsProcessor implements InfoProcessor<Map<String, TableStatistics>> {
 
         private final Map<String, TableStatistics.TableStatisticsBuilder> statisticsBuilders = new HashMap<>();
+        private boolean allowTableMappingUpdate = true;
 
         @Override
         public Map<String, TableStatistics> process(byte[] infoResponse) throws SQLException {
@@ -186,11 +220,8 @@ public Map<String, TableStatistics> process(byte[] infoResponse) throws SQLExcep
                     switch (infoItem) {
                     case isc_info_end:
                         break decodeLoop;
-                    case isc_info_truncated: {
-                        Logger logger = LoggerFactory.getLogger(TableStatisticsProcessor.class);
-                        logger.debug("Received truncation processing table statistics, this is likely an implementation bug");
-                        break decodeLoop;
-                    }
+                    case isc_info_truncated:
+                        throw new InfoTruncatedException("Received isc_info_truncated, and this processor cannot recover automatically", infoResponse.length);
                     case isc_info_read_seq_count:
                     case isc_info_read_idx_count:
                     case isc_info_insert_count:
@@ -235,6 +266,25 @@ void processStatistics(int statistic, byte[] buffer, int start, int end) throws
             }
         }
 
+        private String getTableName(Integer tableId) throws SQLException {
+            String tableName = tableMapping.get(tableId);
+            if (tableName == null) {
+                // mapping empty or out of date (e.g. new table created since the last update)
+                if (allowTableMappingUpdate) {
+                    updateTableMapping();
+                    // Ensure that if we have multiple tables missing, we don't repeatedly update the table mapping, as
+                    // that wouldn't result in new information.
+                    allowTableMappingUpdate = false;
+                    tableName = tableMapping.get(tableId);
+                }
+                if (tableName == null) {
+                    // fallback
+                    tableName = "UNKNOWN_TABLE_ID_" + tableId;
+                }
+            }
+            return tableName;
+        }
+
         private TableStatistics.TableStatisticsBuilder getBuilder(int tableId) throws SQLException {
             String tableName = getTableName(tableId);
             return statisticsBuilders.computeIfAbsent(tableName, TableStatistics::builder);