SFCGAL tests failing

[strk]

Many SFCGAL tests started failing recently, I suspect due to adding support for something.

Here's an example:

-----------------------------------------------------------------------------
 sfcgal/tickets .. failed (diff expected obtained: /tmp/pgis_reg/test_117_diff)
-----------------------------------------------------------------------------
--- sfcgal/tickets_expected     2015-08-07 18:03:33.327807303 +0200
+++ /tmp/pgis_reg/test_117_out  2015-08-16 12:03:14.266430015 +0200
@@ -1,3 +1,4 @@
+WARNING:  'postgis.backend' is currently set to 'sfcgal' and cannot be changed until you reconnect
 #2|POLYGON((1 1,1 2,2 2,3 2,3 1,2 1,1 1))
 #11|0
 WARNING:  ST_Locate_Between_Measures and ST_Locate_Along_Measure were deprecated in 2.2.0. Please use ST_LocateAlong and ST_LocateBetween
@@ -15,7 +16,7 @@
 #73|GEOMETRYCOLLECTION(CIRCULARSTRING(1 1,2 3,4 5,6 7,5 6))
 #80|MULTILINESTRING((0 0,1 1))
 #83|MULTICURVE(CIRCULARSTRING(220268 150415,220227 150505,220227 150406))
-ERROR:  LWGEOM2SFCGAL: Unknown geometry type !
+#85|0
 #112|GEOMETRYCOLLECTION(POINT(-10 50))
 WARNING:  ST_Locate_Between_Measures and ST_Locate_Along_Measure were deprecated in 2.2.0. Please use ST_LocateAlong and ST_LocateBetween
 ERROR:  Geometry argument does not have an 'M' ordinate
@@ -129,7 +130,7 @@
 #723|POINT(-11.11111 55)
 #804|<gml:Point srsName="urn:ogc:def:crs:EPSG::4326"><gml:pos srsDimension="2">0 0</gml:pos></gml:Point>
 #845|t
-#834|GEOMETRYCOLLECTION(POINT(0 0),LINESTRING(10 0,10 10))
+#834|GEOMETRYCOLLECTION(POINT(0 0 5),LINESTRING(10 10 5,10 0 5))
 #884|1|f
 #884|2|t
 #938|

Paul, do you build/test with SFCGAL enabled ?

[strk]

Another example:

-----------------------------------------------------------------------------
 sfcgal/wmsservers .. failed (diff expected obtained: /tmp/pgis_reg/test_119_diff)
-----------------------------------------------------------------------------
--- sfcgal/wmsservers_expected  2015-08-07 18:03:35.319818195 +0200
+++ /tmp/pgis_reg/test_119_out  2015-08-16 12:03:15.110438037 +0200
@@ -1,5 +1,6 @@
 Starting up MapServer/Geoserver tests...
 Setting up the data table...
+WARNING:  'postgis.backend' is currently set to 'sfcgal' and cannot be changed until you reconnect at character 39
 Running Geoserver 2.0 NG tests...
 Geoserver1|POLYGON
 Geoserver2|4326
-----------------------------------------------------------------------------

[pramsey]

No, I don't run SFCGAL. I wonder how it is that the library is getting double-loaded… the regress framework doesn't hold connections long enough for that to happen, ISTM. Each SQL call runs in a new psql. And those GUCs really don't exist until the extension gets exercised, try this:

psql postgis
SET postgis.backend = 'foobar';
SHOW postgis.backend;
SELECT postgis_full_version();
SHOW postgis.backend;
SET postgis.backend = 'foobar';

The library has to be there for the GUC set to have any effect. So how is _PG_init() getting called twice? What's going on?

[robe]

yap winnie is failing. Sorry I've been busy with packaging and pgRouting stuff so haven't being paying much attention to 2.2.

​http://winnie.postgis.net:1500/job/PostGIS_EDB_Regress_winnie/78/consoleFull

started happening after: ​http://winnie.postgis.net:1500/job/PostGIS%202.2%20windows/1162/

r13902

[robe]

postgis_reg=# sET postgis.backend = 'geos';
SET
postgis_reg=# show postgis.backend;
 postgis.backend
-----------------
 geos
(1 row)


postgis_reg=# SET postgis.backend = 'sfcgal';
SET
postgis_reg=# SELECT postgis_full_version();
WARNING:  'postgis.backend' is currently set to 'sfcgal' and cannot be changed until you reconnect
CONTEXT:  SQL statement "SELECT postgis_lib_version()"
PL/pgSQL function postgis_full_version() line 19 at SQL statement
NOTICE:  Function postgis_topology_scripts_installed() not found. Is topology support enabled and topology.sql installed?
                                                                                  postgis_full_version
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 POSTGIS="2.2.0dev r13911" GEOS="3.5.0dev-CAPI-1.9.0 r4072" SFCGAL="1.1.0" PROJ="Rel. 4.9.1, 04 March 2015" GDAL="GDAL 2.0.0, released 2015/06/14" LIBXML="2.7.8" LIBJSON="0.12" RASTER
(1 row)

