feat: Define binary transfer for custom types dynamically (#2554)

[sebasbaumh]

• Get existing oids for binary transfer of custom types from QueryExecutor
• Add new oids for binary transfer of custom types to QueryExecutor
• Unit test to make sure oids are correctly set

#2554

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does ./gradlew autostyleCheck checkstyleAll pass ?
3. Have you added your new test classes to an existing test suite in alphabetical order?

[vlsi]

This sounds like a wrong level of abstraction to me. I do not understand what do you want to achieve

[sebasbaumh]

This sounds like a wrong level of abstraction to me. I do not understand what do you want to achieve

This pull request only complements the already existing API.
I first created a ticket for it (#2554) and I would like to clarify my goal here.

Currently it is possible to add custom types for binary transfer to a connection by using the connection properties while creating the connection. This works for some cases, but unfortunately not for all.

Actually to do this you need to know the oid of the type before setting the connection properties.
But usually this oid varies between different databases as it depends on the other types added before it.
You can easily check that using this SQL on different databases: SELECT oid,typname FROM pg_type ORDER BY typname; for custom types added by modules like PostGIS.

The second issue is more severe: If you use an enterprise application server like WildFly, you do not open new connections in your application. The application server maintains a connection pool and injects connections into the different applications. So you are not able to set these properties for your application only, but the administrator needs to do that and he needs to look up the oids per database, which would be a big hassle.

In the existing API there are already functions for setting the oids to be sent or received binary on the QueryExecutor: QueryExecutor.setBinaryReceiveOids and QueryExecutor.setBinarySendOids.
But calling them will replace all the already set oids, especially the PostgreSQL internal ones here.
Therefore I created functions for adding single oids without removing the already set ones: addBinaryReceiveOid and addBinarySendOid.

Additionally I also added functions for getting the currently set oids: getBinaryReceiveOids and getBinarySendOids, which could be used to determine if the oid was already added.

[vlsi]

QueryExecutor is driver-private class, and I do not understand what are you going to achieve

[sebasbaumh]

QueryExecutor is driver-private class, and I do not understand what are you going to achieve

Just to clarify my intentions:
I am working on postgis-java-ng, which provides the geometry types used by PostGIS.
Therefore I already have to work with the PostgreSQL JDBC internals, for example to add the custom data types using PGConnection.addDataType.
My goal is to support binary transfers as well and also in scenarios like application servers.
And if the connection is not set up by the user (but by the application server) he can call registerDataTypes on the DriverWrapper.

If I know it is a PostgreSQL connection, I am able to use these classes and can add the oids for custom types.
I tested this using QueryExecutor.setBinaryReceiveOids, which worked.
But I explicitly do not want to interfere with any oids the user might have set on the connection.
Using a library should not change any other configurations.

So can you please explain to me, what exactly is the issue with complementing the existing function for setting the oids with two functions for adding a single oid and getting the already set oids?
What negative impact would this have on pgjdbc?

These are all internals, but that is exactly were I come from here - the whole postgis-java-ng implementation is providing types based on PGObject in PostgreSQL ;-)

[davecramer]

The problem is that you are elevating QueryExecutor to being a supported API. The fact that it is currently public does not mean we support it to be used in Applications or 3rd party libraries.

[sebasbaumh]

So what could be a solution for that? Push it up to PGConnection like the data type handling?
And is there some tag or something to see supported API? Your javadoc just lists everything publicly visible.

To me it seemed a less intrusive change to just enhance QueryExecutor to not only be able to set oids, but also to get all oids or add a single oid.
Its like providing a getter when there already is a setter - which might be good for testing anyway ;-)
Currently binary support looks a little bit unfinished to me and I would like to improve that.
One could even expect to just set a flag while calling PGConnection.addDataType to enable binary support - without looking up oids themselves (as there is already a type cache in the code, which could help in that.

I am aware that the core API might change and I will need to adjust my library accordingly.

But I also see that it might benefit the user, if geometries are parsed from the byte data and not converted to a string and parsed back as byte data. And this data can be quite large if a lot of vertexes are used in lines.
We can have millions of points, lines and polygons in a table that should be parsed as fast as possible with low resource usage.

[vlsi]

But I also see that it might benefit the user, if geometries are parsed from the byte data and not converted to a string and parsed back as byte data. And this data can be quite large if a lot of vertexes are used in lines.

Would you please suggest an end-to-end test case?
The tests you add here exercise low-level private API only, and I am afraid that API is not really useful for the end-users.

I agree binary support is worth improving, however, exposing "enable/disable" on a connection level looks like exposing an error-prone API. I would rather activate binary transfer whenever a relevant codec is registered.

[davecramer]

And is there some tag or something to see supported API? Your javadoc just lists everything publicly visible.

In general we support the official java.sql.* API
additionally I would include any classes which implement the api so PgConnection, however the deeper you go the less likely we would consider any extensions to the java.sql API to be supported.

The reason for this is that if we rewrite the underlying implementation we really don't want to be committed to supporting anything other than the official java.sql API

Every extension adds a support burden to the project.

[sebasbaumh]

@vlsi and @davecramer I see both your points, thank you for clarifying this.

At first I just wanted to go for implementing PgBinaryObject on the classes I use for reading the custom types.
But I noticed that even after preparing a SELECT statement and executing it more than 10 times, I would not be provided with binary data.
Finally I found that I need to explicitly ask for binary data by providing the oid of the type to be received as binary.
This is where all started ;-)

