Declared get-connection to return Connection in 0.3.0-alpha2.

Having reviewed the code, the result of get-connection is never closed, only the result of calling .getConnection on a PreparedStatement and that matches how java.jdbc has always worked, so I don't believe this is a problem. Aphyr mentioned on Twitter that he believes the problem was just due to his test code and not inherent in java.jdbc but we are still waiting for confirmation. Lowering priority for now.

I believe I have verified this locally. Under the old API, a connection was only added explicitly at the "top" of a call chain. Under the new API, a connection can be added on-demand in many places which makes the logic much more complex and there are a couple of places where normal usage could indeed cause a connection leak.

I believe this is resolved in the latest snapshot (which will become Alpha 4 at some point), as I've gone thru and changed how get-connection is called and ensured that with-open is used whenever a new connection is actually created.

The code needs a fair bit of clean up now tho'. I'm not happy with the result of adding that logic so I need to spend some time refactoring...

create-table now allows optional :table-spec "table level attributes" at the end of the specs.

I've started the work to reorganize the code to better expose the statement object for this sort of manipulation.

Two options under consideration:

• allow raw PreparedStatement to be passed in by user where appropriate so they have complete control
• allow map of settings that would be used to create the PreparedStatement and set its options

I like the latter better but Alan Malloy made the point that the former may still be needed for total control anyway.

I'm leaning toward adding a function to ease creation of PreparedStatement objects to at least provide a clean API and then I'll consider how best to integrate that with the other functions.

Added prepare-statement* to jdbc.internal which takes a connection, a SQL string and a map of options; returns a PreparedStatement. Will expose this in top-level.

with-query-results now allows the first element in the sql-params vector to be either:

• PreparedStatement object created by the user
• map of options passed to prepare-statement* in order to create the PreparedStatement

Options supported are in the docstring. May add additional post-creation options (beyond fetch-size and max-rows).

Users have verified that being able to specify :fetch-size in with-query-results handles the common use cases for performance.

Applied a variant of Peter's patch and added unit tests.

