HHH-15451 Upgrade PostgreSQL JDBC driver to 42.5.0

[Sanne]

https://hibernate.atlassian.net/browse/HHH-15451

[beikov]

I just checked why the update makes a test fail and reported the following regression for the pgjdbc driver: pgjdbc/pgjdbc#2597

[davecramer]

I think the correct option is float -> float4, not float8

This change was done quite a while ago, seems odd to catch it now ?

[beikov]

I think the correct option is float -> float4, not float8

It's a breaking change after all. Our "simple logic" transforms float(53), the base DDL type of the element type, for the createArray invocation to float, because specifying precision is unsupported for that method.

This change was done quite a while ago, seems odd to catch it now ?

Well, we just tried to update the driver version now. Sorry we didn't report it earlier.

[davecramer]

I'm not denying it was a breaking change, but reverting it after 2 years would also be a breaking change.

[beikov]

I'm not denying it was a breaking change, but reverting it after 2 years would also be a breaking change.

So what do we do? Do I understand it correctly that you reject reverting the mapping to float -> float8?

[davecramer]

So what do we do? Do I understand it correctly that you reject reverting the mapping to float -> float8?

I'd like to hear from the hibernate folks here.
My preference is not to revert it due to breaking code that has existed for 2 years.
But I'm willing to discuss it.

[beikov]

I'd like to hear from the hibernate folks here.

You heard my (Hibernate folk) opinion. It's a breaking change and as such, I think this should be fixed. Maybe other (possibly non-Hibernate) users who didn't update their drivers yet will run into this sooner or later and report that as well. Or maybe nobody relied on this except for our implementation. Maybe nobody started to rely on the float -> float4 mapping in the last 2 years, we simply don't know.

We could of course do something about this in Hibernate, by introducing a new SPI to handle the determination of the array base type name through the Dialect, but I'm pretty sure that the SQL standard says something about float having a default precision of 53. At least the PostgreSQL documentation says that float without precision is interpreted as double precision (see the last sentence of the section https://www.postgresql.org/docs/current/datatype-numeric.html#id-1.5.7.9.8.16 )

[davecramer]

OK, leave it with me. I'm pinned today. Thanks for this perspective. I'm now leaning towards reverting it.

[Sanne]

thank you both :)

I'm a bit shocked that it takes us two years to detect such a regression; I suppose I've not been on top of driver updates on the v.6 branch (as v.5 was the "production" branch until recently), but let's figure out how to automate this going forward.

[davecramer]

I'm a bit shocked that it takes us two years to detect such a regression; I suppose I've not been on top of driver updates on the v.6 branch (as v.5 was the "production" branch until recently), but let's figure out how to automate this going forward.

I wonder if there is a way for pgJDBC to run your tests when we merge ?

Dave

[Sanne]

I wonder if there is a way for pgJDBC to run your tests when we merge ?

Dave

That would be awesome! We'd be happy to help set it up: running the Hibernate ORM tests is not particularly hard, normally the most clumsy aspect for us is to make sure each DB is running and ready.

What kind of CI is being used? We're using a combination of Github Actions and Jenkins; https://ci.hibernate.org/

In a nutshell, it would need to run ./gradlew test -Pdb=pgsql_ci with JDK11; this is expecting access to a DB with particular credentials as defined in:

https://github.com/hibernate/hibernate-orm/blob/main/gradle/databases.gradle#L57-L65

We can also add a new stanza in that file to make it easier.

It might be interesting to test with more JDKs, but I'd start with a vanilla OpenJDK11 - anything more would be diminishing returns. (we also test with JDK8 for older branches, and JDK17 and 18, 19.. and other vendors or archs but I'd suspect it would be overkill in this context)

[beikov]

I wonder if there is a way for pgJDBC to run your tests when we merge ?

I guess Quarkus chose the version 999-SNAPSHOT to support exactly this kind of thing. In such tests, one can always rely on that version to be "latest", but there are other ways to implement this process as well.

I would suggest we make it possible to override the driver version from the CLI. Then we setup a GitHub Actions workflow for pgjdbc which will checkout Hibernate main and run the testsuite with PostgreSQL once a day. Would that work?

[davecramer]

I was thinking we run the test when we merge.

once a day seems quite often.

[beikov]

I don't know how often you push changes, but sure, doing it on every push is fine as well.

[vlsi]

In a nutshell, it would need to run ./gradlew test -Pdb=pgsql_ci with JDK11; this is expecting access to a DB with particular credentials as defined in:

it is probably a good idea to isolate pgjdbc from knowing Hibernate build script internals.
What do you think if you house a shell script that would execute tests with a given pgjdbc version?

E.g. something like PG_URL=... PG_USERNAME=... PG_PASSWORD=... PGJDBC_VERSION=... MAVEN_REPO_LOCATION=... ./run_tests

Then we (pgjdbc) could integrate "clone hibernate & run_tests` into our CI pipeline, and you will know precisely what do we call. So you have certain freedom in reorganizing the build logic (e.g. shuffle modules, change build system)

Of course, Hibernate already uses Gradle, so I don't expect accidental flip to Maven or Bazel, however, I could easily imagine you could restructure modules, so having a script that launches test would probably make sense in the long run.

WDYT?

[davecramer]

We currently have an action which clones debezium and runs tests. I don't see why we couldn't do the same thing here ?

[davecramer]

see pgjdbc/pgjdbc#2598 for fix.

[davecramer]

So regardless which postgres version I run I only get the following errors

XmlMappingTests$Jackson > verifyMappings(SessionFactoryScope) FAILED
    jakarta.persistence.PersistenceException at ExceptionConverterImpl.java:165
        Caused by: org.hibernate.exception.GenericJDBCException at StandardSQLExceptionConverter.java:61
            Caused by: org.postgresql.util.PSQLException at QueryExecutorImpl.java:2676

XmlMappingTests$Jaxb > verifyMappings(SessionFactoryScope) FAILED
    jakarta.persistence.PersistenceException at ExceptionConverterImpl.java:165
        Caused by: org.hibernate.exception.GenericJDBCException at StandardSQLExceptionConverter.java:61
            Caused by: org.postgresql.util.PSQLException at QueryExecutorImpl.java:2676

which is expected as I don't have xml enabled on my version of postgres

Thoughts ?

[Sanne]

Regarding the local Maven repository, you should be able to use the Environment variable ADDITIONAL_REPO: point it to the directory of the root of your local maven repo, or wherever the pgjdbc lib was published.

I believe there's also a Gradle "native" way to directly consume the artifact from one gradle project into another.. but maybe I'm overcomplicating. @sebersole WDYT?

Regarding the XML mapping.. I'm not sure, I hope @beikov can help - we can certainly add some more parameters as needed.

What about Spatial types.. do you have those supported in your version of the DB?

[vlsi]

ADDITIONAL_REPO: point it to the directory of the root of your local maven repo, or wherever the pgjdbc lib was published

I believe that is the way to go.

I believe there's also a Gradle "native" way to directly consume the artifact from one gradle project into another.. but maybe I'm overcomplicating. @ sebersole WDYT?

Even though Gradle has nice integration, I believe it is better to use for debugging purposes.
However, if we setup tight integration right now, it might become painful if Hibernate adjusts Gradle structure some time later.
So I'm inlined pgjdbc would publish to a local folder, and Hibernate would consume from there via ADDITIONAL_REPO.

That would ensure Hibernate verifies actual binaries as if they were published to Central.

[davecramer]

What about Spatial types.. do you have those supported in your version of the DB?

we do not support them.

[davecramer]

ADDITIONAL_REPO: point it to the directory of the root of your local maven repo, or wherever the pgjdbc lib was published

I believe that is the way to go.

I believe there's also a Gradle "native" way to directly consume the artifact from one gradle project into another.. but maybe I'm overcomplicating. @ sebersole WDYT?

Even though Gradle has nice integration, I believe it is better to use for debugging purposes. However, if we setup tight integration right now, it might become painful if Hibernate adjusts Gradle structure some time later. So I'm inlined pgjdbc would publish to a local folder, and Hibernate would consume from there via ADDITIONAL_REPO.

That would ensure Hibernate verifies actual binaries as if they were published to Central.

We have the ability to publish to local so you can just point it to the local repo.

[Sanne]

@vlsi sounds good 👍

@davecramer ok - make sure to skip the :hibernate-spatial subproject then. There's perhaps a couple more sub-projects you might want to skip to save a bit of time, again it's a matter of diminishing returns.

[davecramer]

does this not require similar change

hibernate-orm/databases/pgsql/matrix.gradle

Line 7 in beb7a37

jdbcDependency 'org.postgresql:postgresql:42.2.19'

[beikov]

Thanks @davecramer for the new release!