From a design point of view I totally support your idea of only supporting the plain java.sql API.
For me it would be enough to implement PGBinaryObject and sending/receiving binary data if not explicitly forbidden by the configuration.

So ideally I would go for adding the custom type to the connection and in the background automatically enabling binary transfer for it if it implements PGBinaryObject.
Do you think this is feasible? I saw that there already is a TypeInfoCache, which has information about the oids though it is only used in the background of PgDatabaseMetaData and supplied by the PgConnection.

I would basically register binary types when PgConnection.addDataType gets called and the type is implementing PGBinaryObject.

[davecramer]

I would basically register binary types when PgConnection.addDataType gets called and the type is implementing PGBinaryObject.

This seems more rational to me

[sebasbaumh]

I would basically register binary types when PgConnection.addDataType gets called and the type is implementing PGBinaryObject.

This seems more rational to me

So we should definitely go for that.

I will try to setup an end-to-end test case, I just need some help with that:
If I use the types for PostGIS, we need to have the postgis extension installed in PostgreSQL and we also need a lot of additional classes from postgis-java-ng.
And all the PostGIS types are defined using functions in C in the extension.
I would not want to do that for a simple test case that runs inside pgjdbc.

Instead for testing I would like to have a simple custom datatype in PostgreSQL, which I can write binary data to and read it back.
Something like bytea, but defined as custom type in PostgreSQL.
But how could I define such a simple test data type using CREATE TYPE without using functions in an extension? Can you maybe help me with that?

[davecramer]

Well I think you can create a single type that has binary transfer for the test. That said yes we would have to figure out how to get that installed on the server

[sebasbaumh]

I created a separate commit in the pull requests for these changes:
For types implementing PGBinaryObject I automatically try to enable binary transfer.
If the oid of the type is defined as BINARY_TRANSFER_DISABLE property, I do not enable binary transfer. That allows the user to override the new behavior.
And I added a property to cache this disabled types in the connection (which is null if no types are disabled), because it needs to be checked each time a type is added.
And don't worry, we can rebase/squash the commits later on, when we know how to go on.

Now I need to create an according unit test and this seems to be difficult:
According to the PostgreSQL documentation I create a user defined type like that.
But I don't know how I could define it without having C functions in place for handling it.
Because that would make creating the test much more complex, except if I could reuse some existing functions.
Basically for testing I just need a custom type that gets mapped to my test implementation and in which I can store binary data.

Maybe for a test I could also just use a built-in type like point and overwrite the handler with my test type?
I am not sure if it works if I register a custom type to handle point, that my type gets called and not the built-in one - I have to check that.

[sebasbaumh]

I just squashed the existing commits and rebased them on the current master to be more in line with the current development.
It seems one of the AppVeyor builds fails because of some error not related to my code.

One of of the unrelated tests as an example:

FAILURE   0.1sec, org.postgresql.replication.LogicalReplicationStatusTest > testWriteLocationCanBeLessThanSendLocation
    org.gradle.internal.exceptions.DefaultMultiCauseException: Multiple Failures (2 failures)
    	org.postgresql.util.PSQLException: FATAL: number of requested standby connections exceeds max_wal_senders (currently 3)
    	java.lang.NullPointerException: <no message>
        at org.junit.vintage.engine.execution.TestRun.getStoredResultOrSuccessful(TestRun.java:196)
        at org.junit.vintage.engine.execution.RunListenerAdapter.fireExecutionFinished(RunListenerAdapter.java:226)
        at org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:192)
        at org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:79)
        at org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
        at org.junit.runner.notification.RunNotifier$9.notifyListener(RunNotifier.java:225)
        at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:72)
        at org.junit.runner.notification.RunNotifier.fireTestFinished(RunNotifier.java:222)
        at org.junit.internal.runners.model.EachTestNotifier.fireTestFinished(EachTestNotifier.java:38)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:372)
        Suppressed: org.postgresql.util.PSQLException: FATAL: number of requested standby connections exceeds max_wal_senders (currently 3)
            at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2676)
            at org.postgresql.core.v3.QueryExecutorImpl.readStartupMessages(QueryExecutorImpl.java:2788)
            at org.postgresql.core.v3.QueryExecutorImpl.<init>(QueryExecutorImpl.java:174)
            at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:290)
            at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49)
            at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:253)
            at org.postgresql.Driver.makeConnection(Driver.java:434)
            at org.postgresql.Driver.connect(Driver.java:291)
            at java.sql.DriverManager.getConnection(DriverManager.java:664)
            at java.sql.DriverManager.getConnection(DriverManager.java:208)
            at org.postgresql.test.TestUtil.openDB(TestUtil.java:414)
            at org.postgresql.test.TestUtil.openReplicationConnection(TestUtil.java:354)
            at org.postgresql.replication.LogicalReplicationStatusTest.setUp(LogicalReplicationStatusTest.java:51)
        Suppressed: java.lang.NullPointerException
            at org.postgresql.replication.LogicalReplicationStatusTest.tearDown(LogicalReplicationStatusTest.java:60)
        Cause 1: org.postgresql.util.PSQLException: FATAL: number of requested standby connections exceeds max_wal_senders (currently 3)
            at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2676)
            at org.postgresql.core.v3.QueryExecutorImpl.readStartupMessages(QueryExecutorImpl.java:2788)
            at org.postgresql.core.v3.QueryExecutorImpl.<init>(QueryExecutorImpl.java:174)
            at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:290)
            at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:49)
            at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:253)
            at org.postgresql.Driver.makeConnection(Driver.java:434)
            at org.postgresql.Driver.connect(Driver.java:291)
            at java.sql.DriverManager.getConnection(DriverManager.java:664)
            at java.sql.DriverManager.getConnection(DriverManager.java:208)
            at org.postgresql.test.TestUtil.openDB(TestUtil.java:414)
            at org.postgresql.test.TestUtil.openReplicationConnection(TestUtil.java:354)
            at org.postgresql.replication.LogicalReplicationStatusTest.setUp(LogicalReplicationStatusTest.java:51)
        Cause 2: java.lang.NullPointerException
            at org.postgresql.replication.LogicalReplicationStatusTest.tearDown(LogicalReplicationStatusTest.java:60)

This is the exact AppVeyor build: Environment: pg=9.6.11-1, PlatformToolset=v120
All the other AppVeyor builds seem to succeed.

[sebasbaumh]

Okay that's it... 😄
I have cleaned up all my changes and then squashed everything into a single commit.

Furthermore I have added a new unit tests to not only verify the OIDs set to be transferred binary, but to also test the actual transfer from/to the database.
This works by using the PostgreSQL internal type point and handling it with a custom type.
In this test I put assertions in place to check that my type is used and also that it is transferred in binary mode from and to the database.

[sebasbaumh]

@davecramer @vlsi Do you have any comments on this? Is there something I can do to progress forward/get this merged?

[davecramer]

LGTM, @vlsi ?

[vlsi]

Should this be something like Collections.unmodifiableSet(...)?

[sebasbaumh]

I made it consistent with getBinaryEnabledOids, which also returns a modifiable set of values.
Actually this prevents the need to copy the set before modifying it (in the constructor of PgConnection the disabled OIDs are removed from the enabled OIDs to get the binary OIDs as a result).

[vlsi]

PgConnection is driver-internal API. Client-facing APIs are java.sql.Connection and org.postgresql.PGConnection.

[sebasbaumh]

This is also a driver-internal test. The APIs for checking if a type is sent or received as binary are only available on QueryExecutor. So they can only be tested by either really sending and receiving them (what testCustomBinaryTypes) does or using the QueryExecutor API.
I added testBinaryTransferOids by intention to make sure the internal API works before a database is used in the other test. So it is possible to first test the API works and then to test if the database access works.

[vlsi]

Do you think you could "disable default binary transfer for POINT" via existing connection properties rather than using a private PgConnection + QueryExecutor APIs?

[sebasbaumh]

This might be possible, but as stated above I need to use the internal API anyway.
So it is more flat/simple to just remove this single type without introducing possible side effects by setting additional connection properties.

[vlsi]

Sorry it takes so long to review.
I think the approach with PGBinaryObject makes sense, however, I would like to refrain from using "driver-private APIs" (PgConnection, QueryExecutor)

@sebasbaumh , would you please check if org.postgresql.PGConnection#addDataType(java.lang.String, java.lang.Class<? extends org.postgresql.util.PGobject>) is enough for you?

[sebasbaumh]

Sorry it takes so long to review. I think the approach with PGBinaryObject makes sense, however, I would like to refrain from using "driver-private APIs" (PgConnection, QueryExecutor)

@sebasbaumh , would you please check if org.postgresql.PGConnection#addDataType(java.lang.String, java.lang.Class<? extends org.postgresql.util.PGobject>) is enough for you?

No problem and thank you for reviewing this pull request.
I will update it to incorporate the changes and rebase it on the current master branch.

org.postgresql.PGConnection#addDataType(java.lang.String, java.lang.Class<? extends org.postgresql.util.PGobject>) would work for just adding a custom type. But there are more tests that ensure that the OIDs are correctly handled internally.
I would like to ensure the OIDs are really set instead of just testing the whole connection as a black box. This would even work if some default connection properties etc. change in the future. And it allows to identify what might go wrong on later changes instead of just telling the developer that the connection did no binary transfer.

After all the changes are mostly to internal APIs and do not need to be accessible by the user - but we need to test the internal APIs. So the tests need to be able to access them.

[sebasbaumh]

I just rebased this pull request on the current master as there were merge conflicts after the latest commits to master.

@davecramer & @vlsi I incorporated most of your proposed changes and commented on the others.
Is there anything else I could do to help? It would be great to be able to use the binary transfer mode more easily.
So thank you very much for your feedback and review in advance.

[vlsi]

@sebasbaumh , did you select Allow edits and access to secrets by maintainers?
I've tried to add a review commit to the branch, however, it says I can't do that.

