<< Back to previous view

[JDBC-107] metadata-result leaks ResultSets Created: 12/Mar/15  Updated: 30/Jul/15

Status: Reopened
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Niels van Klaveren Assignee: Sean Corfield
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When working with Oracle DatabaseMetaData, I noticed out of cursors exceptions (ORA-01000) when iterating over tables and requesting their .getIndexInfo and .getImportedKeys information. These errors tend to point to leaked resultsets. Investigating the metadata-result code I noticed this does happen, and the result set are just leaked silently on the other databases I tend to use.

A fix would include a default result-set-fn doall in the :or keys clause, and binding the true clause of the (if (instance? java.sql.ResultSet rs-or-value) to a new var, .close the rs-or-value, and return the new var.



 Comments   
Comment by Sean Corfield [ 12/Mar/15 11:57 AM ]

Thanks Niels. I'll take a look at that shortly. I may have some time next week.

Comment by Sean Corfield [ 18/May/15 11:38 AM ]

I finally got around to looking at this – apologies for the delay – and I'm not convinced metadata-result should close the ResultSet since that is passed in by the user. java.jdbc itself only ever calls metadata-result in a test so I updated that with the way I think it should be handled, by the user:

(sql/with-db-metadata [metadata db-spec]
  (with-open [result (.getTables metadata ...)]
    (let [table-info (sql/metadata-result result)]
      ... do stuff with table-info ...)))

The point here is that the user creates the ResultSet, not java.jdbc, so the user should close it correctly.

Comment by Niels van Klaveren [ 30/Jul/15 8:47 AM ]

The similarities between metadata-result and query (both having row-fn and result-set-fn) made me suppose abstracting away the connection handling of the result-set was the intention. It seems that assumption was a bit too hasty.

However, currently metadata-result IMO straddles uneasily between an implementation that just returns a result-set-seq or value, and one that can do all processing intended for abstracting away connection opening and closing, without actually doing so.

This does opens the way for a quite simple workaround that does though, a simple wrapping macro

(defmacro metadata-query
  [query & params]
  `(with-open [result# ~query]
     (j/metadata-result result# ~@params)))

which takes a java metadata call and the result set processing parameters for metadata-result, and processes the results in a with-open block according to the parameters.

(j/with-db-metadata [meta db-spec]
                    (metadata-query (.getTables meta nil nil nil (into-array String ["TABLE"]))
                                    :result-set-fn (fn [rs] (->> rs
                                                                 (filter #(= (:user db-spec) (:table_schem %)))
                                                                 (map :table_name)
                                                                 doall))))
Comment by Sean Corfield [ 30/Jul/15 10:59 AM ]

A new actionable suggestion has been made.

Comment by Sean Corfield [ 30/Jul/15 11:00 AM ]

That seems like a reasonable addition which would clarify the usage. Thank you.





[JDBC-110] Requesting a nested transaction with a more restrictive isolation level appears to succeed, but does not have the requested effect Created: 22/Jul/15  Updated: 27/Jul/15  Resolved: 27/Jul/15

Status: Resolved
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Donald Ball Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File check-transaction-isolation-level.patch    
Patch: Code and Test

 Description   

The library helpfully allows nested transactions by maintaining an internal depth counter, but silently disregards any transaction options for nested transactions. This has actually bitten me recently in production; code that required a serializable transaction in order to maintain an invariant was inadvertently being executed within a repeatable-read transaction.

This patch changes the behavior to raise if a nested transaction requests a different isolation level in which the actual transaction is running.

A similar problem exists for nested transactions that request read-only mode. I will be happy to modify the patch to cover that case if you are interested in accepting this.



 Comments   
Comment by Sean Corfield [ 22/Jul/15 2:18 PM ]

Thanks Donald. I'll take a look at this.

You don't appear to be listed here: http://clojure.org/contributing – have you signed and submitted the Contributor's Agreement?

Comment by Donald Ball [ 23/Jul/15 1:34 PM ]

Not yet, but I will do so forthwith.

Comment by Donald Ball [ 23/Jul/15 1:37 PM ]

Okay, I'm legal.

Comment by Sean Corfield [ 23/Jul/15 1:38 PM ]

Great! I'll try to take a look at this over the weekend.

Comment by Sean Corfield [ 27/Jul/15 12:46 AM ]

Thanks for the patch Donald!

Comment by Donald Ball [ 27/Jul/15 8:38 AM ]

Would you be amenable to also raising if the inner transaction requests an incompatible read-only mode?

Comment by Sean Corfield [ 27/Jul/15 12:03 PM ]

Sure. Do you want to create a new ticket and attach a patch to it?





[JDBC-109] :port should default to 5432 for PostgreSQL Created: 11/Jun/15  Updated: 27/Jul/15  Resolved: 27/Jul/15

Status: Resolved
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: None


 Description   

We default the port for MySQL and SQL Server. We should do it for PostgreSQL too.

We should probably clean up the handling of Apache Derby, SQLite, H2, and HSQLDB with :dbtype so the host/port are omitted.



 Comments   
Comment by Sean Corfield [ 27/Jul/15 1:19 AM ]

Port defaulted and Derby, SQLite and HSQLDB updated to support dbtype / dbname.





[JDBC-111] db-do-prepared does not correctly check if transaction? was passed Created: 26/Jul/15  Updated: 27/Jul/15  Resolved: 27/Jul/15

Status: Resolved
Project: java.jdbc
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stefan Kamphausen Assignee: Sean Corfield
Resolution: Completed Votes: 0
Labels: None
Environment:

[org.clojure/java.jdbc "0.3.7"]
Ubuntu Linux 12.04
Testing against a DB2 database


Attachments: Text File DBC-111-check-PS.patch     Text File DBC-111-with-docstring.patch    

 Description   

The function db-do-prepared allows an optional second parameter "transaction?". Whether or not this was passed is done by checking it for being a string:

(if (string? transaction?)
      (apply db-do-prepared db true transaction? opts)

The assumption behind this seems to be, that a string indicates an SQL query parameter.

But, as the code a little further down shows, users can also pass in a prepared statement:

(if (instance? PreparedStatement sql)

So, to find out if transaction? was passed as a parameter one could either check for string and prepared statement or turn the logic around an check for boolean:

(if (or (string? transaction?)
            (instance? PreparedStatement sql))
      (apply db-do-prepared db true transaction? opts)
      (if-let [^java.sql.Connection con (db-find-connection db)]

Or

(if (not (instance? Boolean transaction?))
      (apply db-do-prepared db true transaction? opts)

The latter does not deal with nil which would probably be a natural choice for passing falsehood while the former would have to be extended when another thing may be passed as SQL, which seems unlikely.



 Comments   
Comment by Sean Corfield [ 26/Jul/15 2:11 PM ]

The assumption was that if you wanted to pass a PreparedStatement, you had to provide transaction? first (i.e., it could only be omitted when passing a sql string).

I can see value in allowing it to be omitted when a PreparedStatement is passed by expanding the condition to: (if (or (string? transaction?) (instance? PreparedStatement transaction?)) ...

Comment by Stefan Kamphausen [ 26/Jul/15 3:56 PM ]

Assuming that transaction? be passed anyway, if a prepared statement is passed, is fair enough, too. In that case, I humbly suggest adjusting the docstring a bit.

BTW, just replacing

(if (string? transaction?)

with

(if (or (string? transaction?)
          (instance? PreparedStatement sql))

Breaks the tests. Seems like it is not as easy as it seemed at first glance.

Comment by Stefan Kamphausen [ 26/Jul/15 4:01 PM ]
(if (or (string? transaction?)
          (instance? PreparedStatement transaction?))

works of course. Sorry for the noise.

Comment by Stefan Kamphausen [ 26/Jul/15 4:11 PM ]

1. Adjust check for transaction? to test for PreparedStatement, too

2. First patch plus addition to docstring.

Comment by Sean Corfield [ 27/Jul/15 12:31 AM ]

Fixed. Thank you!





Generated at Sun Aug 02 21:47:52 CDT 2015 using JIRA 4.4#649-r158309.