Mapserver 6: Oracle and queryByAttributes doesn't run #4417

Closed
benediktrothe opened this Issue Aug 6, 2012 · 21 comments

Projects

None yet

6 participants

@benediktrothe

We are using Mapserver 6.0.3
We think there is a serious problem in the TRY-Method of maporaclespatial.c

SHORT DIAGNOSIS:
I think there is a serious problem in the TRY-Method of maporaclespatial.c.

After the first error in the msOracleSpatialHandler this method will always return 0, which indicates an error.

Therefore mapserver won't work with this msOracleSpatialHandler after the first OCI-error occured with it.

COMPLEX DIAGNOSIS:
A testcase where this becomes a problem is

  1. Mapfile with 2 layers from the same Oracle-Spatial-Database
  2. Java-Binding with
    mapObj map = new mapObj("small.map");
    layerObj gemLayer = map.getLayerByName("Gemeinden");
    gemLayer.queryByAttributes(map, null, "Gemeinde_id=2", mapscript.MS_SINGLE);
    imageObj img = map.drawQuery();

The layer Gemeinde must be the second layer.

This will fail with the message:
[Fri Aug 03 10:35:58 2012].64000 msOracleSpatialLayerGetShape():
OracleSpatial error. Shape type is null... this probably means a record number was requested that could not have beeen in a result set (as returned by NextShape).

This message occurs while drawing the query-layer (of "Gemeinden").

This message indicates, that there is an problem on converting a Spatial-Geometry to a Mapserver-Geometry in "osGetOrdinates".
Inside "osGetOrdinates" there is an OCI-call surrounded with TRY and this TRY returns 0/error.

In reality the OCI-error already occured while drawing another layer, which used the same msOracleSpatialHandler.
And every layer will cause at least one OCI-Error in msOracleSpatialLayerNextShape when reaching the end of the Cursor!

THERAPY:
Option 1:
In TRY remove the "if (hand->last_oci_status == MS_FAILURE) ..." statement.
Since sequences of OCI-Calls are usually made with TRY() && TRY() ...
this statement seems superflous.

Option 2:
Reset hand->last_oci_status to MS_SUCCESS often.
Reset it in msOracleSpatialLayerNextShape (after moving the cursor)
Rest at the beginning of msOracleSpatialLayerGetShape

I implemented Option 2. After this the test passed.

@sdlime
Member
sdlime commented Aug 6, 2012

Can you post a patch for the options? Steve

@benediktrothe

Is a diff-result capable for you? This is a diff of "my" maporaclespatial.c against the one of mapserver 6.0.3

1578a1579
>     hand->last_oci_status = MS_SUCCESS;  // Line added
2305a2307
>             hand->last_oci_status = MS_SUCCESS;  // Line added
2391a2394
>         if (hand) hand->last_oci_status = MS_SUCCESS;  // Line added

This patch just assures, that mapserver runs for me.
I would not state, that this patch solves the problem generally.

Example: In line 2291 Oracle answers with an OCI-error in the (legal) case, that the cursor is after the end.

This OCI-error is stored in msOracleSpatialHandler.last_oci_status and will newly be found when TRY(...) is called
the next time with this msOracleSpatialHandler-instance. This "next-time-call" is rather unpredictable because of connection-pooling.

I ask myself:

  • Is this the only code where an Oracle-cursor is beyond it's end? Doesn't seem to be so. Otherwise I wouldn't need the other reset-statements.
  • Why is the error-status of previous Oracle-calls queried in TRY(...) anyway? What is it good for? I think it's good for nothing. But problably I don't understand enough from the code. IMHO line 229 and 230 should be deleted.
  • In the case msOracleSpatialHandler.last_oci_status must be re-queried in TRY(...): I think msOracleSpatialHandler.last_oci_status should be reseted more systematically. How should this be done without adding to much "entropy" to the code?

Sorry for being unspecific. (But it wasn't easy to break it down to this point anyway :-)

Thank's for taking care.
Benedikt

@sdlime
Member
sdlime commented Aug 7, 2012

I wish I was the main Oracle Spatial author. I'm not familiar with the code and can't test on my end. Will try and get Mike Smith in on this one too.

Steve

@tbonfort
Member
tbonfort commented Aug 7, 2012
@dmorissette
Contributor

cc @aboudreault Alan is back only next week, but hopefully between him and Mike they can sort this out.

@benediktrothe

Does still anybody take care?

@unicolet unicolet added a commit to unicolet/mapserver that referenced this issue Aug 21, 2012
@unicolet unicolet fix for #4417 5f9e272
@unicolet
Contributor
@benediktrothe

Sorry Umberto.
I tried this patch in Mapserver 6.0.3.
The problem still occures. Even with the small Java-Testprogram. Tah patch didn't change anything...

@unicolet
Contributor

Please send me the map file, java program and a dataset (oracle exp) so that I can try to reproduce the bug

@benediktrothe

Here ist the mapfile I use with the small java program to reproduce the bug.
I think it's simple. Just two Oracle-layers...

MAP
  NAME "WWIMAP"
  EXTENT 260317 5653650 358191 5743725
  DEBUG ON
  SIZE 200 200
  CONFIG "PROJ_LIB" "../../lib/mapserver/6.0.3_win32/projection"
  CONFIG  "MS_ERRORFILE" "umn.log"

  PROJECTION
   "init=epsg:25832"
  END

  # markiert Selektionen in gelb
  QUERYMAP
    COLOR 255 255 0
    STATUS ON
    STYLE HILITE
  END

  WEB
    # Imagepath fuer WMS relativ zur Mapdatei!
    IMAGEPATH "../../../Tomcat/maps"
  END


  # Gewässer
  LAYER
    NAME "Gewaesser"
    TYPE LINE
    STATUS on
    DEBUG 5
    CONNECTION "wwi/XXXXX@YYYYY"
    CONNECTIONTYPE oraclespatial
    DATA "shape FROM (select WWI.Gewaesser.Name, WWI.Gewaesser.shape, WWI.Gewaesser.Gewaesser_id, WWI.Gewaesser.OFFSET_M FROM WWI.Gewaesser) using unique gewaesser_id srid 25832  VERSION 10g" 
    TEMPLATE "dummy.html"
    TOLERANCE 5
      CLASS
        NAME "Gewässer"
        STYLE
           WIDTH 3
           COLOR 0 122 245
        END
      END
  END

  LAYER
    DEBUG 5  
    NAME "Gemeinden"
    TYPE POLYGON
    STATUS on
    #DEBUG ON
    CONNECTION "wwi/XXXXX@YYYYY"
    CONNECTIONTYPE oraclespatial
    DATA "shape FROM (select WWI.Gemeinde.Name, WWI.Gemeinde.shape, WWI.Gemeinde.Gemeinde_id FROM WWI.Gemeinde) using unique gemeinde_id srid 25832   VERSION 10g" 
    TEMPLATE "dummy.html"
    TOLERANCE 0
    CLASS
      NAME "Gemeinden"
      STYLE
        OUTLINECOLOR 0 0 0
        WIDTH 3
      END # STYLE
    END # CLASS
  END


END

Anh here ist the full testprogram:

package de.hydrotec.ims;

import java.io.File;
import java.io.IOException;

import edu.umn.gis.mapscript.imageObj;
import edu.umn.gis.mapscript.layerObj;
import edu.umn.gis.mapscript.mapObj;
import edu.umn.gis.mapscript.mapscript;

public class DrawMaps { 
  public static void main(String argv []) throws IOException {
    makeMap(argv);
  }

  public static void makeMap(String argv []) throws IOException {
    mapObj map = new mapObj("small.map");

    map.setSize(1000,800);
    layerObj gemLayer = map.getLayerByName("Gemeinden");
    gemLayer.queryByAttributes(map, null, "Gemeinde_id=2", mapscript.MS_SINGLE);
    imageObj img = map.drawQuery();

    String fName =File.createTempFile("pic", ".png").toString();  
    img.save(fName , map);
    System.out.println(fName);
  }

}
@unicolet
Contributor
@benediktrothe

Hi Umberto

Thank you. Now this patch works for me fine. Good work.

@unicolet unicolet reopened this Aug 22, 2012
@benediktrothe

This one is sticky. I ust checked out Mapserver 6.2 and the problem rises again. The map file and the testprogram are still capable to reproduce the bug.
Here is the patch. It can be applied to maporaclespatial.c from Mapserver 6.2

Index: mapserver/maporaclespatial.c
===================================================================
--- mapserver/maporaclespatial.c    (revision 35266)
+++ mapserver/maporaclespatial.c    (working copy)
@@ -2153,15 +2153,14 @@
                 && TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_fetched, (ub4 *)0, (ub4)OCI_ATTR_ROWS_FETCHED, hand->errhp ) )
                 && TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_count, (ub4 *)0, (ub4)OCI_ATTR_ROW_COUNT, hand->errhp ) );

