|
[
Permlink
| « Hide
]
Andrew McIntyre added a comment - 03/Jun/05 02:47 AM
Moving fix in version to 10.2.0.0.
Changing Fix Version to Unknown as I do not plan to fix it for 10.2. The workaround for users who cannot connect to recover a prepared XA transaction because of the lock timeout would be to connect with a different user so that the default schema is not in the middle of being created.
Uploading a JUnit test repro for this issue, which, when the bug
is fixed can be added as a new regression test. Uploading an experimental patch which makes the default schema
creation happen in a nested subtransaction which commits when done, so the lock on SYSSCHEMAS can be released immediately. This means that the Derby behaviour would change slightly, in that if the user transaction rolls back, the schema creation would stay, whereas presently it would be rolled back with the rest of the user transaction. It makes the repro test (also part of the patch) pass, but there may be other good reasons why this is not a good approach, so the patch is for review only at this point. Running regressions now. Only one test broke in the regression run, lang/lockTable.sql.
This is because this test runs with table level locking, and the first patch got a deadlock when the nested transaction for auto-creating a schema tried to get a write lock to SYSSCHEMAS presumably because the outer transaction already had a table read lock on SYSSCHEMAS from the first attempt (which failed) to read the schema descriptor. Patch derby-48-2 only tries to perform the schema creation in a subtransaction if row level locking is used. Not sure I like checking the property here, but I did not find any API for asking the database about locking level. lang/lockTable.sq ran OK with the new version of the patch. I think it sounds like a good idea to do the implicit schema creation in a subtransaction. A couple of questions, though:
1. Should we try to handle self-deadlocks someway? I think the lock timeout produced by the ij script below is a self-deadlock: ij> connect 'jdbc:derby:db;user=kah'; ij> autocommit off; ij> set isolation rr; 0 rows inserted/updated/deleted ij> select count(*) from sys.sysschemas; 1 ----------- 11 1 row selected ij> create table t (x int); ERROR 40XL1: A lock could not be obtained within the time requested ij> 2. In which situations can startNestedUserTransaction() fail? Would it be better to expose those errors if they happen instead of silently ignoring them as in the code below (from DDLConstantAction.getSchemaDescriptorForCreate()): + try { + nestedTc = tc.startNestedUserTransaction(false); + } catch (StandardException e) { + } Thanks for looking at this patch, Knut. I agree it is a good idea to
fall back on the outer transaction in case of a timeout. This will also handle the self-lock case more generally than the testing of page vs row level locking in previous version of the patch, although at the expense of a wait. But such wait scenarios are much less rare than the one the issue addresses, I think. I am not sure if there is a normal use case for the failure of creation of a nested transaction here, so I added code to throw in sane builds for now. I also added another test case for the self-lock fallback case. Running regressions over again. Hi Dag,
The approach in the latest patch looks elegant. A couple of small comments and questions: - The try/catch around executeConstantAction() swallows all non-timeout exceptions. I think we need to move "else throw se" from the inner if to the outer if in the catch block. - I think the code below is fine, but it would be clearer if the comment also said: "In non-debug builds, we retry in the parent transaction before giving up." + } catch (StandardException e) { + if (SanityManager.DEBUG) { + // Should not happen + throw e; + } + } - Not that it makes any difference in practice, but... When we call commit() and destroy() on the nested transaction in the catch block, wouldn't it be more natural to call abort()+destroy(), since we don't want any side effects of the work performed by the nested transaction to be persisted? The javadoc for TransactionController.destroy() says that it will also abort the transaction, so I guess a call to destroy() would suffice. - When we check for SQLState.LOCK_TIMEOUT, should we also check for SQLState.LOCK_TIMEOUT_LOG, which is the message id used when lock tracing is enabled? And should SQLState.DEADLOCK be handled the same way? Thanks for catching the fall-through case, Knut. The new patch,
derby-48-4, fixes this plus omits the commit for the failed cases, destroy is sufficient I agree. This further simplifies the code, since destroy can't throw. This removed the need for the extra comment, too. I considered the cases of LOCK_TIMEOUT_LOG and DEADLOCK: I get the former if I enable derby.locks.deadlockTrace=true. I think it may be best to show the the self.lock if the user has enabled this property so she may take steps to avoid it (it only shows on derby.log if we throw it, it seems). I added a testcase, testDerby48SelfLockingRecoveryDeadlockDetectionOn to test this. As for deadlock, I think this will only happen if another transaction is involved, and in that case, trying again in the outer transaction would only deadlock again (usually), so my current thinking is that it may be best to let that one throw? Re-running regressions. Thanks Dag. Some more comments:
- I can't find the test case in the patch. Did you forget svn add? - I agree that we probably shouldn't check for LOCK_TIMEOUT_LOG and DEADLOCK (Although I'm not sure the statement that retrying in the outer transaction usually causes a new deadlock is correct, since aborting the nested transaction probably would allow the other transactions involved in the deadlock to continue.) - In the code below, if we move destroy() out of the try block (which we could and perhaps should?), it becomes obvious that the potentially swallowed exception is an error in commit. This I think would mean that we haven't actually created the schema, and it feels a bit strange to ignore that exception, even in insane builds. I think it would make sense to revert this back to how it was in patch 1 and 2, where exceptions from nestedTc.commit() were not swallowed. + try { + nestedTc.commit(); + nestedTc.destroy(); + } catch (StandardException e) { + if (SanityManager.DEBUG) { + // Should not happen + throw e; + } + } Oops. the test was left out of patch #3, reattaching, plus
a) changed the wording on the rationale for not catching the dead-lock to "it may be better to expose it" b) removed the try-catch around the final commit in the nested tr case, you are right. The regressions passed, so I'll commit this version if there are no further comments. The last patch looks fine.
Some small nits in the test, but they shouldn't stop you from committing. - In a comment near the end of testDerby48testNewSchemaHang: s/shoudl/should - In testDerby48testNewSchemaHang: + } catch (SQLException e) { + if (e.getSQLState().equals(LOCK_TIMEOUT)) { + c1.rollback(); + c1.close(); + fail(" + } else { + throw e; + } Why not just let the SQLException be thrown out of the test method regardless of the SQL state? As it is now, we'll lose the stack trace if it's a lock timeout. - The last part of testDerby48testNewSchemaHang tests that the implicitly created schema is still around after a rollback. It's maybe OK to test it, but it's not actually testing whether or not Derby does the right thing, it only canonizes the current behaviour. But I guess that's what many of our tests do anyway... - Is it intentional that derby.locks.waitTimeout is set to 1 both in setUp() and in testDerby48SelfLockingRecoveryDeadlockDetectionOn()? - waitTimeout is not reset, as far as I can see. I think it would be better to wrap the test with DatabasePropertyTestSetup.setLockTimeouts() which automatically does the right thing for you. DatabasePropertyTestSetup would probably also be the most appropriate way to set reset the deadlockTrace property. - In tearDown() we have this code: + try { + createStatement().executeUpdate("drop schema newuser restrict"); + } catch (SQLException e) { + } Perhaps you could add a comment about why the exception is swallowed. Is it because the schema in some cases does not exist? In that case, perhaps we should have an assertSQLState() to prevent unexpected exceptions from being ignored? - Is engine the right package for the test? We are also testing the client, it seems. Uploading a new version, #6, which:Uploading a new version, #6, which:
- Fixes the typo - Kept the explicit fail because I would like to see the JIRA number show if we get a regressions here. I'd like to see a fail which lets us attach an exception. - Changed the setup for timeouts to use the decorators you suggested, thanks, nicer. - Added an assertSQLState(42Y07) - schema does not exist - for the drop schema case. - As far as the package, I was not sure where to put it, but since you asked, moved it to lang; it is perhaps more appropriate. Thanks for the new patch, Dag! It looks very good, so please go ahead and commit it.
> - Kept the explicit fail because I would like to see the JIRA number show > if we get a regressions here. I'd like to see a fail which lets us > attach an exception. You could do a throw new Exception(" Right, but I'd like a JUnit Failure, not an Error.. Oh well.. I think we are OK with this one, either way
we will know what it is if we see it :) Thanks for your diligent help on this one, Knut! Ah, this stanza does the trick:
AssertionFailedError ae = new AssertionFailedError(" ae.initCause(e); throw ae; This (#7) is the same as #6, except it adds a static method fail(String, Exception) to BaseTestCase and uses that for the case discussed above, plus makes a constant out of 40XL2; all in LazyDefaultSchemaCreationTest.
Committed derby-48-7 as svn 662446. This this patch changes the behavior, I will add
a release note. Attaching a release note for this issue.
Should this fix be ported to the 10.4 branch for the upcoming 10.4.2 maintenance release?
I was (still am) hesitant to backport it since it does change behavior
slightly, cf. the release note. If people think it's ok and important enough, I can backport it. I can't tell from the release note exactly what the incompatible change is so it's hard to comment on if it should be backported.
E.g.: - The summary "This is no longer the case", it doesn't seem clear what 'This' refers to, should it be part of the previous sentence, e.g. "as well, this is ..."? Maybe the summary could be re-written to describe what does happen, rather than focusing on the previous behaviour? - In "Symptoms Seen ..." section it says the schema will be created earlier than it would have before, is this true, from the summary it seems it is created at the same time as previously? I.e. the only change is if the transaction is rolled back, not when the schema is created? - The "Rationale for change" is more describing what is implemented, rather than the justification for changing it. Maybe it could be more along the lines of: "Implicit schema creation is now performed in its own transaction to avoid deadlocks with other connections accessing the same schema" Is this change in behaviour for all implicit schema creations or just for the default schema, it was hard to tell from the comments in this entry? That might change the release note as well. Thanks for your comments, Dan. Uploading a new version of the release
notes which are hopefully clearer. To your specific questions: - The implicit creation happens at the same time as before, the only change is what happens if the user transaction rolls back. - The change is for the default schema only. Apart from the system schemas, I am not aware of other schemas created implicitly. I see Derby implicitly creates schemas in cases I did not realize,
e.g. CREATE TABLE S.T(..) will create S implicitly if it does not exist. I see from the code that the patch change applies for such cases as well, so the release note would need to reflect that as well as the change stands. But is it acceptable to let such implicitly created schemas not be rolled back? I will have a look at see what SQL says about this, if anything. If it is not acceptable, I will reopen this issue and see if I can limit the change to just the default schema. If it is acceptable to let this change impact all implicit schema creation, I will update the release notes. It seems the SQL standard does not allow implicit
schema creation as in CREATE TABLE S.T(...), but Derby does. I am starting to doubt whether the solution chosen in this patch is good. If we want our DDL to be transactional, always creating implicit schemas in separate transactions seems to defeat the transactionality.. I can see four possibilities: 1) let the old behavior remain (essentially: "this is not a bug") and roll back this patch 2) rework the patch to apply only for the initial default schema Other implicit schema creations will happen in the user transaction as before. 3) drop the lazy schema creation, i.e. always create a user schema as soon as a user (which has write access) connects 4) let the patch remain as is, impacting all implicit schema creation and rewrite the release notes. What do you think? Reopening until we resolve my concerns.
There's one more option as described in the original description which would fix the bug and not change behaviour.
5) Allow the connection request to succeed as though the schema does not exist. I.e. the connection succeeds and is in the default schema but does not wait for the real schema creation by the other thread. Thanks, Dan. I had more or less discounted that solution; I think
because I didn't see a way of getting the schema descriptor without risking waiting for a lock (would need something similar to C_LockFactory.NO_WAIT?), but I am sure we could go that route too. I will have a look. Meanwhile, I enclose derby-48b-1 which implements solution 2) and adds an extra test case to verify that other implicit schema creations (besides the initial default schema) are still transactional. Regression tests passed. Attaching a new rev of the release notes, using the
term *initial default schema* to avoid ambiguity. Scrubbed the release note. The angle brackets around <default schema name> were choking the ReleaseNoteReader.
Hi Dag,
The derby-48b-1 patch looks like it is doing the right thing and I have hand-verified that the transactional behavior is as you describe. The release note looks good now. It's hard not to regret the old decision to violate the ANSI rules and create schemas implicitly on the fly. I wonder whether we are just exchanging one edge case for another. With this patch, there is now a path by which transactional work can be committed on behalf of your connection even though you explicitly rollback your transaction. I'm afraid I'm not smart enough to know whether this edge case is already violating some assumptions made elsewhere in the code. At the very least, it will require us to put another post-it on our brains next to the note about implicit schema creation. Thanks for looking at this Rick! I agree, I don't quite like this solution either.
It improves on the previous patch, so I will commit it, but leave the issue open and see if I can make solution 5) work, cf Dan's comment. Committed derby-48b-1 as svn 685141 + svn 685232 (two steps because 685141 wasn't quite the right version).
Uploading derby-48-10_4-1, which I propose to commit to the 10.4 branch.
It is the sum of the two trunk patches, but I had to tailor them a bit, hence this separate patch. Running regressions now. Committed derby-48-10_4-1 on 10.4 branch as svn 687155.
Marking this issue as Resolved so that it will appear in the filters used to generate the 10.4.2 release notes.
Changing the fix version from 10.4.1.4 to 10.4.2.0 so that this issue will appear in the filters used by the 10.4.2 release notes.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
A connection request that has a default schema that is being created by another transaction will fail to connect
