Feature/change schema reset review #106
Conversation
| @@ -398,6 +398,9 @@ private Connection initConnection(Connection conn) throws SQLException { | |||
| if (readOnly) { | |||
| conn.setReadOnly(true); | |||
| } | |||
| if (catalog != null) { | |||
| conn.setCatalog(catalog); | |||
| } | |||
There was a problem hiding this comment.
This is a severe bug, as this is currently missing and does not initialize the catalog correctly on first use
| * 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To me this reads better than the 2 booleans as well. Nice!!
|
|
||
| // this is used for cache computation | ||
| private String cacheKeySchema = UNKNOWN; | ||
| private String cacheKeyCatalog = UNKNOWN; |
There was a problem hiding this comment.
I have renamed currentSchema to cacheKeySchema, because it clarifies, for which purpose this value is used.
| 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; |
There was a problem hiding this comment.
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 nullThere was a problem hiding this comment.
I think we remove the asserts as they have potential for side effects.
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
| currentSchema = null; | ||
| resetSchema = false; | ||
| // we can use original value for cache computation from now on | ||
| cacheKeySchema = originalSchema; |
There was a problem hiding this comment.
Possible bug: We should not set currentSchema respectively cacheKey to null
| * 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; |
There was a problem hiding this comment.
To me this reads better than the 2 booleans as well. Nice!!
| 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; |
There was a problem hiding this comment.
I think we remove the asserts as they have potential for side effects.
| 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 |
There was a problem hiding this comment.
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).
| @@ -398,6 +398,9 @@ private Connection initConnection(Connection conn) throws SQLException { | |||
| if (readOnly) { | |||
| conn.setReadOnly(true); | |||
| } | |||
| if (catalog != null) { | |||
| conn.setCatalog(catalog); | |||
| } | |||
| currentSchema = null; | ||
| resetSchema = false; | ||
| // we can use original value for cache computation from now on | ||
| cacheKeySchema = originalSchema; |
rbygrave
merged commit b053076
into
ebean-orm:feature/change-schema-reset
Hello Rob. I've reviewed your changes. I will write down my thoughts in this PR
This PR can be closed, if #105 is merged