+  
+      if (!success || sthand->rows_fetched == 0 || sthand->row_num >= sthand->rows_count) {
+        hand->last_oci_status=MS_SUCCESS;
+        return MS_DONE;
+      }
       if(layer->debug >= 4 )
         msDebug("msOracleSpatialLayerNextShape on layer %p, Fetched %d more rows (%d total)\n", layer, sthand->rows_fetched, sthand->rows_count);

-      if (sthand->row_num >= sthand->rows_count)
-        return MS_DONE;
-
-      if (!success || sthand->rows_fetched == 0)
-        return MS_DONE;
-
       sthand->row = 0; /* reset buffer row index */
     }

@@ -3156,11 +3155,10 @@
       success = TRY( hand, OCIStmtFetch( sthand->stmthp, hand->errhp, (ub4)ARRAY_SIZE, (ub2)OCI_FETCH_NEXT, (ub4)OCI_DEFAULT ) )
                 && TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_fetched, (ub4 *)0, (ub4)OCI_ATTR_ROW_COUNT, hand->errhp ) );

-      if (!success || sthand->rows_fetched == 0)
-        break;
-
-      if (sthand->row_num >= sthand->rows_fetched)
+      if (!success || sthand->rows_fetched == 0 || sthand->row_num >= sthand->rows_fetched) {
+        hand->last_oci_status=MS_SUCCESS;
         break;
+      }

       sthand->row = 0; /* reset row index */
     }

I'd like to see this in the next Mapserver release.
I don't know how to upload the complete C-file here. Doe anybody need it?

@unicolet
Contributor

Benedikr, sorry about this. We reverted the patch because it was breaking
other code and after reverting I was not able to reproduce the issue
anymore.
I'll work on this next week, perhaps we could arrange an IRC session?

Umberto

On Friday, November 16, 2012, Benedikt Rothe wrote:

This one is sticky. I ust checked out Mapserver 6.2 and the problem rises
again. The map file and the testprogram are still capable to reproduce the
bug.
Here is the patch. It can be applied to maporaclespatial.c from Mapserver
6.2

Index: mapserver/maporaclespatial.c

--- mapserver/maporaclespatial.c (revision 35266)
+++ mapserver/maporaclespatial.c (working copy)
@@ -2153,15 +2153,14 @@
&& TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_fetched, (ub4 *)0, (ub4)OCI_ATTR_ROWS_FETCHED, hand->errhp ) )
&& TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_count, (ub4 *)0, (ub4)OCI_ATTR_ROW_COUNT, hand->errhp ) );

  •  if (!success || sthand->rows_fetched == 0 || sthand->row_num >= sthand->rows_count) {
    
  •    hand->last_oci_status=MS_SUCCESS;
    
  •    return MS_DONE;
    
  •  }
    

    if(layer->debug >= 4 )
    msDebug("msOracleSpatialLayerNextShape on layer %p, Fetched %d more rows (%d total)\n", layer, sthand->rows_fetched, sthand->rows_count);

  •  if (sthand->row_num >= sthand->rows_count)
    

- return MS_DONE;

  •  if (!success || sthand->rows_fetched == 0)
    

- return MS_DONE;

   sthand->row = 0; /* reset buffer row index */
 }

@@ -3156,11 +3155,10 @@
success = TRY( hand, OCIStmtFetch( sthand->stmthp, hand->errhp, (ub4)ARRAY_SIZE, (ub2)OCI_FETCH_NEXT, (ub4)OCI_DEFAULT ) )
&& TRY( hand, OCIAttrGet( (dvoid *)sthand->stmthp, (ub4)OCI_HTYPE_STMT, (dvoid *)&sthand->rows_fetched, (ub4 *)0, (ub4)OCI_ATTR_ROW_COUNT, hand->errhp ) );

  •  if (!success || sthand->rows_fetched == 0)
    

