Merge lp:~zorba-coders/zorba/zorba-for-sqlite into lp:zorba

[Luis Rodriguez Gonzalez]

Add SQLite module.

Added sqlite module to ExternalModules.conf

[Chris Hillery (ceejatec)]

I've pushed a couple changes to get the module compiling on Linux. However, several of the test cases fail for me with the error "http://www.zorba-xquery.com/modules/sqlite:SQLI9999 library routine called out of sequence".

I found this link regarding that error message:

http://sqlite.org/cvstrac/wiki?p=LibraryRoutineCalledOutOfSequence

However, I think the error may be in the test cases. You are using a number of "let" statements to perform sequential tasks such as opening the database connection and querying it, and as far as I know XQuery doesn't guarantee the order of operations of lets in a single FLWOR. Changing these to sequential blocks using scripting may solve these problems. For instance, test0.xq currently reads

let $path := f:path-to-native(resolve-uri("./"))
let $db := s:connect(concat($path, "small2.db"))
let $isconn := s:is-connected($db)
let $result := s:execute-query($db, "select * from smalltable")
let $old-db := s:disconnect($db)
return ($result, $isconn)

This throws the SQLI9999 error. However, if I change the query to the following:

let $path := f:path-to-native(resolve-uri("./"))
let $db := s:connect(concat($path, "small2.db"))
return {
   variable $isconn := s:is-connected($db);
   variable $result := s:execute-query($db, "select * from smalltable");
   variable $old-db := s:disconnect($db);
   ($result, $isconn)
}

the test passes. (Note that when running this query from the command line, I get "Zorba static warning [zwarn:ZWST0004]: sequential FLWOR expr may not have the semantics you expect", so perhaps my code isn't totally correct either. But the test passes, at least.

review: Needs Fixing

[Chris Hillery (ceejatec)]

Another problem is that you have hard-coded English error messages in a number of places in C++ code, such as "DB ID not recognized", "SQL Statement is not valid", and so on. I'm actually not sure how to handle this from a non-core module; I'll send an email about it to the list.

[Chris Hillery (ceejatec)]

Also, need to put something in the ChangeLog.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/zorba-for-sqlite-2012-12-19T09-53-00.358Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/zorba-for-sqlite into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job zorba-for-sqlite-2012-12-19T09-53-00.358Z is finished.
  The final status was:

  8 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Luis Rodriguez Gonzalez (kuraru)]

I fixed the problems Chris pointed out. Please review.

[Chris Hillery (ceejatec)]

Code review comments:

1. Many of the functions are not marked %an:sequential. I believe all of them should be.

2. s:disconnect() returns xs:anyURI, but the documentation says it returns "true if everything went OK". I suspect it should be xs:boolean.

See also the discussion in email about JDBC module API vs. SQLite module API.

review: Needs Fixing

[Chris Hillery (ceejatec)]

Oh, also a typo: the word "prepated" (instead of "prepared") appears in the XQDoc 6 times.

review: Needs Fixing

[Chris Hillery (ceejatec)]

I believe there's a memory leak. valgrind reports a large block (around 73k) of memory lost at line 847 in sqlite_module.cpp for most of the tests. This is the call to sqlite3_open_v2(). After a little debugging, I see that sqlite3_close() is being called in ConnMap::destroy(). However, according to http://www.sqlite.org/c3ref/close.html , calling sqlite3_close() defers all memory cleanup if there are any outstanding prepared statement objects.

Prepared statements are cleaned up with the call to sqlite3_finalize() in StmtMap::destroy(); however, this function does not get called when executing one of the test queries. Hence, the entire database connection is leaked.

[Chris Hillery (ceejatec)]

There are two memory errors when running the module tests, one in test9, one in test6. I'm attaching the relevant parts of the valgrind report.

[Chris Hillery (ceejatec)]

Start testing: Dec 23 02:50 PST
----------------------------------------------------------
3755/4573 Testing: zorba_sqlite_module/test6.xq
3755/4573 Test: zorba_sqlite_module/test6.xq
Command: "/home/ceej/utils/bin/valgrind" "--tool=memcheck" "--leak-check=full" "--track-fds=yes" "--num-callers=50" "--track-origins=yes" "--keep-unloaded-syms=yes" "/home/ceej/zo/src/build/test/rbkt/testdriver" "--rbkt-src" "/home/ceej/zo/all_modules/sqlite-module/test" "--module-path" "/home/ceej/zo/src/build/URI_PATH/:" "test6.xq"
Directory: /home/ceej/zo/src/build/zorba_modules/zorba_sqlite_module
"zorba_sqlite_module/test6.xq" start time: Dec 23 02:50 PST
Output:
----------------------------------------------------------
==616== Memcheck, a memory error detector
==616== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==616== Using Valgrind-3.8.0.SVN and LibVEX; rerun with -h for copyright info
==616== Command: /home/ceej/zo/src/build/test/rbkt/testdriver --rbkt-src /home/ceej/zo/all_modules/sqlite-module/test --module-path /home/ceej/zo/src/build/URI_PATH/: test6.xq
==616==
test test6
=== Query: ===
import module namespace s = "http://www.zorba-xquery.com/modules/sqlite";

