Re: Bug in TypeInfoCache causes getObject to fail - Mailing list pgsql-jdbc

Kris Jurka wrote:
> Nice work.  Applied to 8.1 and HEAD.

Thank you!

> Actually it is necessary for the maps to be synchronized because at any
> time a client may call PGConnection.addDataType which will modify the maps.

Ah, i forgot that the driver needs to be thread safe. I looked at it
again, and made a few changes to improve this in other fringe cases.

Mostly, i checked where and how the maps were used. I also noticed that
there are two PreparedStatements, which might be not thread safe (don't
know enough about the internals).

The only map that can be accessed from outside the class is
_pgNameToSQLType, via an iterator returned from
getPGTypeNamesWithSQLType(). That must obviously be thread safe, more on
that below. The other maps are not used outside the class, and can be
protected by synchronizing the accessing methods.

Also, the other methods often rely on a consistent relation between two
maps. A simple synchronizedMap ensures not that both maps are in a
consistent state relative to each other. Therefore any method accessing
any of these maps should be synchronized as a whole.

You wrote in the changelog that _pgNameToSQLType is actually static.
That can be done in a static initializer, and the member can be static
(and final) again. To protect it against any changes, and therefore make
it thread safe, an unmodifiableMap seems enough.

The methods using the PreparedStatements must be synchronized, because
the statements are only created when neccessary, so they can not be used
to synchronize on. They also fall under the category of methods that use
more than one map at once, see above.

I made another patch to the current branch, which i attached.

Hope some of this makes sense ;-)


Till
Index: TypeInfoCache.java
===================================================================
RCS file: /usr/local/cvsroot/pgjdbc/pgjdbc/org/postgresql/jdbc2/TypeInfoCache.java,v
retrieving revision 1.4
diff -u -r1.4 TypeInfoCache.java
--- TypeInfoCache.java    9 Feb 2006 16:29:06 -0000    1.4
+++ TypeInfoCache.java    9 Feb 2006 17:58:30 -0000
@@ -30,7 +30,7 @@
 public class TypeInfoCache {

     // pgname (String) -> java.sql.Types (Integer)
-    private Map _pgNameToSQLType;
+    private static final Map _pgNameToSQLType;

     // pgname (String) -> java class name (String)
     // ie "text" -> "java.lang.String"
@@ -48,7 +48,7 @@
     private PreparedStatement _getOidStatement;
     private PreparedStatement _getNameStatement;

-    private static Object types[][] = {
+    private static final Object types[][] = {
         {"int2", new Integer(Oid.INT2), new Integer(Types.SMALLINT), "java.lang.Integer"},
         {"int4", new Integer(Oid.INT4), new Integer(Types.INTEGER), "java.lang.Integer"},
         {"oid", new Integer(Oid.OID), new Integer(Types.INTEGER), "java.lang.Integer"},
@@ -71,28 +71,38 @@
         {"timestamptz", new Integer(Oid.TIMESTAMPTZ), new Integer(Types.TIMESTAMP), "java.sql.Timestamp"}
     };

+    static
+    {
+        Map pgNameToSQLType = new HashMap();
+        for (int i=0; i<types.length; i++) {
+            pgNameToSQLType.put(types[i][0], types[i][2]);
+            String arrayType = "_" + types[i][0];
+            pgNameToSQLType.put(arrayType, new Integer(Types.ARRAY));
+        }
+        // needs to be unmodifiable because the iterator is returned getPGTypeNamesWithSQLTypes()
+        // if the content of the map should ever need to be modified, this could be changed into a synchronizedMap()
+        _pgNameToSQLType = Collections.unmodifiableMap(pgNameToSQLType);
+    }
+
     public TypeInfoCache(BaseConnection conn)
     {
         _conn = conn;
-        _oidToPgName = Collections.synchronizedMap(new HashMap());
-        _pgNameToOid = Collections.synchronizedMap(new HashMap());
-        _pgNameToSQLType = Collections.synchronizedMap(new HashMap());
-        _pgNameToJavaClass = Collections.synchronizedMap(new HashMap());
-        _pgNameToPgObject = Collections.synchronizedMap(new HashMap());
+        _oidToPgName = new HashMap();
+        _pgNameToOid = new HashMap();
+        _pgNameToJavaClass = new HashMap();
+        _pgNameToPgObject = new HashMap();

         for (int i=0; i<types.length; i++) {
-            _pgNameToSQLType.put(types[i][0], types[i][2]);
             _pgNameToJavaClass.put(types[i][0], types[i][3]);
             _pgNameToOid.put(types[i][0], types[i][1]);
             _oidToPgName.put(types[i][1], types[i][0]);

             String arrayType = "_" + types[i][0];
-            _pgNameToSQLType.put(arrayType, new Integer(Types.ARRAY));
             _pgNameToJavaClass.put(arrayType, "java.sql.Array");
         }
     }

-    public void addDataType(String type, Class klass) throws SQLException
+    public synchronized void addDataType(String type, Class klass) throws SQLException
     {
         if (!PGobject.class.isAssignableFrom(klass))
             throw new PSQLException(GT.tr("The class {0} does not implement org.postgresql.util.PGobject.",
klass.toString()),PSQLState.INVALID_PARAMETER_TYPE); 
@@ -101,25 +111,25 @@
         _pgNameToJavaClass.put(type, klass.getName());
     }

-    public Iterator getPGTypeNamesWithSQLTypes()
+    public static Iterator getPGTypeNamesWithSQLTypes()
     {
-        return _pgNameToSQLType.keySet().iterator();
+        return _pgNameToSQLType.keySet().iterator(); // the map is unmodfiable
     }

-    public int getSQLType(int oid) throws SQLException
+    public int getSQLType(int oid) throws SQLException // no need to synchronize because getSQLType uses an
unmodfiablemap 
     {
         return getSQLType(getPGType(oid));
     }

-    public int getSQLType(String pgTypeName)
+    public static int getSQLType(String pgTypeName)
     {
-        Integer i = (Integer)_pgNameToSQLType.get(pgTypeName);
+        Integer i = (Integer)_pgNameToSQLType.get(pgTypeName); // map is unmodifiable, no need to synchronize
         if (i != null)
             return i.intValue();
         return Types.OTHER;
     }

-    public int getPGType(String pgTypeName) throws SQLException
+    public synchronized int getPGType(String pgTypeName) throws SQLException
     {
         Integer oid = (Integer)_pgNameToOid.get(pgTypeName);
         if (oid != null)
@@ -152,7 +162,7 @@
         return oid.intValue();
     }

-    public String getPGType(int oid) throws SQLException
+    public synchronized String getPGType(int oid) throws SQLException
     {
         if (oid == Oid.UNSPECIFIED)
             return null;
@@ -187,12 +197,12 @@
         return pgTypeName;
     }

-    public Class getPGobject(String type)
+    public synchronized Class getPGobject(String type)
     {
         return (Class)_pgNameToPgObject.get(type);
     }

-    public String getJavaClass(int oid) throws SQLException
+    public synchronized String getJavaClass(int oid) throws SQLException
     {
         String pgTypeName = getPGType(oid);
         return (String)_pgNameToJavaClass.get(pgTypeName);