pgjdbc-ng icon indicating copy to clipboard operation
pgjdbc-ng copied to clipboard

Issue #98: Added support for postgis datatypes (optional)

Open frode-carlsen opened this issue 9 years ago • 9 comments

Support for postgis geometry datatypes, and box2d/box3d (see #98).

A few notes on implementation:

  1. This uses the suggested alternative serviceloader document. It means the Procs class has to handle an optional service loader (which only will be loaded if org.postgis.Geometry is available on classpath). Perhaps a cleaner approach would be to delegate to the ProcProviders themselves?
  2. Postgis-jdbc latest version isn't available in a well-known maven repository (latest is 2.1.7, only available up until 1.3.3). I think it's a good idea to compile against a recent version, so I have therefore added it to a lib catalogue to ensure it's compiled against one.
  3. Postgis-jdbc has a couple of hard dependencies on postgres jdbc, in particular for org.postgresql.util.PGobject and PGtokenizer. Both are simple,standalone objects, so I have copied them into this project to avoid requiring the other driver on the classpath as well.

frode-carlsen avatar Jun 11 '15 15:06 frode-carlsen

You need a proper rebase too

jesperpedersen avatar Jun 11 '15 16:06 jesperpedersen

Start by updating the PR w/ the comments - removing org.postgres, adding the official dirver as a dependency, ... In order to give people the status of the PR.

Then work with the PostGis community on attacking the issues about that project

jesperpedersen avatar Jun 11 '15 19:06 jesperpedersen

Updated license of files. Also reverted to compile against 1.3.3 since the classes are compatible, so pushing the issue of version to the user.

Regarding the issue with the official driver, I've sent the question to the postgis-devel list: http://lists.osgeo.org/pipermail/postgis-devel/2015-June/025003.html However, I do not expect much to come from it. See comment https://github.com/impossibl/pgjdbc-ng/pull/178#discussion_r32267889 above for details. If not, then I think it's wrong to include this support directly into pgjdbc-ng, but it be better left as a separate compatiblity project (arguing against my own PR here:-).

frode-carlsen avatar Jun 12 '15 07:06 frode-carlsen

@frode-carlsen @jesperpedersen Wait, I don't see any license restriction that would prevent pgjdbc-ng from including PGobject and PGtokenizer in their existing package (org.postgresql.util) in the pgjdbc-ng source.

If we wanted to be nice, and it is just courtesy to users, we could add the following check at the top of the included classes to prevent running in an environment where both pgjdbc-ng and the official driver are present in the classpath, and the pgjdbc-ng versions of the classes are chosen by the classloader:

public class PGobject implements Serializable, Cloneable {
   static {
      try {
         PGobject.class.getClassLoader().loadClass("org.postgresql.jdbc2. AbstractJdbc2Connection");
         throw new RuntimeException("Conflict between pgjdbc-ng and PostgreSQL JDBC classes");
      }
      catch (ClassNotFoundException e) {
         // Good, there are no other PostgreSQL JDBC driver classes present
      }
   }

If both jars are present, and the official driver versions of those classes are loaded, no-harm no-foul.

EDIT: Additionally, I wouldn't be overly concerned about "drift" from the PostgreSQL JDBC versions of those classes. Any change that would break us would almost certainly break Postgis, and in any case could be dealt with quickly if it occurred.

brettwooldridge avatar Jun 12 '15 10:06 brettwooldridge

@brettwooldridge @jesperpedersen sounds good to me, but it's your call whether you want to have them included or not.

frode-carlsen avatar Jun 12 '15 10:06 frode-carlsen

That isn't the point.

PostGis currently requires the official driver as a dependency, which is their choice. It may or may not get resolved such that all PostgreSQL JDBC drivers will work with PostGis without making any changes to them.

The pgjdbc-ng PostGis work is an extension to the "main" driver, and extensions may have dependencies on other libraries.

Just because we have to option to "include" these dependencies inside our driver due to their minimal "size" and their license doesn't mean that we can do that over time for all extensions.

The core must be kept clean, with the exception of Netty.

The current PR will work as long as the official driver is bundled in the same class loader as the postgis.jar which is quite valid.

Part of this work is come to an agreement how our extension architecture should work, and how their dependencies fits in that architecture.

Step 1 is to get something working, and then look at how the extension is bundled, and distributed. F.ex. we could decide that each extension should be its own top-level project linked against the core release.

@frode-carlsen I don't think you should accept to lower the version bundled, but it may have to do for now. Maybe talk to Sonatype if PostGis don't want to upload to Maven Central. You can always deploy with the deploy-file plugin directly.

jesperpedersen avatar Jun 12 '15 11:06 jesperpedersen

Looks like there's now some movement on the postgis side, which might facilitate making a few changes there going forward. In particular the postgis code is getting split out, and moved to github: https://github.com/postgis/postgis-java/issues/1

frode-carlsen avatar Jun 13 '15 12:06 frode-carlsen

Excellent, we will leave this PR open until there is a resolution on their side. Just keep updating this PR

jesperpedersen avatar Jun 15 '15 12:06 jesperpedersen

Regarding extensions to pgjdbc: I've done a prototype for integrating JTS (JTS Topology Suite from http://tsusiatsoftware.net/jts/jts-features.html, not the older one at http://www.vividsolutions.com/jts/JTSHome.htm). This is core to many java GIS tools (including Geotools, Geoserver, Spatial4j, Lucene/Solr/Elasticsearch gis plugins, and so on). For many performance sensitive applications it will make more sense to convert straight to/from JTS than going via the postgis-jdbc datatypes.

The code is nearly the same, but I think it better belongs either as a top-level project or a module (maven multi-module?).

frode-carlsen avatar Jun 15 '15 17:06 frode-carlsen