fix: allow disabling field metadata cache (#1052)

Clients accessing very dynamic schemas can have issues with the field metadata cache getting stale. This change allows configuring the databaseMetadataCacheFields property to 0 to disable the cache and avoid these issues where necessary. This behaviour was already documented, however did not actually work as the codepath assumed it could retrieve the fields from the cache.
pgjdbc · May 9, 2018 · 6ce91721048dea0e73231fa50c365108e9b9d49d · 6ce9172
1 parent e88abd7
commit 6ce91721048dea0e73231fa50c365108e9b9d49d
diff --git a/pgjdbc/src/main/java/org/postgresql/PGProperty.java b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
@@ -108,7 +108,7 @@
          "Specifies the maximum number of fields to be cached per connection. A value of {@code 0} disables the cache."),

  /**
-   * Specifies the maximum number of fields to be cached per connection. A value of {@code 0} disables the cache.
+   * Specifies the maximum size (in megabytes) of fields to be cached per connection. A value of {@code 0} disables the cache.
   */
  DATABASE_METADATA_CACHE_FIELDS_MIB("databaseMetadataCacheFieldsMiB", "5",
          "Specifies the maximum size (in megabytes) of fields to be cached per connection. A value of {@code 0} disables the cache."),
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSetMetaData.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSetMetaData.java
@@ -10,8 +10,9 @@
 import org.postgresql.core.Field;
 import org.postgresql.core.ServerVersion;
 import org.postgresql.util.GT;
+import org.postgresql.util.Gettable;
+import org.postgresql.util.GettableHashMap;
 import org.postgresql.util.JdbcBlackHole;
-import org.postgresql.util.LruCache;
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;

@@ -202,9 +203,8 @@ public String getSchemaName(int column) throws SQLException {
    return "";
  }

