Make transformation and projection cache API available.

[bnordgren]

Attached patch divides the projection cache API into internal and external pieces. The external piece uses an opaque type and wraps the internal type, performing the appropriate casts. The internal piece has been explicitly made static (it could not be used outside the file in which it was defined due to the function signatures containing structs also defined in that *.c file.)

[bnordgren]

Blocked by #1133. Much of the discussion on #1058 applies to this ticket instead of #1058.

[bnordgren]

Updated patch adopts strategy three on #1133 (create a new static library with common code which is Postgresql-aware.) This library is called libcommon. It lives beside postgis, raster, and liblwgeom. Patch is a delta from r7683.

All cunit tests pass.

All regression tests pass, except for "loader/Latin1" with some complaint about something being too long. I assume this is unrelated to anything I did, as it also fails in my other build tree from this svn revision.

The patchfile itself looks funky to me, as it appears to be applying diffs to files after they have been moved. This may be a result of svn mving files to the libcommon directory, altering them, and then doing an svn diff. In any case, to preserve the commit history, you probably want to svn mv the relevant .h and .c files from postgis/ to libcommon/ before applying the patch.

[bnordgren]

Added another version of the patch (attachment:lwgeom_transform_export.patch​) generated via svn export and diff. First patch was made via svn diff, which exhibits strange behavior when describing a source tree with moved files. You may use either, but moving the file with the "patch" will cause the revision history to disappear.

[bnordgren]

Name suggestions for the new library from the dev list:

• libcommon (lame)
• libpg
• libpgdb
• (lib)pgcommon
• (lib)postgres_common

The postgresql client library is "libpq".

I'm a likin' libpgcommon. Any +1's?

Also, any comments on whether it should be a sibling of the postgis/ directory or should it be moved inside postgres/?

[bnordgren]

Updated attachment:lwgeom_transform_export.patch​ to r7720. This svn revision already incorporates #1050. Left the name of the created library as libcommon pending any decisions on the dev list.

Someone with power pls delete attachment:lwgeom_transform.patch​. It's broken.

[strk]

+1 for libpgcommon

libpgcommon, postgis and raster could very well be under a common "pg-something" directory.

[chodgson]

+1 for libpgcommon

Hopefully that should be the last decision we need to get this patch applied?

[mcayland]

-1 for patch application.

Having reviewed the patch, it does not seem to be up to date with the latest discussions on the mailing list. The approach should be to put the transformation code in liblwgeom and link that against the raster .so, not separate it out into another entity. Unfortunately this will mean some refactoring of the projection API as dicussed onlist, but this patch is currently going in the opposite way we need to be heading.

[bnordgren]

I would very much like to consider the internal structure of the projection cache separately from it's location. Looking at this, it's just permeated with MemoryContext references. Not just the cache itself, but every single data structure has a MemoryContext (even the hash table entries). I have no idea what most of it is for, and the very idea of me messing with it is virtually guaranteed to wreck everything.

I could see my way clear to putting these two functions in liblwgeom, however:

int lwgeom_transform(LWGEOM *geom, projPJ inpj, projPJ outpj) ;
int transform_point(POINT4D *pt, projPJ srcpj, projPJ dstpj) ;

That way, you can transform to your heart's content provided you have a projPJ object. liblwgeom will gain a dependency on the proj4 library, tho, which is probably why I didn't do it in the first place.

The projection cache is only used to avoid unnecessary SELECTs against the spatial_ref_sys table, which is a fairly strong argument to keep it out of liblwgeom until an alternate use is found.

[bnordgren]

The latest update renames the common library to "libpgcommon" and places lwgeom_transform and point4d_transform in liblwgeom. The caching bits landed in "libpgcommon" due to their heavy reliance on MemoryContexts.

Note that attachment:lwgeom_transform_export.patch​ is current, and the other one (produced by svn diff) is outdated.

[strk]

I think it's fine for liblwgeom to depend on proj4. Please add copyright/licence informations in new files (like liblwgeom/lwgeom_transform.c) and rebase against r7733.

If you like, you may setup a git-svn repository for easier patch management, see ​http://strk.keybit.net/blog/2011/04/27/postgis-geos-mapserver-with-git/

[bnordgren]

Replying to strk:

Please add copyright/licence informations in new files (like liblwgeom/lwgeom_transform.c) and rebase against r7733.

If you like, you may setup a git-svn repository for easier patch management, see ​http://strk.keybit.net/blog/2011/04/27/postgis-geos-mapserver-with-git/

Oh, that looks awesome! Once this is in, I think I'll transition to this scheme for #1058. Pulling these patches forward has cost me 3 hours each time, mostly due to the fact that one depends on another.

