-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: FIR-43038 allow jdbc to send batch as a single multi statement #494
base: master
Are you sure you want to change the base?
feat: FIR-43038 allow jdbc to send batch as a single multi statement #494
Conversation
Quality Gate passedIssues Measures |
if (sessionProperties.isMergePreparedStatementBatches()) { | ||
if (!inserts.isEmpty()) { | ||
execute(List.of(asSingleStatement(inserts))); | ||
} | ||
} else { | ||
execute(inserts); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be checking that all of the statements are inserts here? I don't think we have a guarantee of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's parsing of statement, which I believe we should avoid. Since the new parameter is "internal" (not documented), and is only expected to be used by Debezium, we should be safe. We'll have this properly supported when we'll have server-side multitatements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, agreed. I was thinking this would be safer if we batched only inserts and everything else would run as before, but for Debezium this should be sufficient.
Let's rename the variable though to avoid confusion, inserts
implies we know we have only inserts here.
try (java.sql.PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO test_table VALUES (?)")) { | ||
for (int i = 0; i < 10; i++) { | ||
preparedStatement.setInt(1, i); | ||
preparedStatement.addBatch(); | ||
} | ||
preparedStatement.executeBatch(); | ||
} | ||
try (ResultSet rs = statement.executeQuery("SELECT COUNT(*) FROM test_table")) { | ||
assertTrue(rs.next()); | ||
assertEquals(10, rs.getInt(1)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a potential improvement, to verify this is indeed sent in one batch we can set a query label, then execute batch and check query history for that label. It should have only one result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise lgtm
Added
merge_prepared_statement_batches
parameter to JDBC. If will send all queries in a prepared statement batch as a single http request, separated by semicolon.