-  private boolean populateFieldsWithCachedMetadata() {
+  private boolean populateFieldsWithMetadata(Gettable<FieldMetadata.Key, FieldMetadata> metadata) {
    boolean allOk = true;
-    LruCache<FieldMetadata.Key, FieldMetadata> metadata = connection.getFieldMetadataCache();
    for (Field field : fields) {
      if (field.getMetadata() != null) {
        // No need to update metadata
@@ -219,6 +219,7 @@ private boolean populateFieldsWithCachedMetadata() {
        field.setMetadata(fieldMetadata);
      }
    }
+    fieldInfoFetched |= allOk;
    return allOk;
  }

@@ -227,8 +228,7 @@ private void fetchFieldMetaData() throws SQLException {
      return;
    }

-    if (populateFieldsWithCachedMetadata()) {
-      fieldInfoFetched = true;
+    if (populateFieldsWithMetadata(connection.getFieldMetadataCache())) {
      return;
    }

@@ -285,8 +285,8 @@ private void fetchFieldMetaData() throws SQLException {

    Statement stmt = connection.createStatement();
    ResultSet rs = null;
+    GettableHashMap<FieldMetadata.Key, FieldMetadata> md = new GettableHashMap<FieldMetadata.Key, FieldMetadata>();
    try {
-      LruCache<FieldMetadata.Key, FieldMetadata> metadataCache = connection.getFieldMetadataCache();
      rs = stmt.executeQuery(sql.toString());
      while (rs.next()) {
        int table = (int) rs.getLong(1);
@@ -300,13 +300,14 @@ private void fetchFieldMetaData() throws SQLException {
        FieldMetadata fieldMetadata =
            new FieldMetadata(columnName, tableName, schemaName, nullable, autoIncrement);
        FieldMetadata.Key key = new FieldMetadata.Key(table, column);
-        metadataCache.put(key, fieldMetadata);
+        md.put(key, fieldMetadata);
      }
    } finally {
      JdbcBlackHole.close(rs);
      JdbcBlackHole.close(stmt);
    }
-    populateFieldsWithCachedMetadata();
+    populateFieldsWithMetadata(md);
+    connection.getFieldMetadataCache().putAll(md);
  }

  public String getBaseSchemaName(int column) throws SQLException {
diff --git a/pgjdbc/src/main/java/org/postgresql/util/Gettable.java b/pgjdbc/src/main/java/org/postgresql/util/Gettable.java
@@ -0,0 +1,10 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.util;
+
+public interface Gettable<K,V> {
+  V get(K key);
+}
diff --git a/pgjdbc/src/main/java/org/postgresql/util/GettableHashMap.java b/pgjdbc/src/main/java/org/postgresql/util/GettableHashMap.java
@@ -0,0 +1,12 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.util;
+
+import java.util.HashMap;
+
+public class GettableHashMap<K,V> extends HashMap<K,V> implements Gettable<K,V> {
+
+}
diff --git a/pgjdbc/src/main/java/org/postgresql/util/LruCache.java b/pgjdbc/src/main/java/org/postgresql/util/LruCache.java
@@ -13,7 +13,7 @@
 /**
 * Caches values in simple least-recently-accessed order.
 */
-public class LruCache<Key, Value extends CanEstimateSize> {
+public class LruCache<Key, Value extends CanEstimateSize> implements Gettable<Key, Value> {
  /**
   * Action that is invoked when the entry is removed from the cache.
   *
@@ -144,6 +144,15 @@ public synchronized void put(Key key, Value value) {
    }
  }

+  /**
+   * Puts all the values from the given map into the cache.
+   */
+  public synchronized void putAll(Map<Key, Value> m) {
+    for (Map.Entry<Key, Value> entry : m.entrySet()) {
+      this.put(entry.getKey(), entry.getValue());
+    }
+  }
+
  public static final CreateAction NOOP_CREATE_ACTION = new CreateAction() {
    @Override
    public Object create(Object o) throws SQLException {
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ResultSetMetaDataTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ResultSetMetaDataTest.java
@@ -9,6 +9,7 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;

+import org.postgresql.PGProperty;
 import org.postgresql.PGResultSetMetaData;
 import org.postgresql.core.ServerVersion;
 import org.postgresql.jdbc.PreferQueryMode;
@@ -17,6 +18,8 @@
 import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;

 import java.sql.Connection;
 import java.sql.DatabaseMetaData;
@@ -26,15 +29,49 @@
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.Types;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Properties;

+@RunWith(Parameterized.class)
 public class ResultSetMetaDataTest extends BaseTest4 {
  Connection conn;
+  private final Integer databaseMetadataCacheFields;
+  private final Integer databaseMetadataCacheFieldsMib;
+
+  public ResultSetMetaDataTest(Integer databaseMetadataCacheFields, Integer databaseMetadataCacheFieldsMib) {
+    this.databaseMetadataCacheFields = databaseMetadataCacheFields;
+    this.databaseMetadataCacheFieldsMib = databaseMetadataCacheFieldsMib;
+  }
+
+  @Parameterized.Parameters(name = "databaseMetadataCacheFields = {0}, databaseMetadataCacheFieldsMib = {1}")
+  public static Iterable<Object[]> data() {
+    Collection<Object[]> ids = new ArrayList<Object[]>();
+    for (Integer fields : new Integer[]{null, 0}) {
+      for (Integer fieldsMib : new Integer[]{null, 0}) {
+        ids.add(new Object[]{fields, fieldsMib});
+      }
+    }
+    return ids;
+  }
+
+  @Override
+  protected void updateProperties(Properties props) {
+    super.updateProperties(props);
+    if (databaseMetadataCacheFields != null) {
+      PGProperty.DATABASE_METADATA_CACHE_FIELDS.set(props, databaseMetadataCacheFields);
+    }
+    if (databaseMetadataCacheFieldsMib != null) {
+      PGProperty.DATABASE_METADATA_CACHE_FIELDS_MIB.set(props, databaseMetadataCacheFieldsMib);
+    }
+  }

  @Override
  public void setUp() throws Exception {
    super.setUp();
    conn = con;
    TestUtil.createTable(conn, "rsmd1", "a int primary key, b text, c decimal(10,2)", true);
+    TestUtil.createTable(conn, "rsmd_cache", "a int primary key");
    TestUtil.createTable(conn, "timetest",
        "tm time(3), tmtz timetz, ts timestamp without time zone, tstz timestamp(6) with time zone");

@@ -57,6 +94,7 @@ public void setUp() throws Exception {
  public void tearDown() throws SQLException {
    TestUtil.dropTable(conn, "compositetest");
    TestUtil.dropTable(conn, "rsmd1");
+    TestUtil.dropTable(conn, "rsmd_cache");
    TestUtil.dropTable(conn, "timetest");
    TestUtil.dropTable(conn, "serialtest");
    if (TestUtil.haveMinimumServerVersion(conn, ServerVersion.v10)) {
@@ -268,6 +306,37 @@ public void testIdentityColumn() throws Exception {
    Assert.assertTrue(rsmd.isAutoIncrement(1));
  }

+  // Verifies that the field metadatacache will cache when enabled and also functions properly
+  // when disabled.
+  @Test
+  public void testCache() throws Exception {
+    boolean isCacheDisabled = new Integer(0).equals(databaseMetadataCacheFields)
+                           || new Integer(0).equals(databaseMetadataCacheFieldsMib);
+
+    {
+      PreparedStatement pstmt = conn.prepareStatement("SELECT a FROM rsmd_cache");
+      ResultSet rs = pstmt.executeQuery();
+      PGResultSetMetaData pgrsmd = (PGResultSetMetaData) rs.getMetaData();
+      assertEquals("a", pgrsmd.getBaseColumnName(1));
+      TestUtil.closeQuietly(rs);
+      TestUtil.closeQuietly(pstmt);
+    }
+
+    Statement stmt = conn.createStatement();
+    stmt.execute("ALTER TABLE rsmd_cache RENAME COLUMN a TO b");
+    TestUtil.closeQuietly(stmt);
+
+    {
+      PreparedStatement pstmt = conn.prepareStatement("SELECT b FROM rsmd_cache");
+      ResultSet rs = pstmt.executeQuery();
+      PGResultSetMetaData pgrsmd = (PGResultSetMetaData) rs.getMetaData();
+      // Unless the cache is disabled, we expect to see stale results.
+      assertEquals(isCacheDisabled ? "b" : "a", pgrsmd.getBaseColumnName(1));
+      TestUtil.closeQuietly(rs);
+      TestUtil.closeQuietly(pstmt);
+    }
+  }
+
  private void assumePreparedStatementMetadataSupported() {
    Assume.assumeTrue("prepared statement metadata is not supported for simple protocol",
        preferQueryMode.compareTo(PreferQueryMode.EXTENDED_FOR_PREPARED) >= 0);