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

Feature/change schema reset review #106

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Feature/change schema reset review #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b427700
Fix: Missing catalog initialization
rPraml Nov 5, 2024
921b428
make suggestion with state variables
rPraml Nov 5, 2024
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
3 changes: 3 additions & 0 deletions ebean-datasource/src/main/java/io/ebean/datasource/pool/ConnectionPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,9 @@ private Connection initConnection(Connection conn) throws SQLException {
if (readOnly) {
conn.setReadOnly(true);
}
if (catalog != null) {
conn.setCatalog(catalog);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a severe bug, as this is currently missing and does not initialize the catalog correctly on first use

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch.

if (schema != null) {
conn.setSchema(schema);
}
Expand Down
98 changes: 71 additions & 27 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 @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried the state thing, as it may make the whole change more understandable (for me). But I also do not have a problem with booleans.

Copy link
Member

Choose a reason for hiding this comment

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

To me this reads better than the 2 booleans as well. Nice!!


private final String name;
private final ConnectionPool pool;
private final Connection connection;
Expand All @@ -82,12 +108,14 @@ final class PooledConnection extends ConnectionDelegator {
*/
private boolean failoverToReadOnly;
private boolean resetAutoCommit;
private boolean resetSchema;
private boolean resetCatalog;
private boolean initialisedSchema;
private boolean initialisedCatalog;
private String currentSchema;
private String currentCatalog;
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed currentSchema to cacheKeySchema, because it clarifies, for which purpose this value is used.


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

Expand Down Expand Up @@ -122,10 +150,17 @@ final class PooledConnection extends ConnectionDelegator {
this.name = pool.name() + uniqueId;
this.originalSchema = pool.schema();
this.originalCatalog = pool.catalog();
this.initialisedSchema = originalSchema != null;
this.initialisedCatalog = originalCatalog != null;
this.currentSchema = originalSchema != null ? originalSchema : "@default";
this.currentCatalog = originalCatalog != null ? originalCatalog : "@default";
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;
}
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some asserts here. Normally, they should apply, but I'm unsure, if we should keep them in production, as some tests are failing, but it helped me to discover the servere bug:

for some DBMS you will not get back the same string; e.g. lower/upper, DB2 may even pad with spaces. (did not check this in detail, I only remember, that it is stored in the catalog as SCHEMA1 with some additional spaces at the end)

postgres is not roundtrip aware. If you set schema that does not exist, you will get null

pgConnection.setSchema("public");
pgConnection.getSchema(); // returns public
pgConnection.setSchema("doesnotexist");
pgConnection.getSchema(); // returns null

Copy link
Member

Choose a reason for hiding this comment

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

I think we remove the asserts as they have potential for side effects.

}
this.pstmtCache = new PstmtCache(pool.pstmtCacheSize());
this.maxStackTrace = pool.maxStackTraceSize();
this.creationTime = System.currentTimeMillis();
Expand Down Expand Up @@ -287,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 @@ -296,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 @@ -350,12 +385,21 @@ public PreparedStatement prepareStatement(String sql, int resultSetType, int res
* Reset the connection for returning to the client. Resets the status,
* startUseTime and hadErrors.
*/
void resetForUse() {
void resetForUse() throws SQLException {
this.status = STATUS_ACTIVE;
this.startUseTime = System.currentTimeMillis();
this.createdByMethod = null;
this.lastStatement = null;
this.hadErrors = false;
// CHECKME: Shoud we keep the asserts here or should we even reset schema/catalog here
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we rely that a connection is successfully reset when it was put back to the pool, or should we play it safe here and enforce the correct schema/catalog here

Copy link
Member

Choose a reason for hiding this comment

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

Can we rely that a connection is successfully reset when ...

Yes. That is in a try catch and that can fail, and if it fails its force closed and so doesn't actually make it back into the pool.

So yes we need to do the schema reset etc when returning the connection because that can fail and a "force close" on the connection (so the connection effectively does not make it back into the pool).

enforce the correct schema/catalog here

I'm saying that is not preferred as we have already obtained this connection from the pool at this point and are just about to give it to the application. Needing to do things here "on borrow" is problematic and we really want to do those things "on return".

In this sense I think we remove these asserts (we don't want any side effects here).

assert schemaState != SCHEMA_CATALOG_CHANGED : "connection is in the wrong state (not properly closed)";
if (schemaState == SCHEMA_CATALOG_KNOWN) {
assert originalSchema.equals(getSchema()) : "connection is in the wrong schema: " + getSchema() + ", expected: " + originalSchema;
}
assert catalogState != SCHEMA_CATALOG_CHANGED : "connection is in the wrong state (not properly closed)";
if (catalogState == SCHEMA_CATALOG_KNOWN) {
assert originalCatalog.equals(getCatalog()) : "connection is in the wrong catalog: " + getCatalog() + ", expected: " + originalCatalog;
}
}

/**
Expand Down Expand Up @@ -426,16 +470,17 @@ public void close() throws SQLException {
resetIsolationReadOnlyRequired = false;
}

if (resetSchema) {
if (schemaState == SCHEMA_CATALOG_CHANGED) {
connection.setSchema(originalSchema);
currentSchema = null;
resetSchema = false;
// we can use original value for cache computation from now on
cacheKeySchema = originalSchema;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible bug: We should not set currentSchema respectively cacheKey to null

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good !!

schemaState = SCHEMA_CATALOG_KNOWN;
}

if (resetCatalog) {
if (catalogState == SCHEMA_CATALOG_CHANGED) {
connection.setCatalog(originalCatalog);
currentCatalog = null;
resetCatalog = false;
cacheKeyCatalog = originalCatalog;
catalogState = SCHEMA_CATALOG_KNOWN;
}

// the connection is assumed GOOD so put it back in the pool
Expand Down Expand Up @@ -706,13 +751,13 @@ public void setSchema(String schema) throws SQLException {
if (status == STATUS_IDLE) {
throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "setSchema()");
}
if (!initialisedSchema) {
if (schemaState == SCHEMA_CATALOG_UNKNOWN) {
// lazily initialise the originalSchema
originalSchema = getSchema();
initialisedSchema = true;
// state would be KNOWN here
}
currentSchema = schema;
resetSchema = true;
schemaState = SCHEMA_CATALOG_CHANGED;
cacheKeySchema = schema;
connection.setSchema(schema);
}

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

Expand Down
Loading