- break;

  •  if (sthand->row_num >= sthand->rows_fetched)
    
  •  if (!success || sthand->rows_fetched == 0 || sthand->row_num >= sthand->rows_fetched) {
    
  •    hand->last_oci_status=MS_SUCCESS;
     break;
    
  •  }
    

    sthand->row = 0; /* reset row index */
    }

I'd like to see this in the next Mapserver release.
I don't know how to upload the complete C-file here. Doe anybody need it?


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4417#issuecomment-10462293.

@benediktrothe

IRC: We could. But to be honorable I'm not used to it. I prefer slower communication to have more time for mediation:-) If you think it's helpful, make a suggestion...

I don't remember the original patch. But I hardly believe, that the suggested patch has any bad side-effects. The difference to the original version is just in resetting "hand->last_oci_status". It's quite unlikely, that other code needs this flag in this situation.

Do you know which code was breaking?

The patch is important: Without it one cannot use Oracle-Layers with selections.

Benedikt

@benediktrothe benediktrothe reopened this Nov 18, 2012
@tbonfort
Member
@benediktrothe

I had a look on #4508

  • I didn't understand the patch unicolet@bc4ccf0 in detail. But after reading #4508 it sounds plausible to me, that this patch could "cause the loop to be finished too early.", because it prevents Mapserver from reading the next row.
  • The now proposed patch (#4417 (comment)) has a different approach: The loop isn't changed. Only the handling of the error-flag (hand->last_oci_status) is changed.

I'm quite confident :-)

What to do? There is a new mapserver-testsuite. What about new testcases?

  • Are all features fetched from DB?
  • Does selection work?
  • Is the extent properly calculated?

Unfortunately testsuites are a bit awkward, when they need a database...

@unicolet
Contributor

Sorry for the delay

I have now opened a pull request with your patch, could you please give it
a final round of testing?
I have tested the patch on a dataset that I loaded on Oracle XE just for
this bug and it works for me, but I'm not a big Oracle Spatial user, so I
might have left s'thing out.

#4532

Regards,
Umberto

On Tue, Nov 20, 2012 at 10:51 AM, Benedikt Rothe
notifications@github.comwrote:

I had a look on #4508 #4508

  • I didn't understand the patch unicolet@bc4ccf0unicolet@bc4ccf0 detail. But after reading
    #4508 #4508 it sounds
    plausible to me, that this patch could "cause the loop to be finished too
    early.", because it prevents Mapserver from reading the next row.
  • The now proposed patch (#4417#4417 (comment))
    has a different approach: The loop isn't changed. Only the handling of the
    error-flag (hand->last_oci_status) is changed.

I'm quite confident :-)

What to do? There is a new mapserver-testsuite. What about new testcases?

  • Are all features fetched from DB?
  • Does selection work?
  • Is the extent properly calculated?

Unfortunately testsuites are a bit awkward, when they need a database...


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4417#issuecomment-10548265.

@benediktrothe

Hi Umberto

I think you asked me to reconfirm this patch, didn'tyou?

I do: This patch is what I suggested and it runs fine for me :-)

Thank you
Benedikt

@unicolet
Contributor

yes I did and thanks for taking the time to write a patch. I will merge it
asap.

On Wed, Dec 12, 2012 at 4:27 PM, Benedikt Rothe notifications@github.comwrote:

Hi Umberto

I think you asked me to reconfirm this patch, didn'tyou?

I do: This patch is what I suggested and it runs fine for me :-)

Thank you
Benedikt


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4417#issuecomment-11293227.

@mkofahl mkofahl pushed a commit to faegi/mapserver that referenced this issue Apr 9, 2013
@unicolet unicolet fix for #4417 ea84c64
@mkofahl mkofahl pushed a commit to faegi/mapserver that referenced this issue Apr 9, 2013
@unicolet @tbonfort unicolet + tbonfort fix for #4417: Oracle and queryByAttributes doesn't run e37778b
@mkofahl mkofahl pushed a commit to faegi/mapserver that referenced this issue Apr 9, 2013
@unicolet unicolet backported pull request #4532: fix for oracle paging bug #4417 546223d
@mapserver-bot

This is an automated comment

This issue has been closed due to lack of activity. This doesn't mean the issue is invalid, it simply got no attention within the last year. Please reopen with missing/relevant information if still valid.

Typically, issues fall in this state for one of the following reasons:

  • Hard, impossible or not enough information to reproduce
  • Missing test case
  • Lack of a champion with interest and/or funding to address the issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment