DBZ-693 Initial SQL statements #516
Conversation
There was a problem hiding this comment.
@jpechane A few small remarks inline. Also what do you think about adding it to PG, too?
| .withType(Type.STRING) | ||
| .withWidth(Width.LONG) | ||
| .withImportance(Importance.LOW) | ||
| .withDescription("A semicolon separated list of SQL statements to be executed when connection to database is established. " |
There was a problem hiding this comment.
I think we should clarify whether that only applies to snapshot reading, log reading, or both.
There was a problem hiding this comment.
Also the default (no statements) should be mentioned.
There was a problem hiding this comment.
I tried to precise it a little bit.
| @@ -586,6 +586,10 @@ public synchronized Connection connection() throws SQLException { | |||
| if (conn == null) throw new SQLException("Unable to obtain a JDBC connection"); | |||
| // Always run the initial operations on this new connection | |||
| if (initialOps != null) execute(initialOps); | |||
| final String statements = config.getString(JdbcConfiguration.ON_CONNECT_STATEMENTS); | |||
| if (statements != null) { | |||
| execute(statements.split(";")); | |||
There was a problem hiding this comment.
Should we handle the case where a ; is given within a string literal?
There was a problem hiding this comment.
TBH, I tried to postpone the scenario till it happens :-). But we can add a support for escaping - either using backslash or doubling the semicolon. In this case I'd probably prefer doubling the semicolon.
|
@gunnarmorling Typos fixed, postgres support added, semicolon as a char supported |
| @@ -581,11 +582,49 @@ public synchronized boolean isConnected() throws SQLException { | |||
| } | |||
|
|
|||
| public synchronized Connection connection() throws SQLException { | |||
| return connection(true); | |||
There was a problem hiding this comment.
I think it'd be safer to not execute the statements when calling the parameterless connection(). Note that when using the PG connector, I see the insert being executed three times, so something is still not quite correct.
There was a problem hiding this comment.
@gunnarmorling It is correct, as postgres connector creates multiple connections in sequence. The functionality is not for executing DML (albeit I abuse it for tests) just mainly for session configuration and in this case I expect the session to be configured every time the connection is created.
There was a problem hiding this comment.
Ok, I'll expand the option's description a little bit then to clarify that fact.
| if (conn == null) { | ||
| conn = factory.connect(JdbcConfiguration.adapt(config)); | ||
| if (conn == null) throw new SQLException("Unable to obtain a JDBC connection"); | ||
| // Always run the initial operations on this new connection | ||
| if (initialOps != null) execute(initialOps); | ||
| final String statements = config.getString(JdbcConfiguration.ON_CONNECT_STATEMENTS); |
There was a problem hiding this comment.
I'd suggest to extract all the parsing logic into its own method which then returns the list of statements to here.
There was a problem hiding this comment.
@gunnarmorling Done
|
Rebased and applied. Added one commit to expend the test a bit and update the connector option description. Thanks, @jpechane! |
https://issues.jboss.org/browse/DBZ-693