maporacle - using dynamically-allocated buffers for constructing SQL queries

[pmauduit]

Since the beginning of this driver, we seem to use stack-allocated memory slots to build the SQL queries. One of our customer regularly asks us to build oracle-activated versions of Mapserver, and depending on his usage, asks us to increase the buffers sizes used in the different arrays.

At a given point, e.g. using queries built on top of an SLD parameter, the generated SQL queries were in the need of more and more memory (last one fit in a 294KB text file), so increasing the array size were not an option, and we tried the following work as to rewrite it with dynamically allocated buffers.

I tried to stay close to the Mapserver's code and use functions provided in the msstring.cpp file (msStringConcatenate and so on) ; after some rapid tests against our customer's infrastructure, I haven't detected memleaks at runtime using valgrind, and the current code does not seem to crash.

During the test phase, we also discover 5 allocated pointers which were not freed before returning (see specific commit).

I added a msautotests/msoracle directory but I haven't actually used it. Oracle provides a virtualbox VM with some of these advertised tools, including an Oracle DB with Oracle Spatial, and I was only able to connect onto the oracle spatial installed in the VM - then getting an error "table does not exist", and I haven't been further, as in the meantime I got access to my customer's one.

Note: it was previously written against the 7.6 branch, I cherry-picked the different commits to realign with main, recompile / retest to make sure it was still functionning as expected, but I might have missed something.

[jmckenna]

Thanks @pmauduit these look like great updates for the Oracle driver. And thanks for including an Oracle test in msautotest, let's see how it runs here....

[pmauduit]

Thanks @pmauduit these look like great updates for the Oracle driver. And thanks for including an Oracle test in msautotest, let's see how it runs here....

That is the complicated thing here: having an Oracle DB with Oracle Spatial is not an easy task ; I tried using the following VM:
https://www.oracle.com/database/technologies/bigdatalite-v411.html

but it's pretty huge to download, and I was not able to load geographical datas into it, before having access to my customer's one, so I left it in this current state. I kept it only for documentation purposes (we need this TNS_ADMIN env variable pointing to a directory containing this tnsnames.ora file to be able to connect onto an Oracle db).

[rouault]

QGIS has some CI testing for Oracle ( https://github.com/qgis/QGIS/blob/master/.docker/docker-compose-testing-oracle.yml ) . I guess part of it could be reused. A first step would be to have a CI build against the OCI library, which we don't have currently, before performing the tests themselves. Not making that a requirement for this PR though.

[pmauduit]

QGIS has some CI testing for Oracle ( https://github.com/qgis/QGIS/blob/master/.docker/docker-compose-testing-oracle.yml ) . I guess part of it could be reused.

I had a look, the docker image is quite huge (3.5GB) but still far less than the VM I mentioned previously. I am also a bit concerned about the licensing issues, but might it be fair use ? And if Qgis is already making use of it, maybe it is also ok to reuse it here as well.

[rouault]

Could you address the following compiler warnings ?

/home/even/mapserver/mapserver/maporaclespatial.c: In function ‘osGeodeticData’:
/home/even/mapserver/mapserver/maporaclespatial.c:720:48: warning: unused parameter ‘version’ [-Wunused-parameter]
  720 | static char * osGeodeticData(int function, int version, char *query_str, char *geom_column_name, char *index_column_name, char *srid, rectObj rect)
      |                                            ~~~~^~~~~~~
/home/even/mapserver/mapserver/maporaclespatial.c:720:129: warning: unused parameter ‘srid’ [-Wunused-parameter]
  720 | static char * osGeodeticData(int function, int version, char *query_str, char *geom_column_name, char *index_column_name, char *srid, rectObj rect)
      |                                                                                                                           ~~~~~~^~~~
/home/even/mapserver/mapserver/maporaclespatial.c:720:143: warning: unused parameter ‘rect’ [-Wunused-parameter]
  720 | static char * osGeodeticData(int function, int version, char *query_str, char *geom_column_name, char *index_column_name, char *srid, rectObj rect)
      |                                                                                                                                       ~~~~~~~~^~~~
/home/even/mapserver/mapserver/maporaclespatial.c: In function ‘osNoGeodeticData’:
/home/even/mapserver/mapserver/maporaclespatial.c:777:131: warning: unused parameter ‘srid’ [-Wunused-parameter]
  777 | static char * osNoGeodeticData(int function, int version, char *query_str, char *geom_column_name, char *index_column_name, char *srid, rectObj rect)
      |                                                                                                                             ~~~~~~^~~~
/home/even/mapserver/mapserver/maporaclespatial.c:777:145: warning: unused parameter ‘rect’ [-Wunused-parameter]
  777 | static char * osNoGeodeticData(int function, int version, char *query_str, char *geom_column_name, char *index_column_name, char *srid, rectObj rect)
      |                                                                                                                                         ~~~~~~~~^~~~
/home/even/mapserver/mapserver/maporaclespatial.c: In function ‘osFilteritem’:
/home/even/mapserver/mapserver/maporaclespatial.c:1049:29: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 1049 |      char * native_filter = msLayerGetProcessingKey(layer, "NATIVE_FILTER");
      |                             ^~~~~~~~~~~~~~~~~~~~~~~

[pmauduit]

Ok done ; for the unused parameters, it was already been done in the main branch, but I reintroduced them when targeting main for this PR.

[jmckenna]

@pmauduit to confirm, you solved the Oracle msautotest issue, correct?

[pmauduit]

@pmauduit to confirm, you solved the Oracle msautotest issue, correct?

It depends of what you mean with the msautotest issues: I reused the docker image from Oslandia, and I was able to load some sample datas taking inspiration of what they did on QGis. I documented the process here:
https://github.com/MapServer/MapServer/blob/main/msautotest/msoracle/README.md

This allows to have an Oracle Spatial where I can launch the added msautotest against. The expected/oracle.png is compliant with the mapfile oracle.map provided.

But if you have a CI in mind, there is still some work to be done, basically:

1. compile Mapserver against the oracle client libraries (libOCI), which are not easily obtainable (need an Oracle account)
2. launch the docker-composition in the msoracle sub-directory: https://github.com/MapServer/MapServer/blob/main/msautotest/msoracle/docker-compose.yaml
3. python run_tests.py