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
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ final class PooledConnection extends ConnectionDelegator {
private boolean resetAutoCommit;
private boolean resetSchema;
private boolean resetCatalog;
private boolean initialisedSchema;
private boolean initialisedCatalog;
private String currentSchema;
private String currentCatalog;
private final String originalSchema;
private final String originalCatalog;
private String originalSchema;
private String originalCatalog;

private long startUseTime;
private long lastUseTime;
Expand Down Expand Up @@ -118,12 +120,14 @@ final class PooledConnection extends ConnectionDelegator {
this.pool = pool;
this.connection = connection;
this.name = pool.name() + uniqueId;
this.originalSchema = pool.schema();
this.originalCatalog = pool.catalog();
this.initialisedSchema = originalSchema != null;
this.initialisedCatalog = originalCatalog != null;
rbygrave marked this conversation as resolved.
Show resolved Hide resolved
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 +143,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 @@ -424,11 +426,13 @@ public void close() throws SQLException {

if (resetSchema) {
rbygrave marked this conversation as resolved.
Show resolved Hide resolved
connection.setSchema(originalSchema);
currentSchema = null;
resetSchema = false;
}

if (resetCatalog) {
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.

currentCatalog = null;
resetCatalog = false;
}

Expand Down Expand Up @@ -700,6 +704,11 @@ public void setSchema(String schema) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setSchema()");
}
if (!initialisedSchema) {
// lazily initialise the originalSchema
originalSchema = getSchema();
rbygrave marked this conversation as resolved.
Show resolved Hide resolved
initialisedSchema = true;
}
currentSchema = schema;
resetSchema = true;
connection.setSchema(schema);
Expand All @@ -710,6 +719,10 @@ public void setCatalog(String catalog) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setCatalog()");
}
if (!initialisedCatalog) {
originalCatalog = getCatalog();
initialisedCatalog = true;
}
currentCatalog = catalog;
resetCatalog = true;
connection.setCatalog(catalog);
Expand Down
Loading