LayerClose and LayerConnectionClose are interacting poorly

[hobu]

sent to -dev

I found a problem with the SDE driver where we weren't closing out layer
information.  The layer virtualization has two methods, LayerCloseConnection and
LayerClose.  In SDE, they were both being used, and LayerClose was set to a NoOp
function that was just returning success.  This wasn't allowing the SDE driver
to free up its layer information and decrement the connection pool when a layer
was closed.

I have fixed this by commenting out the definition of the
layer->vtable->LayerCloseConnection method in the
msSDELayerInitializeVirtualTable method, replacing the NoOp method to use our
regular msSDELayerClose method, and removing the NoOp method completely.  

Was there a reason why we took this approach in the SDE driver?  I vaguely
remember possibly even asking for a LayerCloseConnection method, but I'm not
sure.  Do you have any recollection?  I think the intent for layers using
connection pooling is that each driver's Open and Close methods will take care
of that and it wouldn't be needed in the layer virtualization.  Maybe that is
not the case for a plugin layer that might be providing its own connection
pooling?  Is that why there are two methods?

I think we could get by with just having LayerClose and LayerOpen deal with
connection pooling issues entirely and not have to worry about them in the layer
virtualization, but I'm not a 100% sure.  For one thing, I'd like to see the
connection pool bubbled up to MapScript at some point, but I don't know where
that should happen -- in the layer virtualization or through some other mechanism.

[hobu]

committed a fix to the SDE driver that just uses msSDELayerClose like we were
doing before.

[hobu]

This should definitely be backported to 4.8.  Nothing has really changed since
then in the SDE driver anyway.

[sdlime]

Howard: Is this related to the various query problems folks are seeing or 
something else entirely?

Steve

[hobu]

It's likely related, but I haven't been able to confirm that it actually fixes
the problem.  That one initialization (er, stomping really) bug that Johan
Hallgren posted to -dev could have also been a problem (I fixed that in this
same patch by checking if it wasn't already initialized).

We need some folks to test.

[sdlime]

Trying to test. As an aside I'm running into an error with just a simple point 
query. Something like this bombs with SDE:

  mapserv "QUERY_STRING=map=test.map&mode=query&mapxy=350000+5292000"

msSDELayerWhichShapes(): SDE error. SE_shape_generate_rectangle(): The number 
of points is less than required for feature. (-148)

Looks like the the query extent is the point so minx=maxx and miny=maxy. Not 
sure why that is happening...

Steve

[sdlime]

Ok, I got around the error by setting a small tolerance value (that's still a 
problem though, alas another bug). Now I'm getting:

msSDELayerGetItems(): Memory allocation error. Error allocating layer items 
array.

Steve

[hobu]

The layer->items array may be invalid... I'm blocking with an if (!layer->items)
in a couple of places.  Drop those and see how things change. 

[sdlime]

There was another line missing in mapsde.c that was in 4.6. It's like all the 
lines with malloc or calloc were deleted. Anyway around line 1429 there is code 
like:

  layer->numitems = n+1;

  if(!layer->items) {
    ...

The code to initialize layer->items was not there:

  It should be:

  layer->numitems = n+1;

  layer->items = (char **)malloc...
  if(!layer->items) {

I've committed that change to CVS. With it I can get a query to finally work.

Steve

[hobu]

something is still not right.  It appears to be fully opening and closing a
connection to the SDE layer for each layer drawn.  This is bad.

[hobu]

nevermind.  It's doing the right thing.  I had forgot to set PROCESSING
"CLOSE_CONNECTION=DEFER" on my layers (I'm using FastCGI).

[hobu]

this is fixed now for 4.8.2 and 4.9