Skip to content

Commit

Permalink
Caching to avoid reparsing SQL text
Browse files Browse the repository at this point in the history
  • Loading branch information
@TobiasSQL
TobiasSQL committed Apr 27, 2017
1 parent c1f88c5 commit 0a52a12
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 11 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ local.properties
.classpath
.vscode/
.settings/
.gradle/
.loadpath

# External tool builders
Expand Down
5 changes: 3 additions & 2 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ repositories {

dependencies {
compile 'com.microsoft.azure:azure-keyvault:0.9.7',
'com.microsoft.azure:adal4j:1.1.3'

'com.microsoft.azure:adal4j:1.1.3',
'com.google.guava:guava:19.0'

testCompile 'junit:junit:4.12',
'org.junit.platform:junit-platform-console:1.0.0-M3',
'org.junit.platform:junit-platform-commons:1.0.0-M3',
Expand Down
7 changes: 7 additions & 0 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@
<optional>true</optional>
</dependency>

<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>19.0</version>
<optional>false</optional>
</dependency>

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Apr 29, 2017

Contributor

I don't think a dependency on guava is going to fly. But a simple LRU cache such as the one I wrote for jTDS is trivial to implement (associated key class). Requires external synchronization.

This comment has been minimized.

Sign in to view

Copy link
@sehrope

sehrope Apr 29, 2017

Contributor

-1 to adding additional dependencies just for an LRU cache. Particularly something as large as Guava.

Here's another example of a LRU cache from the Postgres JDBC drive: https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/util/LruCache.java

Separately, but much more importantly, the cache cannot be a static. It needs to be non-static per connection (really per PreparedStatement object). Otherwise two different connections to potentially different databases executing the same SQL will resolve to the same cache entry.

If it is going to be statically referenced (which again I think is a bad idea, see next section), at the very least the cache key should include the "signature" of the connection (most likely the reconstituted JDBC URL). That would allow for separate connections to cache common metadata at the driver level. I don't think the memory savings would be worth it though. If it's decided to go this route I'd suggest enabling this via a connection property with it off by default.

Caching outside the connection also has the issue that it never gets invalidated. If your database schema changes you have to kill and restart the app to pick up the changes. If it's per connection then it'll happen automatically when the old connections cycle out of your connection pool.

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Apr 29, 2017 •
edited
Loading

Contributor

Ok, I'm going to make some work for you @TobiasSQL, sorry (now I'm pumped).

What we need is a map of caches, in order to safely share the parsed SQL cache across Connections. Entries should be reference counted; I'll explain.

The top-level map can be static as far as I am concerned (it is scoped by ClassLoader).

  • The top-level map key should be a composite key class, comprised of SQLServerConnection's:
    • currentConnectPlaceHolder.getServerName()
    • currentConnectPlaceHolder.getPortNumber()
    • databaseName

So basically (names are placeholders only):

class CacheMapKey {
   String serverName;
   String databaseName;
   int serverPort;
   int refCount;

   // equals/hashCode excluding refCount
}

The top-level Map looks like something like:

static class ParsedSQLCacheMap {
   private static HashMap<CacheMapKey, ParsedSqlLRUCache> parsedSqlLRUCacheMap;

   static synchronized ParsedSqlLRUCache leaseParsedCache(CacheMapKey key) {
      ParsedSqlLRUCache cache = parsedSQLCacheMap.computeIfAbsent(key, (k, v) -> new ParsedSQLCache());
      cache.refCount++;
      return cache;
   }

   static synchronized void releaseParsedCache(CacheMapKey key, ParsedSQLCache cache) {
      if (0 == --cache.refCount) {
         parsedSQLCacheMap.remove(key);
      }
   }
}

The SQLServerConnection "leases" a cache in its constructor and stores it in a field, and releases it in close(). This provides collision-free parsed cache sharing across Connections. Users who need to flush the cache in a connection pooled environment simply flush the pool.

By the way, the SQLServerStatement can also leverage the cache. See ensureSQLSyntax(). This would extend the benefit to adhoc SQL for a big win.

Also, regarding the parsed SQL LRUCache, I would not use the SQL text as the key, it can consume a large amount of memory. If it were me, I would use an SHA1 of the text as a key and not worry about collisions. We're more likely to experience the heat-death of the universe before a collision would occur. Several million SHA1 keys can be computed per second, so I wouldn't sweat the overhead.
<!-- dependency for running tests -->
<dependency>
<groupId>junit</groupId>
Expand Down
92 changes: 83 additions & 9 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
import java.util.Vector;
import java.util.logging.Level;

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.Cache;

/**
* SQLServerPreparedStatement provides JDBC prepared statement functionality. SQLServerPreparedStatement provides methods for the user to supply
* parameters as any native Java type and many Java object types.
Expand Down Expand Up @@ -98,6 +101,48 @@ public class SQLServerPreparedStatement extends SQLServerStatement implements IS
*/
private boolean encryptionMetadataIsRetrieved = false;

/** Size of the prepared statement meta data cache */
static final private int preparedStatementMetadataSQLCacheSize = 100;

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Apr 29, 2017

Contributor

Isn't this a "parsedSQLCache" rather than a "preparedStatementMetadataSQLCache"? Or is this a case of forward-looking naming?

/** Cache of prepared statement meta data */
static private Cache<String, PreparedStatementMetadataSQLCacheItem> preparedStatementSQLMetadataCache;
static {
preparedStatementSQLMetadataCache = CacheBuilder.newBuilder()
.maximumSize(preparedStatementMetadataSQLCacheSize)
.build();
}

/**
* Used to keep track of an individual handle ready for un-prepare.
*/
private final class PreparedStatementMetadataSQLCacheItem {
String preparedSQLText;
int parameterCount;
String procedureName;
boolean bReturnValueSyntax;

PreparedStatementMetadataSQLCacheItem(String preparedSQLText, int parameterCount, String procedureName, boolean bReturnValueSyntax){
this.preparedSQLText = preparedSQLText;
this.parameterCount = parameterCount;
this.procedureName = procedureName;
this.bReturnValueSyntax = bReturnValueSyntax;
}
}

/** Get prepared statement cache entry if exists */
public PreparedStatementMetadataSQLCacheItem getCachedPreparedStatementSQLMetadata(String initialSql){
return preparedStatementSQLMetadataCache.getIfPresent(initialSql);
}

/** Cache entry for this prepared statement */
public PreparedStatementMetadataSQLCacheItem metadataSQLCacheItem;

/** Add cache entry for prepared statement metadata*/
public void cachePreparedStatementSQLMetaData(String initialSql, PreparedStatementMetadataSQLCacheItem newItem){

preparedStatementSQLMetadataCache.put(initialSql, newItem);
}

// Internal function used in tracing
String getClassNameInternal() {
return "SQLServerPreparedStatement";
Expand Down Expand Up @@ -128,13 +173,34 @@ String getClassNameInternal() {
stmtPoolable = true;
sqlCommand = sql;

JDBCSyntaxTranslator translator = new JDBCSyntaxTranslator();
sql = translator.translate(sql);
procedureName = translator.getProcedureName(); // may return null
bReturnValueSyntax = translator.hasReturnValueSyntax();

userSQL = sql;
initParams(userSQL);
// Save original SQL statement.
sqlCommand = sql;

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Apr 29, 2017 •
edited
Loading

Contributor

Why is sqlCommand necessary? Isn't userSQL sufficient? I understand having it as a local, but as a field seems unnecessay.

// Check for cached SQL metadata.
PreparedStatementMetadataSQLCacheItem cacheItem = getCachedPreparedStatementSQLMetadata(sql);

// No cached meta data found, parse.
if(null == cacheItem) {

This comment has been minimized.

Sign in to view

Copy link
@brettwooldridge

brettwooldridge Apr 29, 2017

Contributor

Space after if. 😉
JDBCSyntaxTranslator translator = new JDBCSyntaxTranslator();

userSQL = translator.translate(sql);
procedureName = translator.getProcedureName(); // may return null
bReturnValueSyntax = translator.hasReturnValueSyntax();

// Save processed SQL statement.
initParams(userSQL);

// Cache this entry.
cacheItem = new PreparedStatementMetadataSQLCacheItem(userSQL, inOutParam.length, procedureName, bReturnValueSyntax);
cachePreparedStatementSQLMetaData(sqlCommand/*original command as key*/, cacheItem);
}
else {
// Retrieve from cache item.
procedureName = cacheItem.procedureName;
bReturnValueSyntax = cacheItem.bReturnValueSyntax;
userSQL = cacheItem.preparedSQLText;
initParams(cacheItem.parameterCount);
}
}

/**
Expand Down Expand Up @@ -217,12 +283,11 @@ final void closeInternal() {
}

/**
* Intialize the statement parameters.
* Find and intialize the statement parameters.
*
* @param sql
*/
/* L0 */ final void initParams(String sql) {
encryptionMetadataIsRetrieved = false;
int nParams = 0;

// Figure out the expected number of parameters by counting the
Expand All @@ -231,6 +296,15 @@ final void closeInternal() {
while ((offset = ParameterUtils.scanSQLForChar('?', sql, ++offset)) < sql.length())
++nParams;

initParams(nParams);
}

/**
* Intialize the statement parameters.
*
* @param sql
*/
/* L0 */ final void initParams(int nParams) {
inOutParam = new Parameter[nParams];
for (int i = 0; i < nParams; i++) {
inOutParam[i] = new Parameter(Util.shouldHonorAEForParameters(stmtColumnEncriptionSetting, connection));
Expand Down

1 comment on commit 0a52a12

@brettwooldridge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, nice work.

Please sign in to comment.