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

PgDatabaseMetaData.getColumns() incorrectly represents null column scale. #1712

Closed
2 tasks done
crwr45 opened this issue Feb 21, 2020 · 2 comments
Closed
2 tasks done

Comments

@crwr45
Copy link
Contributor

crwr45 commented Feb 21, 2020

I'm submitting a ...

  • bug report
  • feature request

Describe the issue
When a Column is of type Numeric and has null precision and scale the column metadata for that column as reported by PgDatabaseMetaData.getColumns() here will return a scale of 0.
This has a very different meaning to null scale; the PostgreSQL docs says that PostgreSQL does not follow the SQL standard of defaulting to 0 scale in this case and instead such a column will store "values of any precision and scale up to the implementation limit". In the case we are dealing with, we potentially do not have sufficient control to specify the precision and scale on a column.

getColumns() gets the value of atttypmod from pg_catalog.pg_attribute. In the case we are interested in here, this will have a value of -1. TypeInfoCache.getScale() then explicitly returns a 0 for this case.

Therefore the instance of PgResultSet that represents column metadata has an explicit 0 for the scale and means that the caller cannot tell if the column is actually scale=0 or scale=null even by checking PgResultSet.wasNull(), with potentially significant knock-on effects from this.

The replacement of -1 with 0 is also slightly at odds with the implementation of PgResultSet.scaleBigDecimal() that will treat scale == -1 as an instruction to not scale, while scale == 0 will often crash at val.setScale(scale); attempting to scale with no rounding.

Driver Version?
Present at the least in both v9.4.1212 and v42.2.9.

Java Version + OS Version + PostgreSQL Version
Used the postgres Docker container, on Ubuntu 18.04

To Reproduce
Steps to reproduce the behaviour:

  1. Create a table with an unscaled Numeric column.
  2. Call PgDatabaseMetaData.getColumns() on that table.
  3. Examine the column as described in the PgResultSet and compare it to the table as reported within PSQL (please excuse unoriginal naming):
test=# \pset null 'Ø'
Null display is "Ø".
test=# SELECT column_name, data_type, numeric_precision, numeric_precision_radix, numeric_scale FROM information_schema.columns WHERE table_name = 'test';
 column_name |          data_type          | numeric_precision | numeric_precision_radix | numeric_scale 
-------------+-----------------------------+-------------------+-------------------------+---------------
 test        | numeric                     |                 Ø |                      10 |             Ø

getColumns claims scale is 0, The query above shows it's null.

Expected behaviour
It should be possible to tell when a scale is null when calling getColumns.

I am not familiar with the inner workings of PostgreSQL or JDBC so I mostly would like to understand why it works this way, and make one suggestion.

I've always seen and used information_schema.columns to find information about a column definition. Why doesn't getColumns() use this? I've not found anything conclusive.

Possible change

Change getColumns() to return null for an unscaled columns scale. This would allow the caller to use ResultSet.wasNull() to see what the actual scale is.
Since the PgResultSet.getInt method returns 0 for a column that is actually null, uninterested callers could still use getInt(9) with no change to behaviour, but the information about the nullness of the column is available to the caller if they choose to examine further using the mechanisms already available in a ResulstSet.

Example usage from Kafka Connect JDBC Connector (my use case):

diff --git a/src/main/java/io/confluent/connect/jdbc/dialect/GenericDatabaseDialect.java b/src/main/java/io/confluent/connect/jdbc/dialect/GenericDatabaseDialect.java
index 48fbc1f..af48732 100644
--- a/src/main/java/io/confluent/connect/jdbc/dialect/GenericDatabaseDialect.java
+++ b/src/main/java/io/confluent/connect/jdbc/dialect/GenericDatabaseDialect.java
@@ -596,7 +596,9 @@ public class GenericDatabaseDialect implements DatabaseDialect {
         final int jdbcType = rs.getInt(5);
         final String typeName = rs.getString(6);
         final int precision = rs.getInt(7);
-        final int scale = rs.getInt(9);
+        int scale = rs.getInt(9);
+        if (rs.wasNull()) {
+          scale = NUMERIC_TYPE_SCALE_UNSET;
+        }
         final String typeClassName = null;
         Nullability nullability;
         final int nullableValue = rs.getInt(11);
@davecramer
Copy link
Member

Well as to why it does not use information schema. Mostly performance, some historical when information schema wasn't complete.
I don't have any objection to your suggestion of how to deal with this. Can you provide a PR (without using information schema) ?

It looks like we are going to bump the version to 4.3.0 so this change would be fine as it it is slightly breaking.

Thanks!

@davecramer
Copy link
Member

closed with #1716

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

2 participants