Обсуждение: [JDBC] TypeInfoCache.getPGType(pgTypeName) drops schema name when caching

When populating the _oidToPgName and _pgNameToOid caches, TypeInfoCache.getPGType(String pgTypeName) populates
_oidToPgNamewith only pg_type.typname regardless of typnamespace or the search path. This results in the cache
returningunqualified type names in lookups to cached values by getPGType(int oid). 

It makes sense to normalize the name, which is presumably what motivates using internalName returned from oidStatement
ratherthan the user-supplied pgTypeName. getPGType(oid) does schema-qualify the name used to populate _oidToPgName: 

_pgNameToOid.put(schema + "." + name, oid)

I think it makes sense to follow this behavior in getPGType(pgTypeName). Looking at it briefly,   I wonder if adding
"nspname= ANY(current_schemas(true))" and nspname columns to the results of  oidStatement would be sufficient, and
usingthose to construct a normalized qualified type name in the manner of getPGType(oid). 

If this makes sense, I'm happy to submit a patch with tests.

Cheers,

Michael Glaesemann
grzm seespotcode net


PS. When looking through open issues to see if this had been reported, I came across
https://github.com/pgjdbc/pgjdbc/issues/189"Connection.createArrayOf() does not handle enums correctly.". I think what
Idescribe above is the root cause of this as well, as TypeInfoCache.getPGArrayType calls getPGType(pgTypeName). 

> On 2017-07-16, at 01:53, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
> Michael>_pgNameToOid.put(schema + "." + name, oid)
> Michael>I think it makes sense to follow this behavior in getPGType(pgTypeName).
>
> The idea of type cache is to avoid pg_catalog SQLs on the hot path.
> Consider a user issuing `createArrayOf`. Do you mean pgjdbc should query the catalog on each invocation of
createArrayOf?I'm not sure it would be the right thing, as it is basically the only way to create arrays. 

Sorry if I wasn't clear. I understand the purpose of the cache and agree that make sense to cache type information.
WhatI'm trying to get at is the cached name values are different depending on whether getPGType(pgTypeName) or
getPGPGType(oid)are used resulting in inconsistent behavior. 

Here's an example that illustrates what I'm seeing.

> cat GetPGTypeTest.java
/**
   export CLASSPATH="postgresql-42.1.3.jar:."

   javac GetPGTypeTest.java && java -Djdbc.drivers=org.postgresql.Driver GetPGTypeTest

   export PGPORT=5496
   creatdb get_pg_type_test;
   psql -d get_pg_type_test -c "CREATE SCHEMA cars;"
   psql -d get_pg_type_test -c "CREATE TYPE cars.color AS ENUM ('black')"

   Any customer can have a car painted any color that he wants so long as it is black.
   -- Henry Ford
*/

import java.sql.*;
import org.postgresql.util.*;

public class GetPGTypeTest
{
    static String url = "jdbc:postgresql://localhost:5496/get_pg_type_test";

    static String sql = "SELECT CAST(? AS cars.color)";
    static String color = "black";
    static String colorType = "cars.color";

    static String getColumnTypeName(PreparedStatement st) throws SQLException {
        ResultSet rs = st.executeQuery();
        ResultSetMetaData md = rs.getMetaData();
        rs.close();
        return md.getColumnTypeName(1);
    }

    static String getStringTypeName(Connection conn) throws SQLException {
        PreparedStatement st = conn.prepareStatement(sql);
        st.setString(1, color);
        return getColumnTypeName(st);
    }

    static String getObjectTypeName(Connection conn) throws SQLException {
        PreparedStatement st = conn.prepareStatement(sql);
        PGobject colorObject = new PGobject();
        colorObject.setType(colorType);
        colorObject.setValue(color);
        st.setObject(1, colorObject);
        return getColumnTypeName(st);
    }

    public static void main(String[] args) {

        Connection conn = null;

        try {
            System.out.println("## setString first -- caching via getPGType(oid)");
            conn = DriverManager.getConnection(url);
            System.out.format("string: %s%n", getStringTypeName(conn));
            System.out.format("object: %s%n", getObjectTypeName(conn));

            System.out.println("## setObject first -- caching via getPGType(pgTypeName)");
            conn = DriverManager.getConnection(url);
            System.out.format("object: %s%n", getObjectTypeName(conn));
            System.out.format("string: %s%n", getStringTypeName(conn));
        }
        catch (SQLException e) {
            System.err.println(e.getMessage());
            System.exit(1);
        }

    }
}

> export CLASSPATH="postgresql-42.1.3.jar:."
> javac GetPGTypeTest.java
> java -Djdbc.drivers=org.postgresql.Driver GetPGTypeTest
## setString first -- caching via getPGType(oid)
string: "cars"."color"
object: "cars"."color"
## setObject first -- caching via getPGType(pgTypeName)
object: color
string: color

The caching to the cars.colors type name is different depending on whether getPGType(pgTypeName) and getPGType(oid) is
doingthe caching. getPGType(pgTypeName) doesn't include the schema name when the type name is cached. This is clear
fromthe differences in code used to generate the type names: 


In getPGType(pgTypeName):

https://github.com/pgjdbc/pgjdbc/blob/cb3995b5a0311a2f5f7737fdfe83457680305efb/pgjdbc/src/main/java/org/postgresql/jdbc/TypeInfoCache.java#L376-L377

String internalName = rs.getString(2); // typname, from getOidStatement
_oidToPgName.put(oid, internalName);


In getPGType(oid):

https://github.com/pgjdbc/pgjdbc/blob/cb3995b5a0311a2f5f7737fdfe83457680305efb/pgjdbc/src/main/java/org/postgresql/jdbc/TypeInfoCache.java#L412-L432

pgTypeName = "\"" + schema + "\".\"" + name + "\"";
// ...
_oidToPgName.put(oid, pgTypeName);


The behavior is inconsistent and arguably wrong in the case of getPGType(pgTypeName): if I have types with the same
pg_type.typnamebut in different schemas, ResultSetMetaData.getColumnTypeName doesn't provide a way to distinguish
betweenthem if getPGType(pgTypeName) is doing the caching. 

I hope this is a clearer explanation of the behavior. If it isn't, please let me know.

Best,

Michael Glaesemann
grzm seespotcode net

> On 2017-07-16, at 01:53, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
> Michael>_pgNameToOid.put(schema + "." + name, oid)
> Michael>I think it makes sense to follow this behavior in getPGType(pgTypeName).
>
> The idea of type cache is to avoid pg_catalog SQLs on the hot path.
> Consider a user issuing `createArrayOf`. Do you mean pgjdbc should query the catalog on each invocation of
createArrayOf?I'm not sure it would be the right thing, as it is basically the only way to create arrays. 

Sorry if I wasn't clear. I understand the purpose of the cache and agree that make sense to cache type information.
WhatI'm trying to get at is the cached name values are different depending on whether getPGType(pgTypeName) or
getPGPGType(oid)are used resulting in inconsistent behavior. 

Here's an example that illustrates what I'm seeing.

> cat GetPGTypeTest.java
/**
   export CLASSPATH="postgresql-42.1.3.jar:."

   javac GetPGTypeTest.java && java -Djdbc.drivers=org.postgresql.Driver GetPGTypeTest

   export PGPORT=5496
   creatdb get_pg_type_test;
   psql -d get_pg_type_test -c "CREATE SCHEMA cars;"
   psql -d get_pg_type_test -c "CREATE TYPE cars.color AS ENUM ('black')"

   Any customer can have a car painted any color that he wants so long as it is black.
   -- Henry Ford
*/

import java.sql.*;
import org.postgresql.util.*;

public class GetPGTypeTest
{
    static String url = "jdbc:postgresql://localhost:5496/get_pg_type_test";

    static String sql = "SELECT CAST(? AS cars.color)";
    static String color = "black";
    static String colorType = "cars.color";

    static String getColumnTypeName(PreparedStatement st) throws SQLException {
        ResultSet rs = st.executeQuery();
        ResultSetMetaData md = rs.getMetaData();
        rs.close();
        return md.getColumnTypeName(1);
    }

    static String getStringTypeName(Connection conn) throws SQLException {
        PreparedStatement st = conn.prepareStatement(sql);
        st.setString(1, color);
        return getColumnTypeName(st);
    }

    static String getObjectTypeName(Connection conn) throws SQLException {
        PreparedStatement st = conn.prepareStatement(sql);
        PGobject colorObject = new PGobject();
        colorObject.setType(colorType);
        colorObject.setValue(color);
        st.setObject(1, colorObject);
        return getColumnTypeName(st);
    }

    public static void main(String[] args) {

        Connection conn = null;

        try {
            System.out.println("## setString first -- caching via getPGType(oid)");
            conn = DriverManager.getConnection(url);
            System.out.format("string: %s%n", getStringTypeName(conn));
            System.out.format("object: %s%n", getObjectTypeName(conn));

            System.out.println("## setObject first -- caching via getPGType(pgTypeName)");
            conn = DriverManager.getConnection(url);
            System.out.format("object: %s%n", getObjectTypeName(conn));
            System.out.format("string: %s%n", getStringTypeName(conn));
        }
        catch (SQLException e) {
            System.err.println(e.getMessage());
            System.exit(1);
        }

    }
}

> export CLASSPATH="postgresql-42.1.3.jar:."
> javac GetPGTypeTest.java
> java -Djdbc.drivers=org.postgresql.Driver GetPGTypeTest
## setString first -- caching via getPGType(oid)
string: "cars"."color"
object: "cars"."color"
## setObject first -- caching via getPGType(pgTypeName)
object: color
string: color

The caching to the cars.colors type name is different depending on whether getPGType(pgTypeName) and getPGType(oid) is
doingthe caching. getPGType(pgTypeName) doesn't include the schema name when the type name is cached. This is clear
fromthe differences in code used to generate the type names: 


In getPGType(pgTypeName):

https://github.com/pgjdbc/pgjdbc/blob/cb3995b5a0311a2f5f7737fdfe83457680305efb/pgjdbc/src/main/java/org/postgresql/jdbc/TypeInfoCache.java#L376-L377

String internalName = rs.getString(2); // typname, from getOidStatement
_oidToPgName.put(oid, internalName);


In getPGType(oid):

https://github.com/pgjdbc/pgjdbc/blob/cb3995b5a0311a2f5f7737fdfe83457680305efb/pgjdbc/src/main/java/org/postgresql/jdbc/TypeInfoCache.java#L412-L432

pgTypeName = "\"" + schema + "\".\"" + name + "\"";
// ...
_oidToPgName.put(oid, pgTypeName);


The behavior is inconsistent and arguably wrong in the case of getPGType(pgTypeName): if I have types with the same
pg_type.typnamebut in different schemas, ResultSetMetaData.getColumnTypeName doesn't provide a way to distinguish
betweenthem if getPGType(pgTypeName) is doing the caching. 

I hope this is a clearer explanation of the behavior. If it isn't, please let me know.

Best,

Michael Glaesemann
grzm seespotcode net

> On 2017-07-16, at 11:21, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
> Michael>The caching to the cars.colors type name is different depending on whether getPGType(pgTypeName) and
getPGType(oid)is doing the caching. getPGType(pgTypeName) doesn't include the schema name  
>
> This does sound odd on the first sight.
> You might try to file a PR to see if the change would fail some existing tests.

Will do. Thanks!

>
> There was a recent change
https://github.com/pgjdbc/pgjdbc/commit/cddcd185cc71267e8eb177c6ba1dab3f029da089#diff-9cedabddb25727db7a1346738c8b7b5dR522
thatdid touch _pgNameToOid. 
> We might want to use some common approach for _pgNameToOid.
>
> Vladimir

Michael Glaesemann
grzm seespotcode net

> On 2017-07-16, at 11:21, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
> Michael>The caching to the cars.colors type name is different depending on whether getPGType(pgTypeName) and
getPGType(oid)is doing the caching. getPGType(pgTypeName) doesn't include the schema name  
>
> This does sound odd on the first sight.
> You might try to file a PR to see if the change would fail some existing tests.

Will do. Thanks!

>
> There was a recent change
https://github.com/pgjdbc/pgjdbc/commit/cddcd185cc71267e8eb177c6ba1dab3f029da089#diff-9cedabddb25727db7a1346738c8b7b5dR522
thatdid touch _pgNameToOid. 
> We might want to use some common approach for _pgNameToOid.
>
> Vladimir

Michael Glaesemann
grzm seespotcode net