Skip to content
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.

Sign up for GitHub

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

Resetting Schema and Catalog, when connection returend to pool #104

Merged
merged 1 commit into from
Oct 23, 2024
+129 −10
Merged

Resetting Schema and Catalog, when connection returend to pool #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions ebean-datasource/src/main/java/io/ebean/datasource/pool/ConnectionDelegator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
abstract class ConnectionDelegator implements Connection {

private final Connection delegate;
protected String currentSchema;

ConnectionDelegator(Connection delegate) {
this.delegate = delegate;
Expand All @@ -20,12 +19,6 @@ Connection delegate() {
return delegate;
}

@Override
public final void setSchema(String schema) throws SQLException {
delegate.setSchema(schema);
currentSchema = schema;
}

@Override
public final String getSchema() throws SQLException {
return delegate.getSchema();
Expand Down
40 changes: 37 additions & 3 deletions ebean-datasource/src/main/java/io/ebean/datasource/pool/PooledConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ final class PooledConnection extends ConnectionDelegator {
*/
private boolean failoverToReadOnly;
private boolean resetAutoCommit;
private boolean resetSchema;
private boolean resetCatalog;
private String currentSchema;
private String currentCatalog;
private final String originalSchema;
private final String originalCatalog;

private long startUseTime;
private long lastUseTime;
/**
Expand All @@ -106,7 +113,7 @@ final class PooledConnection extends ConnectionDelegator {
* close() will return the connection back to the pool , while
* closeDestroy() will close() the underlining connection properly.
*/
PooledConnection(ConnectionPool pool, int uniqueId, Connection connection) {
PooledConnection(ConnectionPool pool, int uniqueId, Connection connection) throws SQLException {
super(connection);
this.pool = pool;
this.connection = connection;
Expand All @@ -115,6 +122,8 @@ final class PooledConnection extends ConnectionDelegator {
this.maxStackTrace = pool.maxStackTraceSize();
this.creationTime = System.currentTimeMillis();
this.lastUseTime = creationTime;
this.currentSchema = this.originalSchema = connection.getSchema();
this.currentCatalog = this.originalCatalog = connection.getCatalog();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rbygrave thanks for merging,
this PR solves our current problem.

But I want to share a few more thoughts that occurred to me about this PR:

getSchema and getCatalog may communicate with the database. In other words, we send some TCP/IP packets over the network. Since this code is only called when the pool grows, which should rarely happen, it will not cost much runtime.

But what is your philosophy here?

  • be as fast as possible (then we might read the current value once before changing it)
  • trade off between performance and complexity/security
  • be as secure as possible (then we might restore the current value always and not only when resetXXX is set)

The latter point in particular is giving me a headache: as long as you use the setSchema and setCatalog methods, everything works correctly. But as soon as someone would manually execute a statement like set schema xyz (or use database xyz), the pool doesn't recognize a schema change and the connection points to the wrong database.

How I discovered this:

You may remember our problem that we currently have a separate connection pool for each tenant, which leads to a resource problem for many tenants. (Ebean runs with TenantMode.DB_WITH_MASTER)

We have now experimented to see whether we can use a connection pool and simply switch the schema beforehand. (We still use DB_WITH_MASTER but our TenantDataSourceProvider only provides a simple delegate data source that takes care of the schema change)
In concrete terms, there is a new SchemaSwitchingDataSource(config.getDataSource(), tenantSchema) for each tenant, which then points to the MASTER db and uses the config.getDataSource() pool from ebean.

This also worked well for the tenants, as long as the SchemaSwitchingDataSource is always used and a schema change is carried out beforehand.

The code that runs in our management tenant (=MASTER) uses plain config.getDataSource() and here I noticed that I was sometimes in the wrong schema (fixed by this PR)

As mentioned above, as long as setSchema / setCatalog is used, everything works now. But if some bad written code will invoke a set schema xyz statement, the management tenant may point to the wrong schema when it picks that connection, because it does no schema switch here. (By the way, the same problem may exist also for autocommit and isolation level - but it is not as bad as if incorrect data is accessed, as would be the case here.)

So I'm wondering,

  • how and if we should prevent this in the DataSourcePool.
  • whether I prevent this in my application. E.g. using DataSourcePoolListener.onAfterBorrowConnection that checks the connection or use also a SchemaSwitchingDataSource for the master DB (I think I will take this option)
  • or rely that every developer writes correct code 😉

I don't think there is any need for action on your part for now, but I wanted to share this with you.

Roland

pool.inc();
}

Expand All @@ -130,6 +139,8 @@ final class PooledConnection extends ConnectionDelegator {
this.maxStackTrace = 0;
this.creationTime = System.currentTimeMillis();
this.lastUseTime = creationTime;
this.currentSchema = this.originalSchema = "DEFAULT";
this.currentCatalog = this.originalCatalog = "DEFAULT";
}

/**
Expand Down Expand Up @@ -272,7 +283,7 @@ void returnPreparedStatement(ExtendedPreparedStatement pstmt) {
*/
@Override
public PreparedStatement prepareStatement(String sql, int returnKeysFlag) throws SQLException {
String key = sql + ':' + currentSchema + ':' + returnKeysFlag;
String key = sql + ':' + currentSchema + ':' + currentCatalog + ':' + returnKeysFlag;
return prepareStatement(sql, true, returnKeysFlag, key);
}

Expand All @@ -281,7 +292,7 @@ public PreparedStatement prepareStatement(String sql, int returnKeysFlag) throws
*/
@Override
public PreparedStatement prepareStatement(String sql) throws SQLException {
String key = sql + ':' + currentSchema;
String key = sql + ':' + currentSchema + ':' + currentCatalog;
return prepareStatement(sql, false, 0, key);
}

Expand Down Expand Up @@ -411,6 +422,16 @@ public void close() throws SQLException {
resetIsolationReadOnlyRequired = false;
}

if (resetSchema) {
connection.setSchema(originalSchema);
resetSchema = false;
}

if (resetCatalog) {
connection.setCatalog(originalCatalog);
resetCatalog = false;
}

// the connection is assumed GOOD so put it back in the pool
lastUseTime = System.currentTimeMillis();
connection.clearWarnings();
Expand Down Expand Up @@ -511,6 +532,7 @@ public void setReadOnly(boolean readOnly) throws SQLException {
connection.setReadOnly(readOnly);
}


/**
* Also note the Isolation level needs to be reset when put back into the pool.
*/
Expand Down Expand Up @@ -673,11 +695,23 @@ public void setAutoCommit(boolean autoCommit) throws SQLException {
}
}

@Override
public void setSchema(String schema) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setSchema()");
}
currentSchema = schema;
resetSchema = true;
connection.setSchema(schema);
}

@Override
public void setCatalog(String catalog) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setCatalog()");
}
currentCatalog = catalog;
resetCatalog = true;
connection.setCatalog(catalog);
}

Expand Down
92 changes: 92 additions & 0 deletions ebean-datasource/src/test/java/io/ebean/datasource/pool/SchemaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package io.ebean.datasource.pool;

import io.ebean.datasource.DataSourceConfig;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

class SchemaTest {

private final ConnectionPool pool;

SchemaTest() {
pool = createPool();
}

private ConnectionPool createPool() {
DataSourceConfig config = new DataSourceConfig();
config.setUrl("jdbc:h2:mem:tests");
config.setUsername("sa");
config.setPassword("");
config.setMinConnections(2);
config.setMaxConnections(4);

return new ConnectionPool("test", config);
}

@BeforeEach
void initTables() throws SQLException {
try (Connection conn = pool.getConnection()) {
Statement statement = conn.createStatement();
statement.execute("CREATE TABLE test (name VARCHAR(255))");
statement.execute("INSERT INTO test (name) VALUES ('default schema')");
statement.execute("CREATE SCHEMA SCHEMA1");
statement.execute("CREATE SCHEMA SCHEMA2");
conn.commit();
}
try (Connection conn = pool.getConnection()) {
String orig = conn.getSchema();
conn.setSchema("SCHEMA1");
Statement statement = conn.createStatement();
statement.execute("CREATE TABLE test (name VARCHAR(255))");
statement.execute("INSERT INTO test (name) VALUES ('schema1')");
conn.setSchema("SCHEMA2");
statement.execute("CREATE TABLE test (name VARCHAR(255))");
statement.execute("INSERT INTO test (name) VALUES ('schema2')");
conn.setSchema(orig);
conn.commit();
}
}

@AfterEach
void after() {
pool.shutdown();
}

@Test
void getConnectionWithSchemaSwitch() throws SQLException {
try (Connection conn = pool.getConnection()) {
Statement statement = conn.createStatement();
ResultSet rs = statement.executeQuery("SELECT name FROM test");
assertThat(rs.next()).isTrue();
assertThat(rs.getString("name")).isEqualTo("default schema");
}
try (Connection conn = pool.getConnection()) {
conn.setSchema("SCHEMA1");
Statement statement = conn.createStatement();
ResultSet rs = statement.executeQuery("SELECT name FROM test");
assertThat(rs.next()).isTrue();
assertThat(rs.getString("name")).isEqualTo("schema1");
}
try (Connection conn = pool.getConnection()) {
conn.setSchema("SCHEMA2");
Statement statement = conn.createStatement();
ResultSet rs = statement.executeQuery("SELECT name FROM test");
assertThat(rs.next()).isTrue();
assertThat(rs.getString("name")).isEqualTo("schema2");
}
try (Connection conn = pool.getConnection()) {
Statement statement = conn.createStatement();
ResultSet rs = statement.executeQuery("SELECT name FROM test");
assertThat(rs.next()).isTrue();
assertThat(rs.getString("name")).isEqualTo("default schema");
}
}
}
Loading