let $db := s:connect("small2.db")

return {
  variable $comm := s:commit($db);
  variable $roll := s:rollback($db);
  variable $dis := s:disconnect($db);

  ( $comm = $roll, $comm = $db )
}
=== end of Query ===
=== Result: ===
true true
=== end of result ===
testdriver: success (non-canonical result # 1 matches)
testdriver: test runtime was 5305441us
testdriver: success
==616== Invalid read of size 4
==616== at 0xBB82554: ??? (in /usr/lib/x86_64-linux-gnu/libsqlite3.so.0.8.6)
==616== by 0xBBA34FE: sqlite3_close (in /usr/lib/x86_64-linux-gnu/libsqlite3.so.0.8.6)
==616== by 0xB933E61: zorba::sqlite::ConnMap::destroy() (sqlite_module.cpp:196)
==616== by 0x60158AC: zorba::dynamic_context::~dynamic_context() (dynamic_context.cpp:171)
==616== by 0x5C0EDF0: zorba::XQueryImpl::close() (xqueryimpl.cpp:1491)
==616== by 0x5C06939: zorba::XQueryImpl::~XQueryImpl() (xqueryimpl.cpp:177)
==616== by 0x5C06897: zorba::XQueryImpl::~XQueryImpl() (xqueryimpl.cpp:176)
==616== by 0x5C032AD: zorba::SmartObject::free() (smart_ptr.cpp:29)
==616== by 0x4231D6: zorba::SmartObject::removeReference() (smart_ptr.h:44)
==616== by 0x42441F: zorba::SmartPtr<zorba::XQuery>::~SmartPtr() (smart_ptr.h:82)
==616== by 0x422BA4: zorba::SmartPtr<zorba::XQuery>::~SmartPtr() (smart_ptr.h:80)
==616== by 0x41FEC9: main (testdriver.cpp:599)
==616== Address 0xb6aca90 is 80 bytes inside a block of size 880 free'd
==616== at 0x4C2A654: free (vg_replace_malloc.c:427)
==616== by 0xBB6868F: sqlite3_free (in /usr/lib/x86_64-linux-gnu/libsqlite3.so.0.8.6)
==616== by 0xBBA381A: sqlite3_close (in /usr/lib/x86_64-linux-gnu/libsqlite3.so.0.8.6)
==616== by 0xB9394AF: zorba::sqlite::DisconnectFunction::evaluate(std::vector<zorba::ItemSequence*, std::allocator<zorba::ItemSequence*> > const&, zorba::StaticContext const*, zorba::DynamicContext const*) const (sqlite_module.cpp:876)
==616== by 0x668FA9F: zorba::ExtFunctionCallIterator::nextImpl(zorba::store::ItemHandle<zorba::stor...

[Luis Rodriguez Gonzalez (kuraru)]

A new SQLite module version is now uploaded. I fixed most of the problems but I'm still testing out things.

[Chris Hillery (ceejatec)]

I've worked with Luis extensively over the past few days, and he has fixed all the issues that have come up. These test cases now run valgrind clean.

review: Approve

[Rodolfo Ochoa (rodolfo-ochoa)]

review: Approve

[Matthias Brantner (matthias-brantner)]

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Attempt to merge into lp:zorba failed due to conflicts:

text conflict in ChangeLog

[lp:~zorba-coders/zorba/zorba-for-sqlite]

11143. By Chris Hillery on 2013-01-10

Merge from trunk.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/zorba-for-sqlite-2013-01-10T00-02-39.557Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job zorba-for-sqlite-2013-01-10T00-02-39.557Z is finished. The final status was:

All tests succeeded!

=== modified file 'ChangeLog'
--- ChangeLog	2013-01-09 23:52:36 +0000
+++ ChangeLog	2013-01-10 00:02:24 +0000
@@ -9,11 +9,13 @@
     resolving URIs.
   * Can now specify jsoniq-strip-top-level-array option to parse-json() to
     strip the top-level array from JSON streams.
+  * New external module client for SQLite Database.
   * New external module client for Oracle NoSQL Database.
   * New info-extraction module for querying concepts and entities in
 	natural language text.
   * Extended cast and castable expression to allow any simple target type
     (as specified by XQuery 3.0)
+>>>>>>> MERGE-SOURCE
 
 Optimizations:
   * Various optimizations in the implementation of the optimizer rules.

=== modified file 'modules/ExternalModules.conf'
--- modules/ExternalModules.conf	2013-01-04 16:21:57 +0000
+++ modules/ExternalModules.conf	2013-01-10 00:02:24 +0000
@@ -50,4 +50,5 @@
 schema-tools    bzr  lp:zorba/schema-tools-module     zorba-2.7
 stack           bzr  lp:zorba/stack-module            zorba-2.7
 queue           bzr  lp:zorba/queue-module            zorba-2.7
+sqlite          bzr  lp:zorba/sqlite-module