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

fix: correct DatabaseMetaData.getFunction to have proper column name … #918

Merged
merged 1 commit into from Nov 8, 2017

Conversation

Projects
None yet
5 participants
@zemian
Contributor

zemian commented Aug 12, 2017

…and idx (#360)

@codecov-io

This comment has been minimized.

codecov-io commented Aug 12, 2017

Codecov Report

Merging #918 into master will increase coverage by <.01%.
The diff coverage is 90.9%.

@@             Coverage Diff              @@
##             master     #918      +/-   ##
============================================
+ Coverage     65.94%   65.95%   +<.01%     
- Complexity     3584     3588       +4     
============================================
  Files           167      167              
  Lines         15323    15333      +10     
  Branches       2481     2484       +3     
============================================
+ Hits          10105    10113       +8     
+ Misses         4043     4042       -1     
- Partials       1175     1178       +3
@davecramer

Why remove the function call ?

String sql;
sql = "SELECT NULL AS FUNCTION_CAT, n.nspname AS FUNCTION_SCHEM, p.proname AS FUNCTION_NAME, "
+ "d.description AS REMARKS, "
+ DatabaseMetaData.procedureReturnsResult + " AS FUNCTION_TYPE, "

This comment has been minimized.

@jorsol

jorsol Aug 12, 2017

Contributor

Since we don't know the FUNCTION_TYPE the correct value should be DatabaseMetaData.functionResultUnknown

This comment has been minimized.

@zemian

zemian Aug 12, 2017

Contributor

Hi @jorsol, hum... I actually just took the original query from getProcedures and fix the column names and removed extra reserved fields. I didn't check this "FUNCTION_TYPE" is actually valid or not, since I assume it was working before. Let me take a look again...

This comment has been minimized.

@zemian

zemian Aug 12, 2017

Contributor

@jorsol ah yes, I see what you mean now. I can fix it with DatabaseMetaData.functionResultUnknown. BTW, how come we choose to always give unknown type? Can't we calculate the correct value from pg_catalog.pg_proc.prorettype?

This comment has been minimized.

@zemian

zemian Aug 12, 2017

Contributor

I also noticed couple of joins are not used, so I will simplified even further on the query.

This comment has been minimized.

@jorsol

jorsol Aug 12, 2017

Contributor

@zemian , probably by joining pg_catalog.pg_proc.prorettype with pg_catalog.pg_type.oid and filter that pg_catalog.pg_type.typname = 'record' is functionReturnsTable else functionNoTable would do the trick.

Or maybe something like this makes sense:

sql = "SELECT NULL AS FUNCTION_CAT, n.nspname AS FUNCTION_SCHEM, "
+ "p.proname AS FUNCTION_NAME, d.description AS REMARKS, "
+ "CASE WHEN (format_type(p.prorettype, null) = 'record') THEN " 
+ DatabaseMetaData.functionReturnsTable + " ELSE " 
+ DatabaseMetaData.functionNoTable + " END AS FUNCTION_TYPE, "
+ "p.proname || '_' || p.oid AS SPECIFIC_NAME "
+ "FROM pg_catalog.pg_proc p "
+ "INNER JOIN pg_catalog.pg_namespace n ON (p.pronamespace=n.oid) "
+ "LEFT JOIN pg_catalog.pg_description d ON (p.oid=d.objoid)"

I'm not totally sure that 'record' type is equivalent to a table always, I guess this whould require more research and testing.

BTW, if this approach is fine it would be nice to fix getProcedures too in another PR.

This comment has been minimized.

@zemian

zemian Aug 13, 2017

Contributor

Hm... what about function that returns refcursor?

This comment has been minimized.

@zemian

zemian Aug 13, 2017

Contributor

Hm... you can use 'Row Type' as function return type, which can be a table name itself! So this can be harder to detect.

This comment has been minimized.

@davecramer

davecramer Aug 13, 2017

Member

FYI this is what psql does:

SELECT n.nspname as "Schema",
  p.proname as "Name",
  pg_catalog.pg_get_function_result(p.oid) as "Result data type",
  pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types",
 CASE
  WHEN p.proisagg THEN 'agg'
  WHEN p.proiswindow THEN 'window'
  WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN 'trigger'
  ELSE 'normal'
 END as "Type"
FROM pg_catalog.pg_proc p
     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
WHERE pg_catalog.pg_function_is_visible(p.oid)
      AND n.nspname <> 'pg_catalog'
      AND n.nspname <> 'information_schema'
ORDER BY 1, 2, 4;

This comment has been minimized.

@zemian

zemian Aug 13, 2017

Contributor

Thanks @davecramer for this query tips, which give us more to play with. But this query only return user defined functions though, and the getFunctions() API javadoc says it should return both system + user function list.

Also, the getFunctions() API says one of the field:

FUNCTION_TYPE short => kind of function:
functionResultUnknown - Cannot determine if a return value or table will be returned
functionNoTable- Does not return a table
functionReturnsTable - Returns a table

Would the "table" here means anything that will resulted in a "ResultSet" object return by the function? In that case, would result type of "refcursor" or "rowtype (table)" should be considered true or not?

For "refcursor" type we can check easily, but for this "rowtype (table)", the returned value is an actual table name. So this might complicate things.

This comment has been minimized.

@davecramer

davecramer Aug 14, 2017

Member

Welcome to the ambiguity of JDBC. I would say we are free to implement it as we see fit and then argue later with people who see it differently. I would say rowtype(table) returns true. refcursor, false.

@zemian

This comment has been minimized.

Contributor

zemian commented Aug 12, 2017

@davecramer, I am not clear with the question. Are you asking why "remove" line 2544 getProcedures() in the patch? As I mentioned in the (#360) comment that getFunctions() is still broken because you can't simply call getProcedures() as result underneath. Per JDBC API getFunctions() javadoc, it supposed to return different column names, and there few reserved columns that should NOT be there compare to getProcedures().

@zemian

This comment has been minimized.

Contributor

zemian commented Aug 16, 2017

Hi, I have updated the code with a better query now. Please review.

@zemian

This comment has been minimized.

Contributor

zemian commented Aug 17, 2017

Finally, the last commit is now green by CI. Please review.

// The pg_get_function_result only exists 8.4 or later
String ver = connection.getDBVersionNumber();
Version dbVer = ServerVersion.from(ver);
boolean pgFuncResultExists = dbVer.getVersionNum() >= ServerVersion.v8_4.getVersionNum();

This comment has been minimized.

@jorsol

jorsol Sep 2, 2017

Contributor

I think the proper way to check server version support is using connection.haveMinimumServerVersion(ServerVersion.v8_4).

This comment has been minimized.

@zemian

zemian Sep 5, 2017

Contributor

@jorsol, Thanks for the good suggestion. I pushed an update for this now.

@zemian

This comment has been minimized.

Contributor

zemian commented Sep 7, 2017

I see CI failed on PG 9.4 and 9.2 alone with something not related to this PR? Let me know if there is something else I can help.

`[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test (default-test) on project postgresql: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test failed: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?

[ERROR] Command was /bin/sh -c cd /home/travis/build/pgjdbc/pgjdbc/pgjdbc-jre6 && /usr/lib/jvm/java-6-openjdk-amd64/bin/java -Xmx512m -jar /home/travis/build/pgjdbc/pgjdbc/pgjdbc-jre6/target/surefire/surefirebooter6814232988850463543.jar /home/travis/build/pgjdbc/pgjdbc/pgjdbc-jre6/target/surefire/surefire8491128135780131858tmp /home/travis/build/pgjdbc/pgjdbc/pgjdbc-jre6/target/surefire/surefire_010313453588960797tmp`

@jorsol

This comment has been minimized.

Contributor

jorsol commented Sep 7, 2017

Yes, there is an issue with openjdk6 on travis, I'm working on a fix #939.

// Build query and result
String sql;
sql = "SELECT NULL AS FUNCTION_CAT, n.nspname AS FUNCTION_SCHEM, p.proname AS FUNCTION_NAME, "

This comment has been minimized.

@jorsol

jorsol Sep 21, 2017

Contributor

I think it's safe to use current_catalog for FUNCTION_CAT instead of null.

This comment has been minimized.

@jorsol

jorsol Sep 22, 2017

Contributor

Actually current_database() is more appropriate since current_catalog is for Pg8.4+

+ funcTypeSql + " AS FUNCTION_TYPE, "
+ " p.proname || '_' || p.oid AS SPECIFIC_NAME "
+ "FROM pg_catalog.pg_proc p "
+ "LEFT JOIN pg_catalog.pg_namespace n ON p.pronamespace=n.oid "

This comment has been minimized.

@jorsol

jorsol Sep 21, 2017

Contributor

Schemas should always exist, so using an INNER JOIN should be more appropriate here.

@vlsi vlsi added the needs-review label Sep 25, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Oct 6, 2017

@zemian this looks good, but the test is pretty basic, it would be great to have many test that works with many combinations.

One idea could be to have a test that creates a dummy function that return a table, another test that creates a dummy function that return nothing (and in other schema passing schemaPattern), another test that return a value, and test the correct output not just that it's not null.

And if it's not hard, a test that check they are ordered by FUNCTION_CAT, FUNCTION_SCHEM, FUNCTION_NAME and SPECIFIC_ NAME. So if someone changes this code in the future, this will act as a regression test.

A check for the FUNCTION_CAT should not be on the value, since if I change the name of the database in the property file to something else, that should not break.

As a side note the testGetProcedures() should not be in this PR, and AFAICT the DatabaseMetaData.getProcedures() needs the same lifting as getFunctions(), so another PR replicating some changes of this PR in DatabaseMetaData.getProcedures() would be great.

Thank you for your patience and hard work.

@zemian

This comment has been minimized.

Contributor

zemian commented Nov 2, 2017

HI @jorsol, sorry for the delay on this. I have little more time and will try to add more tests. And yes I won't touch GetProcedures() on this PR.

Upon revisit this again, I see the JDBC API documentation stated the following on parameters:

Parameters:
catalog - a catalog name; must match the catalog name as it is stored in the database; "" retrieves those without a catalog; null means that the catalog name should not be used to narrow the search
schemaPattern - a schema name pattern; must match the schema name as it is stored in the database; "" retrieves those without a schema; null means that the schema name should not be used to narrow the search
functionNamePattern - a function name pattern; must match the function name as it is stored in the database

Can you help clarify when catalog ="" (retrieves those without a catalog) would mean in PGJDBC? As you reviewed the current implementation, it does not thing special about this parameter and always return current databae as catalog only!

Also the javadoc does not seem to allow NULL value to pass for functionNamePattern parameter ? Or are just treating null value as match all?

@jorsol

This comment has been minimized.

Contributor

jorsol commented Nov 2, 2017

Can you help clarify when catalog ="" (retrieves those without a catalog) would mean in PGJDBC? As you reviewed the current implementation, it does not thing special about this parameter and always return current databae as catalog only!

The JDBC api is database agnostic, and in PostgreSQL there is always a name of current database (called “catalog” in the SQL standard), it's never "" (empty).

PostgreSQL don't allow cross database queries, so it should not matter if the method completely ignores the catalog parameter (should not be used to narrow the search) or in other words, it behaves as NULL always. Maybe a really strict jdbc implementation should make the difference so calling with catalog "tests" should not return nothing for database "test" (another filter for current_database() = catalog), but this is a grey area in the specification.

Also the javadoc does not seem to allow NULL value to pass for functionNamePattern parameter ? Or are just treating null value as match all?

IMO that is another annoying thing of the specification, again, a strict jdbc implementation should not allow NULL value, and one should have to pass "%" as functionNamePattern to get all functions, but I don't really see any reason to not treat null value as match all (beyond that using null is a bad practice :P) as with schemaPattern.

Now that you bring this to light, the schemaPattern "" retrieves those without a schema, since PostgreSQL always have schemas, the empty schemaPattern "" should not return nothing.

@zemian

This comment has been minimized.

Contributor

zemian commented Nov 3, 2017

Thanks for the input @jorsol I have improved the tests per your suggestion.

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 7, 2017

@zemian
I was going to squash all of the commits into one, but realized the remaining commit message was not very clear.

Could you find time to summarize the changes better ?

fix: Correct DatabaseMetaData.getFunctions() implementation
The old DatabaseMetaData.getFunctions() method return incorrect column names and
function return types. The new implementations comply with the JDBC docs.
@zemian

This comment has been minimized.

Contributor

zemian commented Nov 7, 2017

Hi @davecramer. Sure. I've squashed the PR down to 1 commit with better message now.

@davecramer davecramer merged commit 8884202 into pgjdbc:master Nov 8, 2017

2 checks passed

codecov/project 65.95% (+<.01%) compared to 2277ffb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: Correct DatabaseMetaData.getFunctions() implementation (pgjdbc#918)
The old DatabaseMetaData.getFunctions() method returned incorrect column names and
function return types. The new implementation complies with the JDBC docs.

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: Correct DatabaseMetaData.getFunctions() implementation (pgjdbc#918)
The old DatabaseMetaData.getFunctions() method returned incorrect column names and
function return types. The new implementation complies with the JDBC docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment