Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect type lookup for array of off-path types #1948

Closed
MMeent opened this issue Nov 9, 2020 · 7 comments
Closed

Incorrect type lookup for array of off-path types #1948

MMeent opened this issue Nov 9, 2020 · 7 comments

Comments

@MMeent
Copy link
Contributor

MMeent commented Nov 9, 2020

Please read https://stackoverflow.com/help/minimal-reproducible-example

Describe the issue
If I define an array of a custom type in a table that is not on the search path / requires schema prefixing, then that array will fail to detect as an array.

The root cause of this issue is that the preparedStatement returned by TypeInfoCache#prepareGetTypeInfoStatement (and used by TypeInfoCache#getSQLType) is run with the type name as exposed to the user, i.e. quoted and with schema qualifications. As the schema qualified sql type cannot be found in the pg_catalog.pg_type catalog under that name, no entry is returned and the type is subsequently considered 'custom'.

Driver Version?
42.2.18

Java Version?
1.8.0_272

OS Version?
Debian Stretch (9.13)

PostgreSQL Version?
11.9

To Reproduce

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.Statement;
import java.util.Properties;

public class TestPostgresIssues {
    public static void main(String []args) throws Exception {
        String url = "jdbc:postgresql://localhost:5432/test";
        Properties props = new Properties();
        props.setProperty("user", "postgres");
        props.setProperty("password", "postgres");
        try (Connection conn = DriverManager.getConnection(url, props)) {
            // Create schema for testing. NOT on search_path.
            try (Statement statement = conn.createStatement()) {
                statement.execute("CREATE SCHEMA test");
            }
            // Create custom type in that schema
            try (Statement statement = conn.createStatement()) {
                statement.execute("CREATE TYPE test.test_enum AS ENUM ('val')");
            }
            // ... and a table that uses that type in an array.
            try (Statement statement = conn.createStatement()) {
                statement.execute("CREATE TABLE test.test_table (val test.test_enum[])");
            }
        }
        try (Connection conn = DriverManager.getConnection(url, props)) {
            ResultSet resultSet = conn.getMetaData().getColumns(null, "test", null, "%");
            resultSet.next();
            System.out.format("Type: [%s], JSQL-type: [%s]", resultSet.getString("TYPE_NAME"), resultSet.getString("DATA_TYPE"));
            // The only result we should be getting is a column of an array type (2003?). The current result is of unknown type (1111)
            if("1111".equals(resultSet.getString("DATA_TYPE"))) {
                throw new AssertionError("array is an unknown type");
            };
        }
    }
}

Expected behaviour
Schema discovery of the test schema will detect attribute 'vars' of table 'testtable' as an array type, which has component type 'testenum'.

@davecramer
Copy link
Member

Thought about this a bit and I don't think we want to do this.

What happens if I define a type in schema 1 and a type with the same name in schema 2. Ideally we only want to find the types on the search path.

@MMeent
Copy link
Contributor Author

MMeent commented Nov 9, 2020

I agree with the sentiment, but it currently fails to correctly resolve some types that are not on the path. The schema discovery should (in my humble opinion) not resolve column table(var test.test_enum) to public.test_enum because that test_enum is on the search path. It could very well result in incorrect behaviour.

I believe it is beneficial to instead update the code in getSQLType(oid) to query pg_type on OID directly instead of relying on the (poisoned with FQ names) oidToPgName cache, and then looking up the type by name in pg_type.

Reversing the relation between getSQLType(String) and getSQLType(oid) should be enough to fix this issue.

@davecramer
Copy link
Member

So how do you propose to deal with aliased types ?

@MMeent
Copy link
Contributor Author

MMeent commented Nov 10, 2020

I'm not quite sure what you mean by that. DatabaseMetadata#getColumns correctly uses TypeInfoCache#getSQLType(oid), which by definition should not have aliases/cannot shadow other type definitions (as type oids should be unique). The implementation of getSQLType(oid) then incorrectly assumes that getSQLType(name) will result in correct answers, but that function only gives correct results for names that are not fully qualified, and those names are only of the types that are visible on the search path (see TypeInfoCache#getPGType(oid))

@MMeent
Copy link
Contributor Author

MMeent commented Nov 10, 2020

@davecramer PR #1949 basically implements the changes required to fix the type discovery failure.

MMeent pushed a commit to MMeent/pgjdbc that referenced this issue Nov 11, 2020
…es not have the issue of name shadowing / qual-names, and has the added benefit of fixing pgjdbc#1948.

Types that are not on the search path (e.g. they are shadowed, or in a schema that is not included in the search path) are stored in the caches as fully qualified type names and OIDs. As we cannot easily query the pg_type catalog using qualified type names, we replace the pgTypeName with the oid of the type name to query properties of the type.

Testcases are added to improve coverage of correctly detecting SQL types that are not on the path, but are available through OID or qualified lookup. These types are stored internally as a fully qualified type, but we cannot use this name for lookup in pg_type.

Special consideration has been given to Oid.UNSPECIFIED, as that needs to be mapped to Types.OTHER without first hitting the database. That mapping is static, but is not in `types` because it is not an actual type.

Fixes pgjdbc#1948
@MMeent
Copy link
Contributor Author

MMeent commented Feb 22, 2021

@davecramer I see that 42.2.19 has been released, but it does not contain the fix for this issue. Would you know when the fix will be included in a release?

@jie-d
Copy link

jie-d commented Jun 29, 2021

+1 to the above question. 42.2.22 which was released two weeks ago still doesn't seem to contain this fix. Can we possibly include it in one of the upcoming releases? @davecramer

davecramer added a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
Fix: Rework sql type gathering to use OID instead of typname. This do…
…es not have the issue of name shadowing / qual-names, and has the added benefit of fixing pgjdbc#1948.

Types that are not on the search path (e.g. they are shadowed, or in a schema that is not included in the search path) are stored in the caches as fully qualified type names and OIDs. As we cannot easily query the pg_type catalog using qualified type names, we replace the pgTypeName with the oid of the type name to query properties of the type.

Testcases are added to improve coverage of correctly detecting SQL types that are not on the path, but are available through OID or qualified lookup. These types are stored internally as a fully qualified type, but we cannot use this name for lookup in pg_type.

Special consideration has been given to Oid.UNSPECIFIED, as that needs to be mapped to Types.OTHER without first hitting the database. That mapping is static, but is not in `types` because it is not an actual type.

Fixes pgjdbc#1948
davecramer added a commit that referenced this issue Jul 5, 2021
Fix: Rework sql type gathering to use OID instead of typname. This does not have the issue of name shadowing / qual-names, and has the added benefit of fixing #1948.

Types that are not on the search path (e.g. they are shadowed, or in a schema that is not included in the search path) are stored in the caches as fully qualified type names and OIDs. As we cannot easily query the pg_type catalog using qualified type names, we replace the pgTypeName with the oid of the type name to query properties of the type.

Testcases are added to improve coverage of correctly detecting SQL types that are not on the path, but are available through OID or qualified lookup. These types are stored internally as a fully qualified type, but we cannot use this name for lookup in pg_type.

Special consideration has been given to Oid.UNSPECIFIED, as that needs to be mapped to Types.OTHER without first hitting the database. That mapping is static, but is not in `types` because it is not an actual type.

Fixes #1948
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants