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.

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

Use pool schema/catalog for original values, lazy initialise originalSchema if required #105

Merged
merged 9 commits into from
Nov 7, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,11 @@ default DataSourceBuilder schema(String schema) {
return setSchema(schema);
}

/**
* Set the default database catalog to use.
*/
DataSourceBuilder catalog(String catalog);

/**
* @deprecated - migrate to {@link #driver(String)}.
*/
Expand Down Expand Up @@ -812,6 +817,11 @@ interface Settings extends DataSourceBuilder {
*/
String getSchema();

/**
* Return the database catalog.
*/
String catalog();

/**
* Return the driver instance to use.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class DataSourceConfig implements DataSourceBuilder.Settings {
private String password;
private String password2;
private String schema;
private String catalog;
private Driver driver;
private Class<? extends Driver> driverClass;
private String driverClassName;
Expand Down Expand Up @@ -103,6 +104,7 @@ public DataSourceConfig copy() {
copy.password = password;
copy.password2 = password2;
copy.schema = schema;
copy.catalog = catalog;
copy.platform = platform;
copy.ownerUsername = ownerUsername;
copy.ownerPassword = ownerPassword;
Expand Down Expand Up @@ -171,6 +173,9 @@ public DataSourceConfig setDefaults(DataSourceBuilder builder) {
if (schema == null) {
schema = other.getSchema();
}
if (catalog == null) {
catalog = other.catalog();
}
if (minConnections == 2 && other.getMinConnections() < 2) {
minConnections = other.getMinConnections();
}
Expand Down Expand Up @@ -307,6 +312,17 @@ public DataSourceConfig setSchema(String schema) {
return this;
}

@Override
public String catalog() {
return catalog;
}

@Override
public DataSourceConfig catalog(String catalog) {
this.catalog = catalog;
return this;
}

@Override
public String getDriver() {
return driverClassName;
Expand Down Expand Up @@ -740,6 +756,7 @@ private void loadSettings(ConfigPropertiesHelper properties) {
password = properties.get("password", password);
password2 = properties.get("password2", password2);
schema = properties.get("schema", schema);
catalog = properties.get("catalog", catalog);
platform = properties.get("platform", platform);
ownerUsername = properties.get("ownerUsername", ownerUsername);
ownerPassword = properties.get("ownerPassword", ownerPassword);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public void copy() {
source.setUrl("url");
source.setReadOnlyUrl("readOnlyUrl");
source.setSchema("sch");
source.catalog("cat");

Map<String,String> customSource = new LinkedHashMap<>();
customSource.put("a","a");
Expand All @@ -90,6 +91,7 @@ public void copy() {
assertEquals("url", copy.getUrl());
assertEquals("readOnlyUrl", copy.getReadOnlyUrl());
assertEquals("sch", copy.getSchema());
assertEquals("cat", copy.catalog());
assertEquals(42, copy.getMinConnections());
assertEquals(45, copy.getMaxConnections());

Expand All @@ -113,6 +115,7 @@ public void defaults() {
assertThat(readOnly.getUsername()).isEqualTo(config.getUsername());
assertThat(readOnly.getPassword()).isEqualTo(config.getPassword());
assertThat(readOnly.getSchema()).isEqualTo(config.getSchema());
assertThat(readOnly.catalog()).isEqualTo(config.catalog());
assertThat(readOnly.getMinConnections()).isEqualTo(config.getMinConnections());
assertThat(readOnly.getCustomProperties()).containsKeys("useSSL");
}
Expand Down Expand Up @@ -258,6 +261,7 @@ private static void assertConfigValues(DataSourceBuilder.Settings config) {
assertThat(config.getUsername()).isEqualTo("myusername");
assertThat(config.getPassword()).isEqualTo("mypassword");
assertThat(config.getSchema()).isEqualTo("myschema");
assertThat(config.catalog()).isEqualTo("mycat");
assertThat(config.getApplicationName()).isEqualTo("myApp");
Properties clientInfo = config.getClientInfo();
assertThat(clientInfo.getProperty("ClientUser")).isEqualTo("ciu");
Expand Down
1 change: 1 addition & 0 deletions ebean-datasource-api/src/test/resources/example.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
datasource.foo.username=myusername
datasource.foo.password=mypassword
datasource.foo.schema=myschema
datasource.foo.catalog=mycat
datasource.foo.url=myUrl
datasource.foo.readOnlyUrl=myReadOnlyUrl
datasource.foo.applicationName=myApp
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
bar.username=myusername
bar.password=mypassword
bar.schema=myschema
bar.catalog=mycat
bar.url=myUrl
bar.readOnlyUrl=myReadOnlyUrl
bar.applicationName=myApp
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
username=myusername
password=mypassword
schema=myschema
catalog=mycat
url=myUrl
readOnlyUrl=myReadOnlyUrl
applicationName=myApp
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ final class ConnectionPool implements DataSourcePool {
private final List<String> initSql;
private final String user;
private final String schema;
private final String catalog;
private final String heartbeatSql;
private final int heartbeatFreqSecs;
private final int heartbeatTimeoutSeconds;
Expand Down Expand Up @@ -119,6 +120,7 @@ final class ConnectionPool implements DataSourcePool {
this.clientInfo = params.getClientInfo();
this.queue = new PooledConnectionQueue(this);
this.schema = params.getSchema();
this.catalog = params.catalog();
this.user = params.getUsername();
this.shutdownOnJvmExit = params.isShutdownOnJvmExit();
this.source = DriverDataSource.of(name, params);
Expand Down Expand Up @@ -251,6 +253,14 @@ public int size() {
return size.get();
}

String schema() {
return schema;
}

String catalog() {
return catalog;
}

/**
* Increment the current pool size.
*/
Expand Down Expand Up @@ -388,6 +398,9 @@ private Connection initConnection(Connection conn) throws SQLException {
if (readOnly) {
conn.setReadOnly(true);
}
if (catalog != null) {
conn.setCatalog(catalog);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Every day I think of something new. 🤣 Do you think the order is important? I could imagine that you have to change the catalog first before you can change the schema, because the schema does not exist in the current catalog, for example. We did it right here. But not in close(). From my gut feeling, I would say change the catalog first, then the schema

if (schema != null) {
conn.setSchema(schema);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,32 @@ final class PooledConnection extends ConnectionDelegator {

private static final int RO_MYSQL_1290 = 1290;

/**
* Constant for schema/catalog, when we are in SCHEMA_CATALOG_UNKNOWN state
* This is used for correct cache key computation. (We cannot use 'null'
* here, as we might get a collision in edge cases)
*/
private static final String UNKNOWN = "@unknown";

/**
* The schema/catalog is unknown, that means, we have not yet touched the
* value on the underlying connection.
* We do not have to restore it.
*/
private static final int SCHEMA_CATALOG_UNKNOWN = 0;

/**
* The schema/catalog is changed. The original value has to be restored on
* close()
*/
private static final int SCHEMA_CATALOG_CHANGED = 1;

/**
* We know the original value of the underlying connection, but there is no
* demand to restore it.
*/
private static final int SCHEMA_CATALOG_KNOWN = 2;

private final String name;
private final ConnectionPool pool;
private final Connection connection;
Expand All @@ -82,12 +108,16 @@ 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 int schemaState = SCHEMA_CATALOG_UNKNOWN;
private int catalogState = SCHEMA_CATALOG_UNKNOWN;

// this is used for cache computation
private String cacheKeySchema = UNKNOWN;
private String cacheKeyCatalog = UNKNOWN;

// original values are lazily initialized and restored on close()
private String originalSchema;
private String originalCatalog;

private long startUseTime;
private long lastUseTime;
Expand Down Expand Up @@ -118,12 +148,23 @@ final class PooledConnection extends ConnectionDelegator {
this.pool = pool;
this.connection = connection;
this.name = pool.name() + uniqueId;
this.originalSchema = pool.schema();
this.originalCatalog = pool.catalog();
if (originalSchema != null) {
schemaState = SCHEMA_CATALOG_KNOWN;
this.cacheKeySchema = originalSchema;
// if schema & catalog is defined, we can be sure, that connection is initialized properly
assert originalSchema.equals(connection.getSchema()) : "Connection is in the wrong schema: " + connection.getSchema() + ", expected: " + originalSchema;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about these asserts? On the one hand, I think they are good because you can see, for example, if you specify a non-existent schema in the PoolConfiguration (at least pg returns null in that case). On the other hand, they could cause problems if the schema is returned in uppercase, for example.
They definitely help to find potential programming errors.

I would keep them in until someone provides a use case. (and the user can disable assertions anyway)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about these asserts?

I've honestly changed my mind a couple of times on these asserts. On one hand they look useful, but on the other they can throw SQLException and fail on case but only with asserts enabled, and if the driver allows setting unknown schema etc maybe its best to just allow that.

So I'm thinking of removing them. That really means that we are leaving it up to developers to set original schema and catalog appropriately and to test that themselves as needed.

}
if (originalCatalog != null) {
catalogState = SCHEMA_CATALOG_KNOWN;
this.cacheKeyCatalog = originalCatalog;
assert originalCatalog.equals(connection.getCatalog()) : "Connection is in the wrong catalog: " + connection.getCatalog() + ", expected: " + originalCatalog;
}
this.pstmtCache = new PstmtCache(pool.pstmtCacheSize());
this.maxStackTrace = pool.maxStackTraceSize();
this.creationTime = System.currentTimeMillis();
this.lastUseTime = creationTime;
this.currentSchema = this.originalSchema = connection.getSchema();
this.currentCatalog = this.originalCatalog = connection.getCatalog();
pool.inc();
}

Expand All @@ -139,8 +180,6 @@ 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 @@ -283,7 +322,7 @@ void returnPreparedStatement(ExtendedPreparedStatement pstmt) {
*/
@Override
public PreparedStatement prepareStatement(String sql, int returnKeysFlag) throws SQLException {
String key = sql + ':' + currentSchema + ':' + currentCatalog + ':' + returnKeysFlag;
String key = sql + ':' + cacheKeySchema + ':' + cacheKeyCatalog + ':' + returnKeysFlag;
return prepareStatement(sql, true, returnKeysFlag, key);
}

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

Expand Down Expand Up @@ -422,14 +461,17 @@ public void close() throws SQLException {
resetIsolationReadOnlyRequired = false;
}

if (resetSchema) {
if (schemaState == SCHEMA_CATALOG_CHANGED) {
connection.setSchema(originalSchema);
resetSchema = false;
// we can use original value for cache computation from now on
cacheKeySchema = originalSchema;
schemaState = SCHEMA_CATALOG_KNOWN;
}

if (resetCatalog) {
if (catalogState == SCHEMA_CATALOG_CHANGED) {
connection.setCatalog(originalCatalog);
Copy link
Contributor

Choose a reason for hiding this comment

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

Important: We should switch the order here. First restore catalog, then restore schema. See other comment

By the way, I can imagine an edge case here again:

Postgres fails silently, if I call setSchema("doesnotexist") and getSchema() returns null

So if you set up your pool, that

  • has originalSchema = "TENANT_MASTER"
  • and a thread takes a connection from the pool and changes the schema to "TENANT_123"
  • the thread does some long running tasks...
  • in the meantime, the DBA deletes "TENANT_MASTER" (yes, a stupid idea)
  • the thread above puts back the connection to the pool
  • we try to switch back to originalSchema, but PG does not fail in this case
  • in what state is that connection? 😉
    • Still in "TEANT_123"
    • or in no man's land? (at least getSchema() would return null for Postgres)
    • is there a danger, that the next one, that pulls the connection will start in "TENANT_123"

I think we would be overdoing it here if we also covered this special case, because it is really a stupid idea to delete the main schema

Copy link
Member Author

Choose a reason for hiding this comment

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

First restore catalog, then restore schema

Yes, I'm good with that. I'll make that change.

I think we would be overdoing it here if we also covered this special case

I agree. We can leave that sort of case with developers to own.

resetCatalog = false;
cacheKeyCatalog = originalCatalog;
catalogState = SCHEMA_CATALOG_KNOWN;
}

// the connection is assumed GOOD so put it back in the pool
Expand Down Expand Up @@ -700,8 +742,13 @@ public void setSchema(String schema) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setSchema()");
}
currentSchema = schema;
resetSchema = true;
if (schemaState == SCHEMA_CATALOG_UNKNOWN) {
// lazily initialise the originalSchema
originalSchema = getSchema();
rbygrave marked this conversation as resolved.
Show resolved Hide resolved
// state would be KNOWN here
}
schemaState = SCHEMA_CATALOG_CHANGED;
cacheKeySchema = schema;
connection.setSchema(schema);
}

Expand All @@ -710,8 +757,11 @@ public void setCatalog(String catalog) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setCatalog()");
}
currentCatalog = catalog;
resetCatalog = true;
if (schemaState == SCHEMA_CATALOG_UNKNOWN) {
originalCatalog = getCatalog();
}
catalogState = SCHEMA_CATALOG_CHANGED;
cacheKeyCatalog = catalog;
connection.setCatalog(catalog);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ void test_with_clientInfo() throws SQLException {
void test_with_applicationNameAndSchema() throws SQLException {
DataSourceConfig ds = new DataSourceConfig();
ds.setUrl("jdbc:postgresql://127.0.0.1:9999/app");
ds.setSchema("fred");
ds.setSchema("public");
ds.setUsername("db_owner");
ds.setPassword("test");
ds.setApplicationName("my-application-name");
Expand Down Expand Up @@ -112,7 +112,7 @@ void test_with_applicationNameAndSchema() throws SQLException {
void test_password2() throws SQLException {
DataSourceConfig ds = new DataSourceConfig();
ds.setUrl("jdbc:postgresql://127.0.0.1:9999/app");
ds.setSchema("fred");
ds.setSchema("public");
ds.setUsername("db_owner");
ds.setPassword("test");
ds.setPassword2("newRolledPassword");
Expand Down
Loading