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

[frode-carlsen]

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.

[davecramer]

Please retain the copyright

[jesperpedersen]

@frode-carlsen You can't take this package namespace. See XAConnection for an example, and maintain license and copyright as Dave states

[frode-carlsen]

@davecramer I can add copyright same as XAconnection, though I thought it be better to reference the actual license terms for the code, since the short copyright statement doesn't give a clue as to what permissions it's been added under. But maybe that's better left for the LICENSE file

@jesperpedersen Changing the namespace raises a bigger issue (se comment 3 above).
In essence, that would not work for postgis-jdbc, since it has a hard dependency on org.postgresql.util.PGobject.
It can get this from the postgres jdbc driver, but that would be a bit silly - requiring the driver I'm trying to swap out still be there runtime, or having the user implement those objects if they care, which is bad usability for this library.

A better solution would then be to go back to having this patch as a separate project, probably under a different namespace, and instead opt for going straight to JTS as an optional dependency in pgjdbc-ng. Can then start weaning people off postgis-jdbc, since it cannot work without these parts of the postgres driver. JTS is pretty much the default low-level java GIS/topology library which everyone uses (note how hibernate spatial converts postgis-jdbc polygons to JTS polygons) .

Let me know what's your preference

[jesperpedersen]

The best thing is to fix PostGis to include the required classes. Having pgjdbc-ng depend on (directly / indirectly) or include org.postgres classes isn't going to fly.

postgis.jar can always depend on the official driver, if they won't refactor / update their code, but has that path been tried yet ?

[frode-carlsen]

@jesperpedersen what I'm trying to say is that AFAIU postgis can't change that way without breaking with the official driver, and if not, this PR should probably be discarded.

Pgobject is at the root of the inheritance hierarchy of the core classes that define the api for postgis (e.g.all geometry objects). It is the class defined by the postgres driver to allow datatype extensions, so postgres and postgis needs it to be that way to work. If you try and remove the dependency, the driver needs a new extension mechanism, or you break apps that wants to upgrade.
If pgobject is added to the postgis library as a duplicate from the driver, you either get lucky since they should be the on the same classloader, compiled the same, or you set users up for an incompatible class issue runtime.

As for the second option, postgis today does exactly that. But then runtime users of pgjdbc-ng would need the official driver as well. Do you want to carry that dependency through pgjdb-ng going forward ? This dependency would not go away, until they decide to create some breaking changes.

[jesperpedersen]

You need a proper rebase too

[jesperpedersen]

This needs to be optional - see HStore as an example

[frode-carlsen]

done

[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

[frode-carlsen]

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 #178 (comment) 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:-).

[brettwooldridge]

@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.

[frode-carlsen]

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

[jesperpedersen]

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.

[frode-carlsen]

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:
postgis/postgis-java#1

[jesperpedersen]

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

[frode-carlsen]

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?).