Due to minor inconveniences like federal law, the "license" on anything primarily written by me is public domain. However, as I recall, most of the new files in this patch are just copy/pasted from other pre-existing files, so I can just copy/paste the copyright notice from the appropriate source file too. If there's anything that ends up "public domain", I'll call your attention to it when I update. From what I gather, there's no issues with you slapping a license on a public domain work. This may be more of an issue with #1058.

[strk]

I confirm there's no issue with relicensing public domain code. Will be waiting for an updated patch to apply/test/commit.

[bnordgren]

I fixed liblwgeom/lwgeom_transform.c by copying the license from the original file.

Many of the files I moved to libpgcommon didn't have copyright/license info. I didn't add license info because I don't know which license to add (it's all GPL as far I can see, but the copyright holders and dates are all different.) Fixing this would seem to be part of a larger legal review.

If the patch tests clean on my end, it should be uploaded soon.

[bnordgren]

New (and hopefully last) patch is here. Based on r7733.

[strk]

Did it apply cleanly there ?

$ patch -p1 < /home/strk/Desktop/lwgeom_transform_export.patch patching file configure.ac patching file GNUmakefile.in patching file liblwgeom/liblwgeom.h Hunk #1 FAILED at 1. 1 out of 3 hunks FAILED -- saving rejects to file liblwgeom/liblwgeom.h.rej patching file liblwgeom/lwgeom_transform.c patching file liblwgeom/Makefile.in patching file libpgcommon/common.h patching file libpgcommon/cunit/Makefile.in patching file libpgcommon/gserialized.h patching file libpgcommon/lwgeom_pg.c patching file libpgcommon/lwgeom_pg.h patching file libpgcommon/lwgeom_transform.c patching file libpgcommon/lwgeom_transform.h patching file libpgcommon/Makefile.in patching file libpgcommon/pgsql_compat.h patching file postgis/gserialized.h patching file postgis/lwgeom_in_gml.c Hunk #1 FAILED at 1. 1 out of 2 hunks FAILED -- saving rejects to file postgis/lwgeom_in_gml.c.rej patching file postgis/lwgeom_pg.c patching file postgis/lwgeom_pg.h patching file postgis/lwgeom_transform.c Reversed (or previously applied) patch detected! Assume -R? [n] y Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file postgis/lwgeom_transform.c.rej patching file postgis/lwgeom_transform.h Reversed (or previously applied) patch detected! Assume -R? [n] y Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file postgis/lwgeom_transform.h.rej patching file postgis/Makefile.in patching file postgis/pgsql_compat.h patching file raster/rt_pg/Makefile.in Hunk #1 FAILED at 1. 1 out of 2 hunks FAILED -- saving rejects to file raster/rt_pg/Makefile.in.rej

[strk]

Never mind, I've handled to clean it manually. Most issues were with $Id$ tags, which we could even remove (they don't seem to be substituted anyway).

[strk]

You didn't --enable-raster, it seems:

In file included from rt_pg.c:52:
rt_pg.h:36:39: error: ../../postgis/gserialized.h: No such file or directory
rt_pg.c: In function ‘RASTER_asRaster’:
rt_pg.c:5553: warning: implicit declaration of function ‘SERIALIZED_FORM’
rt_pg.c:5553: warning: passing argument 1 of ‘lwgeom_deserialize’ makes pointer from integer without a cast
/usr/src/postgis/postgis/liblwgeom/liblwgeom.h:1102: note: expected ‘uchar *’ but argument is of type ‘int’

[strk]

What's libpgcommon/cunit for ?

[bnordgren]

Ok, I think I see what happened. I never enabled raster without also applying the patch on #1058, which contains the fix. Grrr.

It's a one-liner: just change

#include "../../postgis/gserialized.h" 

to

#include "gserialized.h"

As long as you've run ./configure after applying the patch, it'll pick it up. I'll upload a new patch (with $Id$ hunks removed) shortly. This one just built with --enable-raster.

[bnordgren]

Updated patch against r7733

[bnordgren]

New patch against r7733 works with --enable-raster and has $Id$ hunks removed.

[bnordgren]

Replying to strk:

What's libpgcommon/cunit for ?

It's a stub. I assume people will be writing cunit tests for the things which accumulate in libpgcommon over time, so I gave the tests a home. ;)

[strk]

I've pushed a cleaned up version of the patch to a 'libpgcommon' branch on github. Hopefully that way others can have an easier time testing it and cleaning it further before application into SVN.

Assuming you've a recent enough git, this should do to fetch:

git clone --branch libpgcommon --depth 1 github.com:strk/postgis.git

[strk]

The branch was pushed to SVN as r7734 and further cleaned in subsequent commits. A new bug #1158 shows some problems about finding postgresql headers. I'm closing this one and adding `bnordgren' in Cc of the other.