Empirically, I can tell you the one in do-prepared*, (.prepareStatement (connection), and the one in transaction* (let [con (connection*) ...) are both performance critical for me.

Thanx Allen. The impression I get from clojure-dev is new contrib libraries should have zero reflection warnings so I plan to find and fix all such cases in c.j.j.

All reflection warnings have been removed.

I just committed a potential fix for this. It's available in the Sonatype snapshots repo.

I think the correct behavior would be to call rollback on the transaction and not just not calling commit? The user will expect at the end of the transaction block one of commit or rollback to be called.

I was thinking about how to test this. How do you feel about a mock Connection object, where we can check if commit and/or rollback have been called as part of the test?

The same mechanism could be used to test other features.

I was thinking in something like the attached for testing the issue (and other functionality).
It needs several improvements, but it's enough to get the idea.
What do you think?

FYI, here's the IRC discussion about this, for the record:

[5:11pm] <paraseba>
In clojure.java.jdbc there is a transaction function, which is supposed to rollback if the passed body throws an exception, but it only does that in case of an Exception, not any Throwable. What's the rationale behind this?
[5:11pm] <paraseba>
I'd want my transaction rolledback in case of any errors
[5:12pm] <paraseba>
seancorfield: maybe you can explain me this?
[5:53pm] <amalloy>
paraseba: catching Errors is generally a bad practice. i'm not saying it's wrong all the time, but Errors are often unrecoverable anyway
[5:53pm] <amalloy>
eg, "You ran out of heap space! I can't even allocate memory to throw an exception!"
[5:54pm] <paraseba>
but, even in the worst conditions, shouldn't we try to rollback the transaction? is not better that commiting in this unexpected error condition?
[5:55pm] <paraseba>
we can then rethrow the Throwable, after trying to rollback
[5:55pm] <lucian>
paraseba: i don't think the db will commit if it gives you an error
[5:55pm] <lucian>
and as amalloy, errors like OOM are best solved with exit()
[5:55pm] <paraseba>
lucian: it will ... jdbc is catching Exception and rolling back in that case .... but it commits in a finally
[5:56pm] <paraseba>
so, if you have an Error thrown, it will commit
[5:56pm] <paraseba>
I guess that's more surprising than a rollback
[5:57pm] <paraseba>
the logic is .... (try do-your-thing (catch Exception roll-back) (finally commit))
[5:57pm] <lucian>
paraseba: well, then maybe you don't want to commit in finally?
[5:57pm] <paraseba>
I don't, not if I got an Error
[5:58pm] <amalloy>
lucian: i think he's upset that a library he's using is committing on error
[5:59pm] <paraseba>
amalloy: I can solve it easily by wrapping my operations in a catch Throwable -> rollback -> rethrow, but I think it's not the right behavior for the library
[5:59pm] <paraseba>
I would expect a commit only if the block ended without any kind of exceptions or errors. don't you agree ?
[6:01pm] <amalloy>
paraseba: meh. all kinds of weird stuff can happen if an Error occurs; i wouldn't be entirely certain that an attempt to "recover" makes things worse due to some weird program state caused by the Error. i mean, my completely-unresearched opinion is that catching Throwable would be better, but you can't really rely for your program's correctness on anything that happens after an Error
[6:02pm] <paraseba>
but, we are forcing a commit after an Error
[6:04pm] <paraseba>
the usual logic should be .... (do do-your-thing (commit)) if do-your-thing throws anything ... no commit is done. Puting a commit in a finally enforces the commit, even after Errors
[6:06pm] <amalloy>
yeah, i think i agree
[6:08pm] <paraseba>
amalloy: ok, I'll report an issue, thanks

It's important to realize that there are 2 problems here.

a.- Transaction not rolled back on Throwable
b.- The original Exception is "swallowed" and a new, different one is thrown

I think both are equally serious.

I agree Sebastian. I've raised this issue for input from clojure-dev (looking for stylistic guidelines for contrib libraries) because I think both problems need to be fixed.

My current leaning is toward supporting three paths:

• success: rollback transaction if user code sets rollback true, else commit
• Exception: rollback transaction and rethrow (without wrapping)
• Throwable: allow it to escape, take no action

I'm attaching a patch that fixes the problem without adding any tests.

On success, commit unless rollback has been set (in which case rollback).
On Exception, rollback and rethrow as-is.
On Error (Throwable), allow it to escape.
In all cases, set rollback false and restore auto-commit setting.

The only two changes here should be:

• Exception is no longer wrapped when thrown (may break code that expected the wrapping?)
• No commit attempted on Error (Throwable)

Unfortunately .executeBatch had to be changed to .executeUpdate in order to solve JDBC-10:

https://github.com/clojure/java.jdbc/commit/9240d66c9124454187416861a0e2021952eb4f40

MS SQL Server and PostgreSQL both fail on .executeUpdate in that call position.

Clearly we need as many of these DBs up and running for automated testing as possible before I can attempt to fix this so that I don't accidentally break something else.

I now have some unit tests for MySQL, HSQLDB and Derby. insert-records fails with the exception reported here, only for Derby. I'll complete the suite of tests, try to get PostgreSQL and MS SQL Server tests running as well and then I'll be in a good position to fix things...

The culprit was .addBatch which has now been removed and do-prepared-return-keys now explicitly only has one param-group argument.

See JDBC-3 for some background on this.

The way resultset-seq uses create-struct and struct doesn't seem to leverage any inherent functionality of structmaps except perhaps some vague guarantee of ordering of columns in rows. Replacing the create-struct / struct calls with

(into {} (map vector keys (row-values)))

maintains the order up to 16 columns (PersistentArrayMap) so that seems a reasonable compromise.

I'm not sure that preserving order is something that should be documented or supported?

No longer uses structmap. Attempts to provide column order (up to 16 columns) but don't rely on that... it was never documented as a guarantee!

Actually, do-prepared doesn't seem to do anything at all. (jdbc/do-commands "insert into foo (foo,bar) VALUES (1,2)") works as expected, same command with do-prepared doesn't do anything.

Ensure .addBatch is called even when there are no param-groups.

Oracle JDBC, probably applies universally

Thanx Ghadi.

clojure 1.4.0
clojure.java.jdbc 0.2.1

It was a bug. Thanks for spotting that.

Ubuntu
OpenJDK:
java version "1.6.0_22"
OpenJDK Runtime Environment (IcedTea6 1.10.6) (6b22-1.10.6-0ubuntu1)
OpenJDK 64-Bit Server VM (build 20.0-b11, mixed mode)

OS X Java
java version "1.6.0_29"
Java(TM) SE Runtime Environment (build 1.6.0_29-b11-402-10M3527)
Java HotSpot(TM) 64-Bit Server VM (build 20.4-b02-402, mixed mode)

Relevant clojure libraries
[org.clojure/clojure "1.3.0"]
[korma "0.3.0-beta9"]
[org.clojure/java.jdbc "0.1.3"]
[postgresql/postgresql "9.1-901.jdbc3"]

RETURN_GENERATED_KEYS is required for inserts to return the generated keys. Since this works on Sun's Java 6 and with all intended databases, and is per spec, it is not going away.

Right now OpenJDK / PostgreSQL is not a supported combination, sorry.

Fixed in 0.2.0 release.

How about an option that takes a map like:

{:connection-uri "jdbc:postgresql://ec2-22-11-231-117.compute-1.amazonaws.com:5432/d1kuttup5cbafl6?user=pcgoxvmssqabye&password=PFZXtxaLFhIX-nCA0Vi4UbJ6lH&ssl=true"}

Essentially as a shorthand for the workaround you've come up with?

Try 0.2.3-SNAPSHOT which has support for :connection-uri and let me know if that is a reasonable solution for you?

Yup, 0.2.3-SNAPSHOT's :connection-uri works fine. I've since moved on to using a pooled datasource, but this will hopefully be a more obvious path to newcomers than having to learn about :factory and DriverManager.

Resolved by adding :connection-uri option.

accessing an heroku database outside heroku, "sslfactory=org.postgresql.ssl.NonValidatingFactory" doesn't work. i get "ERROR: syntax error at or near "user" Position: 13 - (class org.postgresql.util.PSQLException". this happens whether adding it to :subname or :connection-uri Strings

another minor issue - why the documentation of "with-connection" (0.2.3) refers the following format for the connection string URI:
"subprotocol://user:password@host:post/subname
An optional prefix of jdbc: is allowed."
but the URI which can actually be parsed successfully is like the one above: jdbc:postgresql://ec2-22-11-231-117.compute-1.amazonaws.com:5432/d1kuttup5cbafl6?user=pcgoxvmssqabye&password=PFZXtxaLFhIX-nCA0Vi4UbJ6lH&ssl=true
"subprotocol://user:password@host:post/subname" (format like the DATABASE environment variables on heroku) will not be parsed correctly. why the format for the URI that is used on heroku is not supported by the parser?

maybe i'm doing something wrong here

thanks in advance

Carlos, the :connection-uri passes the string directly to the driver with no parsing. The exception you're seeing is coming from inside the PostgreSQL driver so you'll have to consult the documentation for the driver.

The three "URI" styles accepted by java.jdbc are:

• :connection-uri - passed directly to the driver with no parsing or other logic in java.jdbc,
• :uri - a pre-parsed Java URI object,
• a string literal - any optional "jdbc:" prefix is ignored, then the string is parsed by logic in java.jdbc, based on the pattern shown (subprotocol://user:password@host:port/subname).

If you're using :connection-uri (which is used on its own), you're dealing with the JDBC driver directly.

If you're using :uri or a bare string literal, you're dealing with java.jdbc's parsing (implemented by Phil Hagelberg - of Heroku).

Hope that clarifies?

Sean, thank you for such comprehensive explanation.

Still, it didn't work with any of the options. I used before a postgres JDBC driver to export to the same database (in an SQL modeller - SQLEditor for the MAC) and it worked (though it would connect some times, but others not). The connection String used was like "jdbc:postgresql://host:port/database?user=xxx&password=yyy&ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory". The driver name was "org.postgresql.Driver" (JDBC4). Anyway, time to give up. I will just use a local database.

Thank you!

Sean, JDBC combinations were working after. i was neglecting an insert operation in a table with a reserved sql keyword "user", so i was getting a "ERROR: syntax error at or near "user" Position: 13", and therefore the connection was already established at the time.

i'm sorry for all the trouble answering the question (^_

thank you

Glad you got to the bottom of it and confirmed that it wasn't a problem in java.jdbc!

If we execute the above (with connection - with-query results) i get the following exception

IllegalArgumentException db-spec com.jolbox.bonecp.ConnectionHandle@53371566 is missing a required parameter clojure.java.jdbc/get-connection (jdbc.clj:192)

Use the :datasource option to define your db-spec:

(def db {:datasource bonecpDataSourceObject})

I looked at the BoneCP docs and I don't see a ConnectionManager class but if you define the db-spec based on the BoneCPDataSource object, java.jdbc will call getConnection on that object to get a working connection for operations.

Amir, have you had a chance to try my suggestion?

Hi Sean,

No, we moved to clojure based pooling in a while. We thought its better to have all stuff in clojure rather than having java+clojure. But i can give it a try today.

Thanks for your suggestion. will keep you posted.

I'll close this out then Amir, thanx.

I believe there is a way to access the the BoneCP connection pool directly and use it with java.jdbc as-is, however it is no longer an issue for the creator of this ticket.

Made do-prepared-return-keys public. Will cut a new build.

Thanx for the patch!

Specifically using the MySQL connector 5.0.x or 5.1.x against a Tungsten Connector proxy

Further update: this looks to be a configuration or version issue with the Tungsten Connector setup as we've been able to repro the problem in one environment but it works as expected in a newer environment.

May still go ahead with this change anyway since I believe it's a good change for robustness, but it looks lower priority right now.

Pushed a fix for this but would like more hands on this in SNAPSHOT form before declaring it resolved.

Tungsten's Connector defeats this fix. Even tho' the fields have an underlying mysqlType value of 16 (BOOLEAN), (.getColumnType) returns -3 (byte[] presumably) so Boolean is not forced as I had hoped.

The underlying behavior is a quirk of the Tungsten Connector configuration which we believe we've successfully corrected. Given the result of (.getColumnType) is -3, I'm not sure anything could be done in code to address this, so I'm going to resolve this as invalid.

Not a code issue.

Can you provide a self-contained test case? Lots of people are using java.jdbc in production without running into this problem, and at World Singles we've had this in production for a long time in a high traffic environment without seeing any problems with timezones. Dates go in and out of the database unchanged, which is exactly as expected - and we have databases running in three different timezones.

I have added a new test and it is failing with my Postgresql.

https://github.com/jestinepaul/java.jdbc/commit/195397b1b2a0245d2439ab9963fe2138450a27f3

Looking at recommended practices out there, such as listed here: http://stackoverflow.com/questions/2532729/daylight-saving-time-and-timezone-best-practices one important thing is: "On Servers, set hardware clocks and OS clocks to UTC. Use NTP services on all servers." The same advice is repeated here: http://www.dbspecialists.com/blog/database-theory/intelligent-date-handling/

Looking further into PostgreSQL (a database I generally don't use), I see this: http://stackoverflow.com/questions/6151084/which-timestamp-type-to-choose-in-a-postgresql-database which seems to contain a lot of PostgreSQL-specific stuff. However, that article makes it clear that you need to set the database timezone to UTC in order to SELECT timestamp columns correctly.

In my opinion, the behavior you're seeing is not a bug (in java.jdbc) but an artifact of your environment being set up incorrectly. I'll leave this ticket open for a little while for more discussion but without a concrete patch that is shown to not affect other use cases, I will close this ticket by the end of August.

As mentioned earlier, the database server timezone is in UTC and the JDBC library runs on a client machine set to local time. If the database has a date value of 2012-7-23, it changes to 2012-7-22 on the client side (if the client is running at timezone greater than UTC) when coerced using to-date-time in clj-time. This is extremely dangerous and is not specific to any database. I noticed it first on Sybase Enterprise Server and I have now also replicated it in the test case with Postgresql.

I have attached a patch which fixes this problem by passing in an optional parameter. The test case is also modified to use clj-time, as it expresses the problem more clearly. Please let me know if you need any more clarification.

p.s. I have already mailed the CA and should reach Durham in a few days.

Since the proposed patch requires the user to pass in a list of columns to treat specially - and reorders columns in results and adds some performance overhead for all users, not just those wanting to adjust column values - I believe users who choose to run JDBC clients in timezones different to the database, against the widely-listed best practice recommendations, should bear the burden of adjusting the columns themselves in their own client code. Since they already know which columns to adjust and they know the difference between UTC and local time, they should be able to make the adjustments easily enough as part of post-processing the resultset-seq.

See JDBC-3 for some more background.

The new API has been added. There is also an optional DSL to make it slightly easier to generate SQL that can be used with both the original and the new API.

I will open tickets for follow-up issues.

db-do-commands added

0.2.3

Thanx Gary. In the latest source there may be two pieces of transaction logic so please check for anywhere else that Exception is mentioned!

This patch changes Exception to Throwable in two places – transaction* and db-transaction*. The tests pass.

Patch applied.

Postgres database on Debian machine

This looks like an old / long-standing bug that no one has run into before.

In the new (soon-to-be 0.3.0) release, (clojure.java.jdbc.sql/insert :table [1 1] [2] []) would throw an exception:

IllegalArgumentException insert called with inconsistent number of columns / values clojure.java.jdbc.sql/insert-multi-row (sql.clj:116)

I will update clojure.java.jdbc/insert-rows to throw a similar exception.

Fixed in 0.3.0-SNAPSHOT by throwing an exception if the value-groups are not all the same length.

Currently relies on the DB to trap column names not matching value groups (which is known not to work on SQLite but does work on other DBs as far as I know).

insert-rows is deprecated in 0.3.0 - use insert! instead going forward (which does more validation).

Postgres on Debian

Further analysis based on a suggestion of Sean leads to:

It seems to be that there is an issue in the naming-strategy in this issue. Either in the way I define it of the way it is implemented
(sql/with-connection db
(sql/with-naming-strategy {:keyword identity :entity (fn [x] (str \" x \"))}
...
))

When I remove the naming-strategy and replace the offending insert by a manual quoting strategy via
(sql/insert-records "test_jdbc_naming3" {((fn [x] (str \" x \")) "b- b") 7})
which corresponds to
(sql/insert-records "test_jdbc_naming3" {"\"b- b\"" 7})
then the code runs fine.

So the issue is located in the handling of the naming-strategy, or in the way I defined the naming-strategy (for postgres).

Fixed in 0.3.0-SNAPSHOT by wiring as-str variable into compatibility layer as :entities argument.

Fixed in 0.3.0-SNAPSHOT and insert-values / insert-rows are fully deprecated.

Fixed in 0.3.0-SNAPSHOT - :transaction? argument is now supported on insert!

Most of this has been done now. It needs extensive testing!

When are you planning on making a stable release? Are the new APIs going to change at all?

There are still a number of open tickets that need completing before I can call this a stable release. Folks are welcome to test the SNAPSHOT release on Sonatype and provide feedback.

0.3.0-alpha1 has been released so this is "done". What's left is polishing and bug fixing.

Implemented. Will be in alpha2.

Attached patch. Is there any straightforward way to test this? Or a place it should be documented?

Docs: just needs good docstring since the clojure.github.com site is auto-generated from those (although it is still triggered manually it seems).

Test: mvn test

I have Derby, HSQLDB, MySQL, MS SQL Server (two drivers), MySQL. I don't have Postgres to test against. By default mvn test only tries Derby, HSQLDB and SQLite. TEST_DBS=... mvn test will test more.

Would be nice to have a test with a custom conversion, mapping any String to some constant perhaps? Perhaps a separate test ns that is required and tested from test_jdbc.clj?

Oh, we have to be careful with testing because I believe any extensions to the protocol are going to be effective for all the remaining tests, right?

Attached new patch with docstrings and a test. I've only run the tests with MySQL. Let me know if there's anything else I can do.

This patch recently came in handy in an different situation, when jdbc was returning Integers instead of Longs, we had a really clean spot to do the conversion.

There have been a lot of code changes since you submitted the patch and right now the patch doesn't apply. Can you re-submit the patch against the latest master branch so I can try again? Thanx!

I'm getting 3 test failures on master currently (with a basic mvn test), and after applying this new patch (p3) I still get 3 failures. So hopefully that means I didn't break anything.

Applied. Will be in 0.3.0-alpha2

I'm thinking this feature is no longer necessary due to the :row-fn option – since the only motivation in the first place was to have the opportunity to transform a result before the connection closes.

There would still be no easy way to do it from Korma, but that's Korma's fault. So maybe we should remove this before 0.3.0 is released?

Duplicate of https://github.com/clojure/java.jdbc/issues/7 - thanx for the patch!

with-naming-strategy, with-quoted-identifiers, as-quoted-str address this.

Having reviewed this after using the library for quite a while, I don't really see any inconsistencies except, perhaps in the naming of update-values (and update-or-insert-values).

insert-values is the base function, taking a table name, a vector of column names and any number of vectors of column values. Generates one insert statement (containing multiple rows). If only one row is inserted, returns a map of generated keys if the database supports that.

insert-rows takes a table name and any number of vectors of column values (for all columns); uses insert-values.

insert-records takes a table name and any number of maps (containing column/value pairs); uses insert-values to insert one record at a time (and thus may return a sequence of maps of generated keys).

insert-record takes a table name and a single map; uses insert-records and returns a map of generated key if the database supports that.

delete-rows takes a table name and a where clause. Perhaps delete-where might be a better name?

update-values takes a table name, a where clause and a map (of column/value pairs to update). Arguably this should be renamed since it doesn't take the same argument pattern as insert-values, but whilst update-records might sound more appropriate it only takes one "record" and update-record is clearly not right since it can update more than one row!

update-or-insert-values (taking the same arguments as update-values) can however lead to inserting a single record but it can also update multiple records instead.

On both of the latter functions, an analogy could be drawn with the findAndModify command in MongoDB which has an upsert option (to insert the document if no matches are found for the update), but most notably, findAndModify only updates a single record. I don't believe SQL has any portable equivalent but perhaps find-and-modify is a better name for update-values? Or perhaps, in the spirit of my suggestion for delete-where, we might call it update-where - and use an optional keyword argument to indicate that an insert-record should be performed when no rows match?

The only other comment I could make on update-values is that it might be nice to have a variant that took: table where-params column-names value-group - somewhat inline with insert-values (although not allowing any number of vectors of column values).

The API aspects of this ticket are covered in JDBC-20 (post 0.1.0) and the struct-map issue is covered in JDBC-15 so I'm closing this out as a duplicate for now.

Essentially duplicated with more focus in JDBC-15 and JDBC-20.

For production usage, I've been relying on c3p0 for connection pooling which works very well with c.j.j so I'm not sure that we need something specific within the library itself. What we do need, of course, is documentation on how to use something like c3p0 for connection pooling with c.j.j!

Given the ease of using 3rd-party connection pooling libraries like c3p0, I think this is low priority (and I may simply close out after documenting how to use c3p0 with c.j.j).

I've documented it here: https://github.com/clojure/java.jdbc/blob/master/doc/clojure/java/jdbc/ConnectionPooling.md

I don't believe this should be in c.j.j itself so I'm going to close this.

Documented how to use a 3rd party library for connection pooling which I think is sufficient since it's so easy.

I can't find the button to edit the issue, this happened on ver. 0.0.6

I actually think that would be rather dangerous. Imagine constructing the where parameters dynamically and accidentally passing in an empty sequence - you'd delete everything in the table.

I believe you can safely and deliberately get the effect you are looking for by doing either of these:

(delete-rows "foo" [true])
(delete-rows "foo" [1])

Given the lack of response to my comment and the fact that there is a clear and explicit workaround, I'm resolving this, declined.

Patch applied. Thanx Stuart!

Thanx Jonas. This made me look thru all my other Clojure code but it was the only instance of replicate that I could find.

Talked this over with Sean on IRC; I'll put together a patch.

Implements said functionality. Patch depends upon supplied patch for JDBC-22.

Hold off on this patch; apparently the scheme and subprotocol don't always match up. Will provide an updated patch.

This one should take care of the subprotocol issue; it also adds support for running the tests against postgres.

Patches applied - thank you!

I've been thinking about this a bit more, and I'm thinking that a function "match-clause" might deal with this in a fairly composable way. So

(match-clause { :name nil :age 42 })

would return

["Name is null and Age = ?" 42]

It's not exactly ClojureQL, but it's non-invasive and makes query-by-example easy to achieve.

That's nice idea. I actually do something similar in the CRUD wrapper I built around java.jdbc at World Singles so that might be a nice addition to the library.

This will be incorporated into the new API I'm adding in JDBC-20 so you can say (where {:name nil :age 42}) and it will return the correct SQL where clause. Accordingly, I'm closing this as a duplicate.

A subset of what will be implemented as part of JDBC-20

This should take care of it. Updated the tests to remove the inferred value.

Sorry, I should have closed this out ages ago - when the patch was applied.

I'm aware that I might have to sign the Clojure CA. I just thought I'd see what you think first...

See JDBC-14 since I'm declining this for the same reason. Performing a global update is an unusual operation and I think the API should prevent you from accidentally updating all rows, just as it should prevent you from deleting all rows. Requiring a where clause does that.

This will break things - see JDBC-16 which required that .addBatch be called when there are no param-groups. I'm not going to reject this out of hand but patches for new database support need testing against MySQL, PostgreSQL and MS SQL Server.

Test suite runs correctly for TEST_DBS=mysql,postgres,derby,hsqldb,sqlite3 and TEST_DBS=mysql-str,postgres-str. This just leaves MS SQL server, which I do not have the ability to run.

Regarding JDBC-16, when I revert to just using .executeBatch for the no param-groups case I can see the errors produced in the test-suite. Using .executeUpdate for the no params-group case continues to fix these errors (note MS SQL server untested). I do not see any other information in the ticket that would explain why using .executeUpdate instead of .addBatch/.executeBatch for the no params-group case would break JDBC-16. Is there a reason I am missing?

I ran the test suite against an ec2 instance with MS SQL server 2008 R2. Current test suite on master fails with attached mssql-error.log. Using patch produces the same output.

Please let me know if there is anything that you would like changed.

Thanx Nelson. I've been a bit busy with work this week but I'll try to look at this at the weekend. I appreciate your thoroughness on testing this!

Integrated parts of the patch. SQLite 3 is now supported. Thanx!

I still get the transaction issue (part 2 from original description) when clojure.java.jdbc tries to return the generated keys, in a transaction, using sqlite. https://gist.github.com/1947746

I believe this is because the ResultSet from .getGeneratedKeys never gets .close called, so sqlite keeps the db locked and can't rollback. Would you like me to make a new issue?

The tests pass locally (Mac OS X) and on the Clojure build server (Linux) but you're right that I didn't include the close call on the record set.

I didn't include your additional test which I'm guessing is the key to highlighting the errant behavior for SQLite?

(I couldn't actually apply your patch because I'd spent some time getting all the tests passing for MS SQL Server with both the Microsoft and jTDS drivers)

Attached patch that fails with `mvn test`.

I believe this was fixed in 0.1.3 (at the end of February). As far as I can tell, all of the patch components are applied. Specifically, your transaction test is included in the test suite and passes.

Nice catch!

clojure 1.3.0
clojure.java.jdbc 0.2.1
oracle 10.2.0.4.0

Interesting. (.getUpdateCount stmt) is a standard part of the JDBC API so it looks reasonable to see if the result of the (.executeBatch stmt) is a single sequence with a -2 value, the code could call (.getUpdateCount stmt) and return that (single) value as a sequence. It looks like that should be completely portable but it will be hard to test since none of the other DBs return -2 as far as I know and I don't have an Oracle install to test against

Dependencies used:

[org.clojure/clojure "1.4.0"]
[org.clojure/java.jdbc "0.2.3"]
[org.xerial/sqlite-jdbc "3.7.2"]

It seems like the more flexible thing to do would be to allow users to specify that a transaction not be used on the existing operations. I'm reworking the API substantially soon so I'll bear that in mind. I might not make it possible with the existing API but I'll make sure that's an option in the new API that I'm adding.

0.2.4-SNAPSHOT contains db-do-commands which allows bare execution of commands against a specific database connection with or without a transaction wrapping the commands.

Neat!

I assume you mean the order of columns within the rows, not the actual order of rows within the result set?

I'm looking at providing an alternative result set format using arrays instead of maps which will preserve column orders.

Since maps are unordered by default in Clojure, I'd be suspicious of code that relies on ordering of keys (so I'm reducing this to 'minor').

You are right, I mean the order of columns, not the rows.

I understand, that maps are unordered. There are few reasons when
the order is matters. The one of them is my task - simple
SQL database client.

Probably it would be better to do this possibility optional,
since it is not used very often?

An optional keyword argument sounds like a reasonable compromise. I'm already looking at keyword arguments on resultset-seq for returning arrays instead of maps and returning an order-preserving map should be as simple as replacing (zipmap keys (row-values)) with (apply array-map (interleave keys (row-values)))

Fixed in 0.3.0-SNAPSHOT by allowing :as-arrays argument to query and result-set-seq functions, which returns result as vector of column names, followed by vectors for each row.

This has sort of been handled along the way with functions used in the test suite to reflect how various databases handle key generation / return. Surprisingly few seem to reliably return keys on an insert

Right now, that's in the internal namespace which is going away so I can't make it part of the API. You can always pull private vars out with @#'clojure.java.jdbc.internal/classnames but bear in mind that internal ns will merge with the parent ns soon!