trafodion
trafodion copied to clipboard
apache
TRAFODION-2957 one stmt support multi-queries
- reconstitute the constructor of TrafT4Statement & TrafT4PreparedStatement
- add new property allowMultiQueries to support exec multi-queries in one statement
- new class extends from TrafT4Statement & TrafT4PreparedStatement to exec multi-queries
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2582/
Can you explain a bit how this will work at the JDBC interface level? Will a user pass a statement like this to Connection::prepareStatement:
conn.prepareStatement("begin insert into t1 values(?,?); delete from t2; insert into t2 values (?); end");
If/when we resurrect the Trafodion code to support compound statements, will the interface work the same?
I am not clear on the application interface to support this feature. Are we trying to conform to mysql way as given in this link https://stackoverflow.com/questions/10797794/multiple-queries-executed-in-java-in-single-statement
@zellerh seems you lose a ";" after begin, if no ";" there will throw syntax error. so if query like "begin; insert into t1 values(?,?); delete from t2; insert into t2 values (?); end", actually there will have 5 preparedStatements , each do one query.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2587/
@mashengchen, I was thinking of a compound statement in my example. Your reply and the link that @selvaganesang provided helped me understand better. Trying to summarize what I heard, what you are implementing is a way to run multiple statements, in the following way:
- Users need to specify an option in the connection string to enable this feature.
- Users need to specify multiple statements, separated by semicolons (no begin/end block).
- This feature does not implement a compound statement (begin end block).
If that's true, then that sounds ok to me. I just wanted to make sure we can add compound statements like in MySQL or IBM DB2 later without running into trouble with your feature.
@zellerh it's right in your summary, this PR is just a simple implement of the feature, the fully implement should change the sql engine code, which i am not familiar with it.
You should ensure that all the methods of the standard PreparedStatement and Statement work seamlessly with the multi-queries statements. If a particular method doesn't make sense or can't work correctly, then it is better to return an error. For eg. addBatch, executeBatch etc.
In case of select statement, I couldn't figure out how result sets from different select statements are interfacing with the application.
The transaction boundaries needs to be explained too for both autocommit and non-autocommit mode.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2610/
Previous Test Aborted. New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2611/
It looks like the weakReference of Statement object is removed. The weakReference is used to implicitly close the statement to clean up the resources associated with the Statement object if the application doesn't explicitly close the statement. This is to take care of resource leaks in the SQL side. Are there any other alternatives added to take care of such resource leaks. If not, I would suggest this PR may not be merged.
@selvaganesang the weakReference still exist, I just refactor the construction of TrafT4Statement & TrafT4PreparedStatement, each time invoke TrafT4MultiQueriesStatement or TrafT4MultiQueriesPreparedStatement or TrafT4PreparedStatement, there will invoke the construction of TrafT4Statement, in which there will make the statement weakReference. see line 1415 of TrafT4Statement, it's just fold in the page.
Thanks @mashengchen. Now I get it. You have refactored to call another constructor within a constructor. I remember there was one constructor of T4Statement (that is called internally) where the weakReference shouldn't be constructed. I am hoping that remains the same way even now.
@selvaganesang i think what you descript is TrafT4Statement(); i did not change the behavior that. also i check all constructor, and there is no add behavior and no lose behavior.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2638/
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2643/
@selvaganesang and @zellerh, does this change look ready to merge now?
I am ok with merging this PR but the following needs to be addressed soon.
- str.split(';') could split at the wrong place if there is ';' in string literal in the given sql string. This could result in unintended syntax error.
- Transaction semantics needs to be clarified.
I had raised these concerns earlier too
I'm concerned that the issue Selva points out (incorrect parsing of semicolons) is not just a potential syntax error, but a potential security vulnerability -- via SQL injection into string values. If so, then "fix it later" is not good enough. It needs to be fixed before this is merged.
Thanks @svarnau for pointing that out. I didn't sufficiently pick that up out of @selvaganesang's comments. I agree: the parsing issue needs to be fixed to avoid SQL injection vulnerabilities.
Thanks, very good point! That always makes me think of this cartoon: https://xkcd.com/327/.