and why on earth did it trigger topology checking. I don't even have postgis topology installed

[robe]

Is there something in our code that tries to maybe switch the backend back to geos?

What I am noticing here, though its postgis_lib_version() triggering the issue, it doesn't happen if I call postgis_lib_version() directly.

So as far as I can reckon, the postgis_lib_version() is maybe trying to flip back to geos (it's default state), and it can't do that when it's running within postgis_full_version(); because it's illegal for a subprocess to do that in within a transaction.

Anyrate, all works fine (aside from that annoying topology message), when I flip back to geos backend.

[robe]

I think our check for guc

const char *guc_installed = GetConfigOption(guc_name, TRUE, FALSE);

is flawed.

The issue AFAICS is when I do:

set postgis.backend = 'sfcgal';

It registers that guc is installed, but my postgis library hasn't actually loaded yet.

So what happens is it doesn't use the one I set and ends up using the default postgis.backend='geos' one (the default when no guc is set).

I have to remark out both these lines:

 elog(WARNING, "'%s' is currently set to '%s' and cannot be changed until you reconnect", guc_name, guc_installed);
 return;

to get it to work, otherwise sfcgal backend does not kick in.

[strk]

is that warning even true, that you cannot _change_ its value ? I think our only issue is about being or not being able to register our own handler for the GUC.

[robe]

the warning is wrong too, I change the backend of sfcgal all the time during one session to compare answers.

[pramsey]

You can change the value, but if the callback is wrong you won't change the handler, so things are even more terrible, in that your GUC will say "sfcgal" but the actual handler will still be delegating to geos.

[strk]

we then need a way to check for an handler being installed, and either override if possible or error out if not (I'd prefer an error over a warning)

[pramsey]

OK, so it's really very complex, but I've read through the backend code a lot now and understand it. It's really odd.

If you connect to a database that has postgis "installed", as we know, it won't load the library right away, it waits until the first function call. And the way the GUCs respond is a little odd:

postgis_reg=# show postgis.backend;
ERROR:  unrecognized configuration parameter "postgis.backend"
postgis_reg=# show foo;
ERROR:  unrecognized configuration parameter "foo"
postgis_reg=# set postgis.backend = 'sfcgal';
SET
postgis_reg=# set foo = 'bar';
ERROR:  unrecognized configuration parameter "foo"
postgis_reg=# show foo;
ERROR:  unrecognized configuration parameter "foo"
postgis_reg=# show postgis.backend;
 postgis.backend 
-----------------
 sfcgal
(1 row)

Nice, right? So on the one hand, neither postgis.backend nor foo are set, *but* it is legal to set postgis.backend and not foo. At this point, even after setting postgis.backend, the library is still not loaded, and _PG_init() has not yet been called. This is where the regression error is coming from, even though during regression we only load one library at a time. Now load the library, and we get the error.

postgis_reg=# set postgis.backend = 'sfcgal';
SET
postgis_reg=# select postgis_version();
WARNING:  'postgis.backend' is currently set to 'sfcgal' and cannot be changed until you reconnect
            postgis_version            
---------------------------------------
 2.2 USE_GEOS=1 USE_PROJ=1 USE_STATS=1
(1 row)

Errors just like in regression. Because our test for "can I install the GUC" is "is there a GUC value set", "GetConfigOption()", and yes, there's a value set. Also, since we fail to install the GUC formally, with "DefineCustomStringVariable()", we don't get the lwgeom_backend_switch() callback in place, so the backend never actually changes from the default "geos" backend, so functional regression tests also fail further down the line.

How to fix this? Not sure… we want to prevent double-install of the GUC, which can happen during upgrade when both the old and new libraries are co-existing. And we want to allow DefineCustomStringVariable() to be called even when there's a dummy value living in the postgis.backend GUC. Probably we need a better way to catch the double-library case.

[robe]

I was thinking maybe it's time to ask the Postgres Gods for some sound advice before we trip over any more untied shoes. We are probably the only project that changes the .so name between releases (the fundamental problem) so are probably the only ones affected by this issue.

[pramsey]

More craziness committed at r13937

[robe]

winnie is content with the fix, so I'm closing this out.

[strk]

I'm also content, except I still think that warning should instead be an error…

[pramsey]

If the warning is an error it makes "ALTER EXTENSION … UPDATE TO" unusable except in the case where it's the very first thing you call after connecting.

[strk]

Otherwise it makes it usable but results in a broken session until you reconnect… I wonder if there's any command we may run to force reconnection, from sql.