See what I have in https://github.com/vlsi/pgjdbc/tree/pr2556 (e.g. vlsi@8a41067 )

[sebasbaumh]

I have this setting enabled:

Maybe it is because the branch was rebased by me. I did that to make it easier to merge it, but it seems to not work good with active reviews.

So thank you and I will just look on your branch.

[vlsi]

Probably the last time I spelled the command wrong, it yielded "no permissions", so I assumed edits from maintainers was not enabled.

[sebasbaumh]

@vlsi thank you very much for reviewing/adapting the pull request.
To me your changes look good and I would be happy to go on with that.

[vlsi]

There's a failure caused by the new code:

        Suppressed: org.postgresql.util.PSQLException: ERROR: syntax error
            at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2676)
            at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2366)
            at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:356)
            at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:496)
            at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:413)
            at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:190)
            at org.postgresql.jdbc.TypeInfoCache.getPGType(TypeInfoCache.java:489)
            at org.postgresql.jdbc.PgConnection.addDataType(PgConnection.java:758)
            at org.postgresql.jdbc.PgConnection.initObjectTypes(PgConnection.java:772)
            at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:332)
            at org.postgresql.Driver.makeConnection(Driver.java:434)
            at org.postgresql.Driver.connect(Driver.java:291)
            at java.sql.DriverManager.getConnection(DriverManager.java:664)
            at java.sql.DriverManager.getConnection(DriverManager.java:208)
            at org.postgresql.test.TestUtil.openDB(TestUtil.java:388)
            at org.postgresql.test.TestUtil.openReplicationConnection(TestUtil.java:328)
            at org.postgresql.replication.CopyBothResponseTest.setUp(CopyBothResponseTest.java:66)

There's a chicken-and-egg issue:

• the new code in PgConnection.addDataType attempts registering type for binary transfer
• unfortunately, it does so by requesting oid based on the type name via typeCache.getPGType
• getPGType launches a request to the database in case the type is not cached
• DB request fails in case the DB does not support SELECT (e.g. if we talk to replication connection)

A quick workaround is to skip registering binary types when the connection is in "simple queries only", however, we probably should add extra checks to methods like typeCache.getPGType so they don't attempt issuing SELECT on replication connections.

[sebasbaumh]

Good catch. I was wondering why it failed that test.
So PreferQueryMode.SIMPLE is only set if the connection is a replication connection? Or would I need to set additional properties for a normal connection to be able to automatically register binary transferable types?

[vlsi]

preferQueryMode=simple can be set for any connection.
However, with simple mode only binary transfer is not possible anyway, so there's no point in "registering type for binary transfer".

[vlsi]

I just realized we should better implement the standard API java.sql.Connection#setTypeMap(java.util.Map<String,Class<?>> map) rather than add vendor-specific addDataType.
However, PGConnection.addDataType extension was there for ages, so it does not hurt making it a little bit better.

[sebasbaumh]

I just realized we should better implement the standard API java.sql.Connection#setTypeMap(java.util.Map<String,Class<?>> map) rather than add vendor-specific addDataType. However, PGConnection.addDataType extension was there for ages, so it does not hurt making it a little bit better.

Yes, maybe we could deprecate the vendor-specific function later when the standard API gets implemented.
And this would also simplify handling for connections that are proxies to the actual PGConnection (usually this is true for pooled connections).

[vlsi]

I think this is mergeable. Of course, we might adjust oids used for testing, on the other hand, I doubt they will induce issues in the future.

[davecramer]

editorconfig is a gratuitous change but that's fine

[sebasbaumh]

Thanks for merging! 👍

I will try to follow up @vlsi recommendation to implement this also in the standard API java.sql.Connection#setTypeMap(java.util.Map<String,Class<?>> map) in another pull request.
The benefit would then be to be able to use the standard API, which might even be possible in wrapped/proxied connections.
But it might take some time to check, how to do this in compliance with the JDBC specification and the existing type mappings in pgjdbc.