1

We have the following code :

String templateQuery = "select * from my_table where col1=$1 or col2 like '%$2.$1'";
String tmp = templateQuery;

for(int i=1;i<=maxCols;i++) {
    tmp = tmp.replaceAll("\\$"+i, data[i-1]);
}

This code works fine as maxCols never exceeds 10. But my colleague disagree with me stating that this code consumes too much memory. Can you help us ?

EDIT: I have change the initial templateQuery with a much realistic one. Secondly, templateQuery can potentially be a big string.

EDIT 2: Thanks for those who have pointed out the SQLInjection problem.

Improve this question
4
  • Define "works fine". And define "too much memory". Commented Apr 29, 2011 at 9:00
  • In terms of "too much memory", the temporary memory consumed should be far less than 10KB which is worth less than 0.1 cents of memory ;) Commented Apr 29, 2011 at 9:15
  • @Peter @Oli the templateQuery can be much bigger than in the example. Commented Apr 29, 2011 at 10:08
  • So if its 100K, thats 1 cent worth of temporary memory. ;) Commented Apr 29, 2011 at 13:54

5 Answers 5

Reset to default
6

Don't do this.

Not for performance reasons (which will be miniscule compared with the cost of the database query), but to avoid SQL injection attacks. What happens if data[0] is actually the string

' OR 'x' = 'x

?

Then you'll end up with a SQL statement of:

SELECT * FROM my_table WHERE col1='' OR 'x' = 'x'

which I think we can agree isn't what you wanted.

Use a parameterized SQL statement instead (PreparedStatement) and get the database driver to send the parameter values separately.

EDIT: In other comments, the OP has specified that the template string can be quite long, and some parameters may actually involve multiple initial values combined together. I still say that the cost of replacement is likely to be insignificant in the grand scheme of things, and I still say that PreparedStatement is the way to go. You should perform whatever combining operations you need to on the input before setting them as the values for the PreparedStatement - so the template may need the SQL with SQL placeholders, and then "subtemplates" to work out how to get from your input to the parameters for the PreparedStatement. Whatever you do, putting the values directly into the SQL is the wrong approach.

Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

+1: And for maintability of code and using standard practices. BTW: The resource cost of a single database query is far more than the cost of a few Strings.
@Peter: I'm sure I had that (the bit about the performance difference being minimal) in an edit somewhere... it appears to have gone :(
In our application, when running the query, we always limit the query results to the first 50 rows.
@Stephan: Why do you think that's relevant to the question?
@Jon My comment was a reply to the response in your response. Even if the attacker performs SELECT * FROM my_table WHERE col1='' OR 'x' = 'x', he will only gets the 50 first rows.
|
3

Why aren't you just using a PreparedStatement with replacement parameters?

String templateQuery = "SELECT * FROM my_table WHERE col1 = ?";
PreparedStatement ps = con.prepareStatement(templateQuery);
for (int i = 0; i < data.length; i++) {
    ps.setString(i + 1, data[i]);
}
ResultSet rs = ps.executeQuery();

You're otherwise vulnerable to SQL injection if you use string replacement like you have.

Improve this answer

6 Comments

We can have this type of template : select * from my_table where col1=$1 or col2 like '%$2.$1' so PreparedStatement is not well suited
@Stephan: That just means you need to do some preprocessing of your values (and the template) before using PreparedStatement. You should still absolutely not put the values directly into the SQL.
@Jon I clearly understand the SQLInjection big problem. In our case, this is not a concern because the application is a small tool for internal use only (2 users so far...). The main concern is about transforming the templateQuery efficiently.
@Stephan: I still think it's the wrong way to approach the problem. It may be only used internally at the moment... but taking poor decisions like this now tends to bite you later on. If it's just a small internal tool, how much does the performance actually matter - and have you measured it? Without measurement, and discussion of performance is pretty pointless.
@Jon Performance matter because we want the processing of templateQuery be less than 2 seconds. This processing can be rougthly divided into 3 steps : transforming the templateQuery into a resulting query handable by the database server, executing the resulting query and displaying the results. The question arises on performing the first step as quick as possible.
|
3

He is correct, because you create maxCols tmp Strings. I realized that it is for Sql commands, if is it, why you do not use PreparedStatement (http://download.oracle.com/javase/1.4.2/docs/api/java/sql/PreparedStatement.html) for this task?

Also, for formatting strings, rather than use substitute, use Formatter, it is much more elegant: http://download.oracle.com/javase/1.5.0/docs/api/java/util/Formatter.html

Improve this answer

8 Comments

No, he's not correct - it will consume some memory, but do you really think 10 extra strings is going to be significant compared with the cost of a database query? This is a classic example of micro-optimization, concentrating on a tiny problem when there's a whopping great big problem which is best solved in way which avoids the micro-optimization problem anyway.
It is a very simple and easy optimization, and also, he is not using good practices, why he should not make it better?
@Pih: Because it's the wrong fix. The code is still broken afterwards - and the performance difference will almost certainly be unmeasurably small. The OP's colleague is fixating on the wrong issue, and should be told so.
Yes, the performance issue is the smallest problem there, I cited the PreparedStatement for others problems, like SQL Injection and how it looks consecutive SQL commands, it will be better for the DB.
Hmm. I think Pih is correct on this one. Jon Skeet is completely right in his post (obviously), but the actual question was wether a string replacement this way would consume too much memory. Which, indeed, is micro-optimization. Still, that's the question, and it should be answered.
|
0

Whether this consumes too much memory is open to debate (what's "too much"?)

Nonetheless, for this kind of stuff you should use PreparedStatement. It allows you to do pretty much exactly what you're trying to achieve, but in a much cleaner fashion.

Improve this answer

1 Comment

templateQuery can be potentially much more bigger than the example.
0

Your colleague is right in that every string replacement creates a new copy of the string. (However, the cost of these is probably negligible with less than 10 parameters.) Moreover, for every execution of this query the SQL engine needs to parse it afresh, which consumes far more additional resources each time.

The potential bigger problem though is that the code is suscept to SQL injection. If the input data is coming from an external source, a hacker can pass in a parameter such as "col1; drop table my_table;", effectively deleting your whole table.

All of these can be solved by using a PreparedStatement instead.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.