Derby
  1. Derby
  2. DERBY-5493

Same value returned by successive calls to a sequence generator.

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      Patch Available, Release Note Needed
    • Bug behavior facts:
      Deviation from standard, Wrong query result

      Description

      The following script shows the same value being returned from a sequence generator by two successive NEXT VALUE FOR calls. Thanks to Knut for finding this:

      connect 'jdbc:derby:memory:db;create=true';

      create table t (x int);
      create sequence s;
      autocommit off;
      select count from sys.syssequences with rs;
      values next value for s;
      drop table t;
      rollback;

      – same value as previous call
      values next value for s;

      1. releaseNote.html
        4 kB
        Rick Hillegas
      2. derby-5493-02-aa-boostPreallocationTo100.diff
        4 kB
        Rick Hillegas
      3. derby-5493-01-ag-mergedWith5494.diff
        40 kB
        Rick Hillegas
      4. derby-5493-01-af-usersubtran.diff
        42 kB
        Rick Hillegas
      5. derby-5493-01-ae-simplerApproachWithCrashJUnitTest.diff
        44 kB
        Rick Hillegas
      6. derby-5493-01-ad-simplerApproach.diff
        41 kB
        Rick Hillegas
      7. derby-5493-01-aa-correctnessPlusPeekerPlusTest.diff
        76 kB
        Rick Hillegas

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-aa-correctnessPlusPeekerPlusTest.diff. This patch modifies how we allocate new sequence values, in order to fix the known correctness problems with sequence generation. Regression tests pass cleanly for me, but this patch is not ready for commit. It needs additional tests to verify correctness, upgrade, and new user-visible features.

          Mike and I discussed the correctness problems on DERBY-5443. Two proposals were put forward, each of which had its own messy issues:

          1) Use an invisible conglomerate and dedicated transaction to allocate new sequence ranges. This is the approach taken by this patch.

          2) Restrict the isolation level used to read from SYSSEQUENCES.

          In that discussion, two problems with approach (1) were identified:

          i) It creates a new file (the invisible conglomerate). I think that the space occupied by this new file is very small compared to the size of an empty Derby database and well within the growth we have tolerated for Derby feature releases over the last 7 years.

          ii) Orphaned tuples can pile up in the invisible conglomerate after successful DROP SEQUENCE and unsuccessful CREATE SEQUENCE statements. I addressed this problem by garbage-collecting the orphans at database boot time.

          In addition to fixing the known correctness problem, this patch introduces the following user-visible changes:

          A) A new system function has been added: syscs_peek_at_sequence(). This function gives the application the instantaneous current value of the sequence. In previous releases, users tried to get this information by querying SYSSEQUENCES.CURRENTVALUE. But that didn't work because that column holds the end of the pre-allocation range and not the actual next value in the sequence.

          B) SYSCONGLOMERATES.TABLEID is now nullable.

          C) A new SYSGHOST conglomerate is listed in SYSCONGLOMERATES. The SYSGHOST conglomerate does not belong to any corresponding table. Although users can't see it, this is the shape of a SYSGHOST tuple:

          ( keycol varchar( 32672 ), payload Formatable )

          In addition, this patch introduces a testing/diagnostic feature which we should not document:

          D) A new GhostTable VTI has been added. This lets you view the contents of SYSGHOST. The VTI does all of its work in the transaction controller that is dedicated to managing SYSGHOST. Here's how you invoke it:

          select * from new org.apache.derby.diag.GhostTable() vti;

          Behind the scenes, this patch introduces some other new objects:

          E) GhostController, a synchronized object for reading/writing SYSGHOST tuples.

          F) A new Formatable to hold the end of a pre-allocation range: SequenceState.

          G) A new sequence updater for use on databases at level 10.9 or higher: SyssequenceUpdater_10_9.

          Most of the complexity of the patch is in the implementation of GhostController. Extra support code was added to DataDictionaryImpl and SyssequenceUpdater_10_9, but I tried to isolate most of the trickiness in GhostControllerImpl.

          This patch will require some changes to the Reference Manual:

          DOC-1) Add a section describing the new syscs_peek_at_sequence() function.

          DOC-2) Modify the section on SYSCONGLOMERATES to state that TABLEID is nullable.

          DOC-3) Modify the section on SYSSEQUENCES to state that users should not bother querying the CURRENTVALUE column. Instead, they should use syscs_peek_at_sequence() to peek at the instantaneous current value of a sequence generator.

          This patch will require a release note explaining that users should use syscs_peek_at_sequence() rather than SYSSEQUENCES.CURRENTVALUE.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java
          M java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java
          A java/engine/org/apache/derby/iapi/sql/dictionary/SequenceState.java

          New Formatable to hold the end of pre-allocation ranges.

          --------------

          M java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java
          A java/engine/org/apache/derby/iapi/sql/dictionary/GhostDescriptor.java

          New tuple describing a row in SYSGHOST.

          --------------

          M java/engine/org/apache/derby/iapi/sql/dictionary/CatalogRowFactory.java
          M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java
          M java/engine/org/apache/derby/impl/sql/catalog/SYSCONGLOMERATESRowFactory.java
          M java/engine/org/apache/derby/impl/sql/catalog/DD_Version.java

          Support for creating SYSGHOST and deleting orphans.

          --------------

          M java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java
          M java/storeless/org/apache/derby/impl/storeless/EmptyDictionary.java
          A java/engine/org/apache/derby/iapi/sql/dictionary/GhostController.java
          A java/engine/org/apache/derby/impl/sql/catalog/GhostControllerImpl.java

          Logic to manage SYSGHOST.

          --------------

          M java/engine/org/apache/derby/catalog/SystemProcedures.java

          Logic for new syscs_peek_at_sequence() procedure.

          --------------

          A java/engine/org/apache/derby/diag/GhostTable.java
          M tools/jar/extraDBMSclasses.properties

          New diagnostic VTI for viewing SYSGHOST.

          --------------

          M java/engine/org/apache/derby/impl/sql/catalog/SequenceUpdater.java

          New sequence updater for use on databases at level 10.9 and higher.

          --------------

          M java/engine/org/apache/derby/impl/sql/execute/CreateSequenceConstantAction.java

          Add a corresponding SYSGHOST tuple when creating a sequence. If the create action is rolled back, then the SYSGHOST tuple will be garbage-collected the next time the database boots.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java

          Slight change to use GhostTable rather than SYSSEQUENCES.CURRENTVALUE in order to view the end of pre-allocation ranges.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SystemCatalogTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestDbMetaData.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_2.java
          M java/testing/org/apache/derbyTesting/functionTests/master/ij7.out

          Test changes to account for the metadata changes.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-aa-correctnessPlusPeekerPlusTest.diff. This patch modifies how we allocate new sequence values, in order to fix the known correctness problems with sequence generation. Regression tests pass cleanly for me, but this patch is not ready for commit. It needs additional tests to verify correctness, upgrade, and new user-visible features. Mike and I discussed the correctness problems on DERBY-5443 . Two proposals were put forward, each of which had its own messy issues: 1) Use an invisible conglomerate and dedicated transaction to allocate new sequence ranges. This is the approach taken by this patch. 2) Restrict the isolation level used to read from SYSSEQUENCES. In that discussion, two problems with approach (1) were identified: i) It creates a new file (the invisible conglomerate). I think that the space occupied by this new file is very small compared to the size of an empty Derby database and well within the growth we have tolerated for Derby feature releases over the last 7 years. ii) Orphaned tuples can pile up in the invisible conglomerate after successful DROP SEQUENCE and unsuccessful CREATE SEQUENCE statements. I addressed this problem by garbage-collecting the orphans at database boot time. In addition to fixing the known correctness problem, this patch introduces the following user-visible changes: A) A new system function has been added: syscs_peek_at_sequence(). This function gives the application the instantaneous current value of the sequence. In previous releases, users tried to get this information by querying SYSSEQUENCES.CURRENTVALUE. But that didn't work because that column holds the end of the pre-allocation range and not the actual next value in the sequence. B) SYSCONGLOMERATES.TABLEID is now nullable. C) A new SYSGHOST conglomerate is listed in SYSCONGLOMERATES. The SYSGHOST conglomerate does not belong to any corresponding table. Although users can't see it, this is the shape of a SYSGHOST tuple: ( keycol varchar( 32672 ), payload Formatable ) In addition, this patch introduces a testing/diagnostic feature which we should not document: D) A new GhostTable VTI has been added. This lets you view the contents of SYSGHOST. The VTI does all of its work in the transaction controller that is dedicated to managing SYSGHOST. Here's how you invoke it: select * from new org.apache.derby.diag.GhostTable() vti; Behind the scenes, this patch introduces some other new objects: E) GhostController, a synchronized object for reading/writing SYSGHOST tuples. F) A new Formatable to hold the end of a pre-allocation range: SequenceState. G) A new sequence updater for use on databases at level 10.9 or higher: SyssequenceUpdater_10_9. Most of the complexity of the patch is in the implementation of GhostController. Extra support code was added to DataDictionaryImpl and SyssequenceUpdater_10_9, but I tried to isolate most of the trickiness in GhostControllerImpl. This patch will require some changes to the Reference Manual: DOC-1) Add a section describing the new syscs_peek_at_sequence() function. DOC-2) Modify the section on SYSCONGLOMERATES to state that TABLEID is nullable. DOC-3) Modify the section on SYSSEQUENCES to state that users should not bother querying the CURRENTVALUE column. Instead, they should use syscs_peek_at_sequence() to peek at the instantaneous current value of a sequence generator. This patch will require a release note explaining that users should use syscs_peek_at_sequence() rather than SYSSEQUENCES.CURRENTVALUE. Touches the following files: -------------- M java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java M java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java A java/engine/org/apache/derby/iapi/sql/dictionary/SequenceState.java New Formatable to hold the end of pre-allocation ranges. -------------- M java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java A java/engine/org/apache/derby/iapi/sql/dictionary/GhostDescriptor.java New tuple describing a row in SYSGHOST. -------------- M java/engine/org/apache/derby/iapi/sql/dictionary/CatalogRowFactory.java M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java M java/engine/org/apache/derby/impl/sql/catalog/SYSCONGLOMERATESRowFactory.java M java/engine/org/apache/derby/impl/sql/catalog/DD_Version.java Support for creating SYSGHOST and deleting orphans. -------------- M java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java M java/storeless/org/apache/derby/impl/storeless/EmptyDictionary.java A java/engine/org/apache/derby/iapi/sql/dictionary/GhostController.java A java/engine/org/apache/derby/impl/sql/catalog/GhostControllerImpl.java Logic to manage SYSGHOST. -------------- M java/engine/org/apache/derby/catalog/SystemProcedures.java Logic for new syscs_peek_at_sequence() procedure. -------------- A java/engine/org/apache/derby/diag/GhostTable.java M tools/jar/extraDBMSclasses.properties New diagnostic VTI for viewing SYSGHOST. -------------- M java/engine/org/apache/derby/impl/sql/catalog/SequenceUpdater.java New sequence updater for use on databases at level 10.9 and higher. -------------- M java/engine/org/apache/derby/impl/sql/execute/CreateSequenceConstantAction.java Add a corresponding SYSGHOST tuple when creating a sequence. If the create action is rolled back, then the SYSGHOST tuple will be garbage-collected the next time the database boots. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java Slight change to use GhostTable rather than SYSSEQUENCES.CURRENTVALUE in order to view the end of pre-allocation ranges. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SystemCatalogTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestDbMetaData.java M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_2.java M java/testing/org/apache/derbyTesting/functionTests/master/ij7.out Test changes to account for the metadata changes.
          Hide
          Mike Matrigali added a comment -

          i will spend some time commenting more in full, but wanted to post a quick response. I do not think the extra complexity being proposed
          in the patch is warranted, I may be missing the benefits of the implemented patch. In the end I think we are adding hidden conglomerates, extra boot time work, more database activity to handle
          one case where user queries the system catalogs directly causing problems for that transaction. I don't know why users are doing this. I see
          that this patch includes some new derby specific interfaces, that maybe will stop users from doing that. For all other cases I believe there
          are much easier changes that don't require such complex changes.

          My problems with that solution:
          o yet another system catalog, now we would have 2 for sequences
          o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it
          if necessary. How do you tell if there is a problem with it?
          o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single
          thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads,
          as does current implementation where work is done in the user thread.
          o brand new boot time work. I would rather not add sql layer garbage collection to the architecture.
          I know there are existing applications that boot quite often, and any additional work at boot time will cause
          the performance issues.

          Show
          Mike Matrigali added a comment - i will spend some time commenting more in full, but wanted to post a quick response. I do not think the extra complexity being proposed in the patch is warranted, I may be missing the benefits of the implemented patch. In the end I think we are adding hidden conglomerates, extra boot time work, more database activity to handle one case where user queries the system catalogs directly causing problems for that transaction. I don't know why users are doing this. I see that this patch includes some new derby specific interfaces, that maybe will stop users from doing that. For all other cases I believe there are much easier changes that don't require such complex changes. My problems with that solution: o yet another system catalog, now we would have 2 for sequences o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it if necessary. How do you tell if there is a problem with it? o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads, as does current implementation where work is done in the user thread. o brand new boot time work. I would rather not add sql layer garbage collection to the architecture. I know there are existing applications that boot quite often, and any additional work at boot time will cause the performance issues.
          Hide
          Rick Hillegas added a comment -

          Thanks for the quick analysis, Mike. Some responses follow:

          >o yet another system catalog, now we would have 2 for sequences

          More files are bad, I understand. But this would be similar to adding another index on syssequences, which is well within the bounds of what we do during a feature release.

          > o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it if necessary. How do you tell if there is a problem with it?

          Another valid question. I see it as being similar to the property conglomerate. How do we do maintenance on the property conglomerate? With SYSGHOST there is a diagnostic VTI for inspecting it and looking for problems, so I think we are in a better situation than we are with the property conglomerate.

          >o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads, as does current implementation where work is done in the user thread.

          It's true that everything happens in a single transaction controller. However, a thread switch does not happen. Instead, a context manager is pushed and popped around the use of the shared transaction controller. It's also true that calls to the GhostController are synchronized, which will single-thread access to SYSGHOST. We could introduce a separate transaction controller per sequence and synchronize on those sequence-specific transaction controllers. The additional single-threading introduced by the current patch funnels all pre-allocation requests for all sequences through the same chokepoint. This might be an issue for an application which has small pre-allocation ranges and many sequences. I don't know how typical that usage will be or whether it is worth the extra complexity of more transaction controllers.

          >o brand new boot time work. I would rather not add sql layer garbage collection to the architecture. I know there are existing applications that boot quite often, and any additional work at boot time will cause the performance issues.

          I doubt that this extra boot cost will be measurable. However, we could add an initial tuple to SYSGHOST which would flag whether there was any boot-time work to do. This would reduce the extra boot cost to one page read.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for the quick analysis, Mike. Some responses follow: >o yet another system catalog, now we would have 2 for sequences More files are bad, I understand. But this would be similar to adding another index on syssequences, which is well within the bounds of what we do during a feature release. > o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it if necessary. How do you tell if there is a problem with it? Another valid question. I see it as being similar to the property conglomerate. How do we do maintenance on the property conglomerate? With SYSGHOST there is a diagnostic VTI for inspecting it and looking for problems, so I think we are in a better situation than we are with the property conglomerate. >o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads, as does current implementation where work is done in the user thread. It's true that everything happens in a single transaction controller. However, a thread switch does not happen. Instead, a context manager is pushed and popped around the use of the shared transaction controller. It's also true that calls to the GhostController are synchronized, which will single-thread access to SYSGHOST. We could introduce a separate transaction controller per sequence and synchronize on those sequence-specific transaction controllers. The additional single-threading introduced by the current patch funnels all pre-allocation requests for all sequences through the same chokepoint. This might be an issue for an application which has small pre-allocation ranges and many sequences. I don't know how typical that usage will be or whether it is worth the extra complexity of more transaction controllers. >o brand new boot time work. I would rather not add sql layer garbage collection to the architecture. I know there are existing applications that boot quite often, and any additional work at boot time will cause the performance issues. I doubt that this extra boot cost will be measurable. However, we could add an initial tuple to SYSGHOST which would flag whether there was any boot-time work to do. This would reduce the extra boot cost to one page read. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-ab-plusMoreTests.diff. This rev of the patch adds tests for correctness, user-visible features, and garbage collection. Regression tests passed cleanly for me.

          My plan going forward is to address Mike's concerns:

          1) I will add a header tuple to SYSGHOST so that the boot-time expense of garbage collection is reduced to one page read in most situations.

          2) I will test the concurrency of this solution. If the single transaction controller turns out to be a bottleneck, then I will try adding a transaction controller per sequence.

          Touches the following additional files:

          ----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_9.java

          Various upgrade tests.

          ----------

          A java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh

          This is a standalone test for the related correctness bug, DERBY-5494. I didn't wire this test into the JUnit test runs because the test case requires that you crash the VM.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-ab-plusMoreTests.diff. This rev of the patch adds tests for correctness, user-visible features, and garbage collection. Regression tests passed cleanly for me. My plan going forward is to address Mike's concerns: 1) I will add a header tuple to SYSGHOST so that the boot-time expense of garbage collection is reduced to one page read in most situations. 2) I will test the concurrency of this solution. If the single transaction controller turns out to be a bottleneck, then I will try adding a transaction controller per sequence. Touches the following additional files: ---------- M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_9.java Various upgrade tests. ---------- A java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh This is a standalone test for the related correctness bug, DERBY-5494 . I didn't wire this test into the JUnit test runs because the test case requires that you crash the VM.
          Hide
          Mike Matrigali added a comment -

          Rick Hillegas (Commented) (JIRA) wrote:
          > [ https://issues.apache.org/jira/browse/DERBY-5493?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239613#comment-13239613 ]
          > Rick Hillegas commented on DERBY-5493:
          > --------------------------------------
          >
          > Thanks for the quick analysis, Mike. Some responses follow:
          >> o yet another system catalog, now we would have 2 for sequences
          >
          > More files are bad, I understand. But this would be similar to adding another index on syssequences, which is well within the bounds of what we do during a feature release.
          >
          We do continue to keep adding to problem here. This is yet another catalog that is added to db, and everyone has to pay for it even if they
          don't use the sequence feature. I know "1" more does not sound like a problem, but I don't think it is the right direction for derby.

          I know that there are applications out there that use 1000's of databases. We have definitely gotten complaints from users in the past just for having 1 background thread per database. Every resource we add
          on a per database level can get multiplied in some applications, and it
          is sad to add these overheads when the application is not even using the
          feature. I have the same problem with the new authorization work. I understand that the system has current problems in this area (roles), but we should not keep digging bigger holes.
          >> o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it if necessary. How do you tell if there is a problem with it?
          >
          > Another valid question. I see it as being similar to the property conglomerate. How do we do maintenance on the property conglomerate? With SYSGHOST there is a diagnostic VTI for inspecting it and looking for problems, so I think we are in a better situation than we are with the property conglomerate.
          >
          And property conglomerate is a current problem, lets not add to it. At least with property conglomerate the expectation is very low utilization and there are just not that many properties for the database. I think the utilization of this new catalog is unbounded, with size relative to
          number of sequences, and maybe number of auto generated keys.
          >> o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads, as does current implementation where work is done in the user thread.
          >
          > It's true that everything happens in a single transaction controller. However, a thread switch does not happen. Instead, a context manager is pushed and popped around the use of the shared transaction controller. It's also true that calls to the GhostController are synchronized, which will single-thread access to SYSGHOST. We could introduce a separate transaction controller per sequence and synchronize on those sequence-specific transaction controllers. The additional single-threading introduced by the current patch funnels all pre-allocation requests for all sequences through the same chokepoint. This might be an issue for an application which has small pre-allocation ranges and many sequences. I don't know how typical that usage will be or whether it is worth the extra complexity of more transaction controllers.
          ok, sounds like the thread issue is not a problem. If we go with this solution, i would like to understand why a shared single transaction is needed. Seems like we should just create a new context as we need it.
          >
          >> o brand new boot time work. I would rather not add sql layer garbage collection to the architecture. I know there are existing applications that boot quite often, and any additional work at boot time will cause the performance issues.
          >
          > I doubt that this extra boot cost will be measurable. However, we could add an initial tuple to SYSGHOST which would flag whether there was any boot-time work to do. This would reduce the extra boot cost to one page read.
          >
          I would rather see no boot time work, sounds like more complication for not enough benefit.

          I think sequences are new enough that we could implement the following as a fix:
          o do all sequence work, including creating them in a nested transaction as we did before or in this new context that you have created.
          wait on all locks in this nested transaction and throw an error if we
          timeout - don't escalate. The error should not be a generic lock timeout error, it should include warnings that sequence generation can fail if users do direct system catalog selects.

          I think this would fix the correctness issue, and solve the "normal" lock contention issue on sequence generation across threads. It would
          mean sequence creation is autocommitted, which if I understand it is more in keeping with the standard, and would avoid the problem of user
          thread holding locks on system catalogs as part of creating sequences.

          I think the new interface you have as part of your patch would make it even less likely that users would interfere with sequences by doing
          direct system catalog selects.

          Show
          Mike Matrigali added a comment - Rick Hillegas (Commented) (JIRA) wrote: > [ https://issues.apache.org/jira/browse/DERBY-5493?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13239613#comment-13239613 ] > Rick Hillegas commented on DERBY-5493 : > -------------------------------------- > > Thanks for the quick analysis, Mike. Some responses follow: >> o yet another system catalog, now we would have 2 for sequences > > More files are bad, I understand. But this would be similar to adding another index on syssequences, which is well within the bounds of what we do during a feature release. > We do continue to keep adding to problem here. This is yet another catalog that is added to db, and everyone has to pay for it even if they don't use the sequence feature. I know "1" more does not sound like a problem, but I don't think it is the right direction for derby. I know that there are applications out there that use 1000's of databases. We have definitely gotten complaints from users in the past just for having 1 background thread per database. Every resource we add on a per database level can get multiplied in some applications, and it is sad to add these overheads when the application is not even using the feature. I have the same problem with the new authorization work. I understand that the system has current problems in this area (roles), but we should not keep digging bigger holes. >> o A hidden system catalog is going to present even more problems for users if there is a problem with it. How do run maintenance on it if necessary. How do you tell if there is a problem with it? > > Another valid question. I see it as being similar to the property conglomerate. How do we do maintenance on the property conglomerate? With SYSGHOST there is a diagnostic VTI for inspecting it and looking for problems, so I think we are in a better situation than we are with the property conglomerate. > And property conglomerate is a current problem, lets not add to it. At least with property conglomerate the expectation is very low utilization and there are just not that many properties for the database. I think the utilization of this new catalog is unbounded, with size relative to number of sequences, and maybe number of auto generated keys. >> o I can't tell from the patch description, but previous description of this approach seemed to be funneling all sequence work to a single thread/transaction. This is the wrong architectural direction for derby, we should always be looking to spread this work across many threads, as does current implementation where work is done in the user thread. > > It's true that everything happens in a single transaction controller. However, a thread switch does not happen. Instead, a context manager is pushed and popped around the use of the shared transaction controller. It's also true that calls to the GhostController are synchronized, which will single-thread access to SYSGHOST. We could introduce a separate transaction controller per sequence and synchronize on those sequence-specific transaction controllers. The additional single-threading introduced by the current patch funnels all pre-allocation requests for all sequences through the same chokepoint. This might be an issue for an application which has small pre-allocation ranges and many sequences. I don't know how typical that usage will be or whether it is worth the extra complexity of more transaction controllers. ok, sounds like the thread issue is not a problem. If we go with this solution, i would like to understand why a shared single transaction is needed. Seems like we should just create a new context as we need it. > >> o brand new boot time work. I would rather not add sql layer garbage collection to the architecture. I know there are existing applications that boot quite often, and any additional work at boot time will cause the performance issues. > > I doubt that this extra boot cost will be measurable. However, we could add an initial tuple to SYSGHOST which would flag whether there was any boot-time work to do. This would reduce the extra boot cost to one page read. > I would rather see no boot time work, sounds like more complication for not enough benefit. I think sequences are new enough that we could implement the following as a fix: o do all sequence work, including creating them in a nested transaction as we did before or in this new context that you have created. wait on all locks in this nested transaction and throw an error if we timeout - don't escalate. The error should not be a generic lock timeout error, it should include warnings that sequence generation can fail if users do direct system catalog selects. I think this would fix the correctness issue, and solve the "normal" lock contention issue on sequence generation across threads. It would mean sequence creation is autocommitted, which if I understand it is more in keeping with the standard, and would avoid the problem of user thread holding locks on system catalogs as part of creating sequences. I think the new interface you have as part of your patch would make it even less likely that users would interfere with sequences by doing direct system catalog selects.
          Hide
          Mike Matrigali added a comment -

          Long term I believe the goal should be to design a solution for sequences that can also be used for generated keys. I agree with kathey
          that we should look at a phased approach with sequence implementation first and identity column in a subsequent release. So am going
          to talk about that here, though the jira issue is about sequences.

          I think a simple solution that does the following would address both correctness and performance issues:
          o all "next" operations, including ddl to create them always happen in a nested transaction and they should at worst get 1 lock on 1 row
          in a system catalog. I am leaning now that that catalog can not be SYSCOLUMNS as this catalog is just to intertwined with other
          ddl operations. I am hoping that it can be SYSSEQUENCES.
          o these operations should be scalable as possible and be designed to allow good performance for large number of thread across large
          number of processors across a large number of sequences and generated keys.

          Above I described what I hope works for sequences, basically just move all current sequence operations into nested transaction - need to
          verify a create sequence only adds a single row to a sequence catalog. Doing all work for sequences in a nested transaction I think
          eliminates the whole garbage collection issue. Never escalate, throw errors on lock timeouts/deadlocks. By not escalating we eliminate
          the correctness problems with escalating.

          generated keys are not as easy as we can't just move all create tables into nested transactions. My proposal here is that create
          table does not do anything with SYSSEQUENCES, it delays the addition of a row to sequence catalog until first insert. Again all
          operation next operations are done same as above proposal for sequences.
          note: I think drop table is one exception where we will get lock on sequence row in user transaction, but that seems ok as no
          one should be doing anything on table once we have the table exclusive lock on drop table. and also note drop table needs to
          handle the sequence row existing or not, both are valid.
          note2: i think a serialized isolation level is needed to handle on demand creation of the sequences row, to handle two threads trying
          to create the row.

          I hope current sequence catalog can either support this now, or can with an upgrade. If we feel we need another catalog I would rather
          see it designed as a replacement for the sequence catalog(s). At least then new databases only have what is needed, or maybe we
          can even get rid of old catalog on hard upgrade.

          This proposal is not addressing feature of the ghost catalog that prevents users from getting locks on the catalogs by scanning
          them directly. I am ok with throwing errors on lock timeouts and calling out the likely cause. Long term it would be better to somehow turn all system catalogs into "ghosts" with respect at least to unintended lock interactions from user direct access. maybe we could change
          system to not get locks on system catalogs direct scans by users. Maybe we should beef up the documentation on not supporting
          direct access to system catalogs and call out locking issues.

          Show
          Mike Matrigali added a comment - Long term I believe the goal should be to design a solution for sequences that can also be used for generated keys. I agree with kathey that we should look at a phased approach with sequence implementation first and identity column in a subsequent release. So am going to talk about that here, though the jira issue is about sequences. I think a simple solution that does the following would address both correctness and performance issues: o all "next" operations, including ddl to create them always happen in a nested transaction and they should at worst get 1 lock on 1 row in a system catalog. I am leaning now that that catalog can not be SYSCOLUMNS as this catalog is just to intertwined with other ddl operations. I am hoping that it can be SYSSEQUENCES. o these operations should be scalable as possible and be designed to allow good performance for large number of thread across large number of processors across a large number of sequences and generated keys. Above I described what I hope works for sequences, basically just move all current sequence operations into nested transaction - need to verify a create sequence only adds a single row to a sequence catalog. Doing all work for sequences in a nested transaction I think eliminates the whole garbage collection issue. Never escalate, throw errors on lock timeouts/deadlocks. By not escalating we eliminate the correctness problems with escalating. generated keys are not as easy as we can't just move all create tables into nested transactions. My proposal here is that create table does not do anything with SYSSEQUENCES, it delays the addition of a row to sequence catalog until first insert. Again all operation next operations are done same as above proposal for sequences. note: I think drop table is one exception where we will get lock on sequence row in user transaction, but that seems ok as no one should be doing anything on table once we have the table exclusive lock on drop table. and also note drop table needs to handle the sequence row existing or not, both are valid. note2: i think a serialized isolation level is needed to handle on demand creation of the sequences row, to handle two threads trying to create the row. I hope current sequence catalog can either support this now, or can with an upgrade. If we feel we need another catalog I would rather see it designed as a replacement for the sequence catalog(s). At least then new databases only have what is needed, or maybe we can even get rid of old catalog on hard upgrade. This proposal is not addressing feature of the ghost catalog that prevents users from getting locks on the catalogs by scanning them directly. I am ok with throwing errors on lock timeouts and calling out the likely cause. Long term it would be better to somehow turn all system catalogs into "ghosts" with respect at least to unintended lock interactions from user direct access. maybe we could change system to not get locks on system catalogs direct scans by users. Maybe we should beef up the documentation on not supporting direct access to system catalogs and call out locking issues.
          Hide
          Rick Hillegas added a comment -

          Thanks for continuing the discussion, Mike. I think we may be converging on a simpler solution. I need to think some more about this but I agree that a phased approach is probably safest. I also agree that a common approach which handles both sequences and identity columns is desirable. Some quick responses:

          Here's a modification of your proposal:

          1) We eliminate the ghost catalog.

          2) But we dedicate a special transaction controller per sequence. All operations (except ddl) on a SYSSEQUENCES tuple happen in the transaction controller which is dedicated to that row. We could also experiment with a single sequence-controlling transaction if people think that the cost of multiple transaction controllers is too high.

          3) We will tell users to stop trying to query SYSSEQUENCES and to, instead, use the new syscs_peek_at_sequence() function. That function will do its work in the transaction controller which is dedicated to the corresponding SYSSEQUENCES tuple.

          4) The NEXT VALUE FOR invocation will, if necessary, pre-allocate a new range in the SYSSEQUENCE row by using the row's dedicated transaction controller. Access to this transaction controller will be single-threaded so syscs_peek_at_sequence() and NEXT VALUE FOR should not block each other. Attempts to pre-allocate a range should always succeed unless DDL is happening on the sequence or the user is directly querying the row--we will tell users they shouldn't do that but should instead use syscs_peek_at_sequence().

          I don't understand your proposal about how to tackle identity columns. You may be suggesting that we create a sequence (backed by a row in SYSSEQUENCES) for every identity column. I think that is a good approach and it aligns us with the behavior of the SQL Standard, which says that an identity column should behave as though it is implemented with an internal sequence.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for continuing the discussion, Mike. I think we may be converging on a simpler solution. I need to think some more about this but I agree that a phased approach is probably safest. I also agree that a common approach which handles both sequences and identity columns is desirable. Some quick responses: Here's a modification of your proposal: 1) We eliminate the ghost catalog. 2) But we dedicate a special transaction controller per sequence. All operations (except ddl) on a SYSSEQUENCES tuple happen in the transaction controller which is dedicated to that row. We could also experiment with a single sequence-controlling transaction if people think that the cost of multiple transaction controllers is too high. 3) We will tell users to stop trying to query SYSSEQUENCES and to, instead, use the new syscs_peek_at_sequence() function. That function will do its work in the transaction controller which is dedicated to the corresponding SYSSEQUENCES tuple. 4) The NEXT VALUE FOR invocation will, if necessary, pre-allocate a new range in the SYSSEQUENCE row by using the row's dedicated transaction controller. Access to this transaction controller will be single-threaded so syscs_peek_at_sequence() and NEXT VALUE FOR should not block each other. Attempts to pre-allocate a range should always succeed unless DDL is happening on the sequence or the user is directly querying the row--we will tell users they shouldn't do that but should instead use syscs_peek_at_sequence(). I don't understand your proposal about how to tackle identity columns. You may be suggesting that we create a sequence (backed by a row in SYSSEQUENCES) for every identity column. I think that is a good approach and it aligns us with the behavior of the SQL Standard, which says that an identity column should behave as though it is implemented with an internal sequence. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-ad-simplerApproach.diff. This patch is not ready for commit. I want to run some throughput tests with it first. The patch implements the approach which we seem to have converged on:

          1) Retains the syscs_peek_at_sequence() function so that users can get the instantaneous value of a sequence without having to query SYSSEQUENCES. I changed the signature of this function. Instead of a single uuid argument, the function now takes a schema and sequence name pair. The idea here is to not tempt users into driving this function with a select from SYSSEQUENCES in order to get the uuid.

          2) Instead of using an invisible conglomerate, pre-allocation happens in the SYSSEQUENCES row (as in 10.8).

          3) Pre-allocation is done by a transaction which is dedicated to the sequence. The transaction expects to do its work immediately, without waiting for a lock, and then to autocommit that work. If the transaction can't get a lock immediately, it raises a TOO MUCH CONTENTION exception. We seem to agree that TOO MUCH CONTENTION can only arise if sequence DDL is in flight or if the application scans SYSSEQUENCES against our advice.

          I tried a slightly different approach at first. In that approach, pre-allocation happened in a sub-transaction of the session's execution transaction. That approach fixed this bug (DERBY-5493) but did not fix DERBY-5494. By using a separate, dedicated transaction, I am able to fix DERBY-5494 as well.

          Touches the following files:

          --------------

          M java/storeless/org/apache/derby/impl/storeless/EmptyDictionary.java
          M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java
          M java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java
          M java/engine/org/apache/derby/catalog/SystemProcedures.java

          Support for the new syscs_peek_at_sequence() function.

          --------------

          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java

          I changed the wording of the TOO MUCH CONTENTION exception so that it points the user at the likely cause of the problem: an open scan of SYSSEQUENCES. I changed this exception to Transaction severity so that the language layer does not get confused during cleanup.

          --------------

          M java/engine/org/apache/derby/impl/sql/catalog/SequenceUpdater.java

          Re-plumbed the SequenceUpdater as described above.

          --------------

          M java/engine/org/apache/derby/impl/sql/catalog/SequenceGenerator.java

          Adjusted the wording of some comments.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh
          M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_9.java

          Tests for the new system function and to verify the fixes to the correctness problems.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_2.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestDbMetaData.java
          M java/testing/org/apache/derbyTesting/functionTests/master/ij7.out

          Tests which had to change to account for the new system function.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-ad-simplerApproach.diff. This patch is not ready for commit. I want to run some throughput tests with it first. The patch implements the approach which we seem to have converged on: 1) Retains the syscs_peek_at_sequence() function so that users can get the instantaneous value of a sequence without having to query SYSSEQUENCES. I changed the signature of this function. Instead of a single uuid argument, the function now takes a schema and sequence name pair. The idea here is to not tempt users into driving this function with a select from SYSSEQUENCES in order to get the uuid. 2) Instead of using an invisible conglomerate, pre-allocation happens in the SYSSEQUENCES row (as in 10.8). 3) Pre-allocation is done by a transaction which is dedicated to the sequence. The transaction expects to do its work immediately, without waiting for a lock, and then to autocommit that work. If the transaction can't get a lock immediately, it raises a TOO MUCH CONTENTION exception. We seem to agree that TOO MUCH CONTENTION can only arise if sequence DDL is in flight or if the application scans SYSSEQUENCES against our advice. I tried a slightly different approach at first. In that approach, pre-allocation happened in a sub-transaction of the session's execution transaction. That approach fixed this bug ( DERBY-5493 ) but did not fix DERBY-5494 . By using a separate, dedicated transaction, I am able to fix DERBY-5494 as well. Touches the following files: -------------- M java/storeless/org/apache/derby/impl/storeless/EmptyDictionary.java M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java M java/engine/org/apache/derby/iapi/sql/dictionary/DataDictionary.java M java/engine/org/apache/derby/catalog/SystemProcedures.java Support for the new syscs_peek_at_sequence() function. -------------- M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java I changed the wording of the TOO MUCH CONTENTION exception so that it points the user at the likely cause of the problem: an open scan of SYSSEQUENCES. I changed this exception to Transaction severity so that the language layer does not get confused during cleanup. -------------- M java/engine/org/apache/derby/impl/sql/catalog/SequenceUpdater.java Re-plumbed the SequenceUpdater as described above. -------------- M java/engine/org/apache/derby/impl/sql/catalog/SequenceGenerator.java Adjusted the wording of some comments. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java A java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_9.java Tests for the new system function and to verify the fixes to the correctness problems. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_2.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RolesTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/TestDbMetaData.java M java/testing/org/apache/derbyTesting/functionTests/master/ij7.out Tests which had to change to account for the new system function.
          Hide
          Mike Matrigali added a comment -

          I will review these changes more carefully in next few days, this comment is just based on your comment describing the changes. Did you
          understand why this approach fixes DERBY-5494 vs the subtransaction approach? In the normal sub transaction case I was thinking
          that it should wait on locks and return too much contention on a lock timeout, to take care of a expected very short lock waits
          if 2 threads are trying to update the sequence at same time, but maybe the 1 transaction per sequence makes this impossible to happen.

          What is the expected behavior of creating a sequence, with respect to autocommit and locking? Is the create done in the nested transaction
          and autocommitted?

          A comment I meant to make before, is that DERBY-5495 requires crash testing, and now there is a framework to do that in junit tests -
          see store/OCRecoveryTest.java, basically there is a routine to fork out a process to run a java test procedure and then you just exit which
          will run through the "unclean" shutdown and next test connects running through whatever recovery will do on that unclean shutdown.

          Show
          Mike Matrigali added a comment - I will review these changes more carefully in next few days, this comment is just based on your comment describing the changes. Did you understand why this approach fixes DERBY-5494 vs the subtransaction approach? In the normal sub transaction case I was thinking that it should wait on locks and return too much contention on a lock timeout, to take care of a expected very short lock waits if 2 threads are trying to update the sequence at same time, but maybe the 1 transaction per sequence makes this impossible to happen. What is the expected behavior of creating a sequence, with respect to autocommit and locking? Is the create done in the nested transaction and autocommitted? A comment I meant to make before, is that DERBY-5495 requires crash testing, and now there is a framework to do that in junit tests - see store/OCRecoveryTest.java, basically there is a routine to fork out a process to run a java test procedure and then you just exit which will run through the "unclean" shutdown and next test connects running through whatever recovery will do on that unclean shutdown.
          Hide
          Rick Hillegas added a comment -

          Thanks for your comments and your offer to review this patch, Mike. Some comments inline...

          ...

          >Did you understand why this approach fixes DERBY-5494 vs the subtransaction approach?

          I don't understand why the subtransaction approach fails. I have attached a revised repro to DERBY-5494. That shows the behavior in the current trunk without this patch. The sequence generator durably commits a change to SYSSEQUENCES, which can be seen by another thread. The system-crash reverses this supposedly durable change. The same behavior can be seen in 10.8.2.2 (except that the size of the pre-allocation range is shorter so the second thread sees a slightly different value before the crash).

          I decided to try a dedicated transaction because I knew, from the ghost conglomerate experiment, that that approach fixed DERBY-5494.

          The only variable that I can see is the use of a subtransaction vs the use of an ordinary transaction. You are the expert in the implementation and use of subtransactions. Do you have any theories about what is going on?

          >In the normal sub transaction case I was thinking that it should wait on locks and return too much contention on a lock timeout, to take care of a expected very short lock waits if 2 threads are trying to update the sequence at same time, but maybe the 1 transaction per sequence makes this impossible to happen.

          The 1 transaction per sequence should make this impossible for the cases which I thought we wanted to support. Waiting for the lock would support the following additional use-cases, but I thought that we agreed they were not worth supporting:

          a) Dropping a sequence and then rolling back that transaction.

          b) Scans of SYSSEQUENCES by the application.

          >What is the expected behavior of creating a sequence, with respect to autocommit and locking? Is the create done in the nested tr nsaction and autocommitted?

          Nothing has changed in the CREATE SEQUENCE logic. That work happens in an ordinary user transaction, just as it did in 10.8. At the end of this comment, I attach a script which shows that if you create a sequence and a table but don't commit that work, then another thread will timeout trying to use those objects.

          This is the behavior which I thought we agreed: DDL happens in ordinary user transactions. It is just the NEXT VALUE FOR which happens in an unusual transaction. Note that SYSCS_PEEK_AT_SEQUENCE doesn't actually do any transactional work, it just looks at an in-memory value via a synchronized method.

          >A comment I meant to make before, is that DERBY-5495 requires crash testing, and now there is a framework to do that in junit tests - see store/OCRecoveryTest.java, basically there is a routine to fork out a process to run a java test procedure and then you just exit which will run through the "unclean" shutdown and next test connects running through whatever recovery will do on that unclean shutdown.

          Great! I will look into making a proper JUnit test out of java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh.

          Thanks,
          -Rick

          ------------------

          Here is the script which demonstrates the behavior of uncommitted ddl:

          connect 'jdbc:derby:memory:db;create=true' as dbo;

          call syscs_util.syscs_set_database_property( 'derby.locks.waitTimeout', '2' );

          autocommit off;

          create sequence foo;
          create table t( a int );

          connect 'jdbc:derby:memory:db' as otheruser;

          – times out waiting for the other thread's ddl to commit
          values next value for foo;
          select * from t;

          set connection dbo;

          commit;

          set connection otheruser;

          – succeeds
          values next value for foo;
          select * from t;

          Show
          Rick Hillegas added a comment - Thanks for your comments and your offer to review this patch, Mike. Some comments inline... ... >Did you understand why this approach fixes DERBY-5494 vs the subtransaction approach? I don't understand why the subtransaction approach fails. I have attached a revised repro to DERBY-5494 . That shows the behavior in the current trunk without this patch. The sequence generator durably commits a change to SYSSEQUENCES, which can be seen by another thread. The system-crash reverses this supposedly durable change. The same behavior can be seen in 10.8.2.2 (except that the size of the pre-allocation range is shorter so the second thread sees a slightly different value before the crash). I decided to try a dedicated transaction because I knew, from the ghost conglomerate experiment, that that approach fixed DERBY-5494 . The only variable that I can see is the use of a subtransaction vs the use of an ordinary transaction. You are the expert in the implementation and use of subtransactions. Do you have any theories about what is going on? >In the normal sub transaction case I was thinking that it should wait on locks and return too much contention on a lock timeout, to take care of a expected very short lock waits if 2 threads are trying to update the sequence at same time, but maybe the 1 transaction per sequence makes this impossible to happen. The 1 transaction per sequence should make this impossible for the cases which I thought we wanted to support. Waiting for the lock would support the following additional use-cases, but I thought that we agreed they were not worth supporting: a) Dropping a sequence and then rolling back that transaction. b) Scans of SYSSEQUENCES by the application. >What is the expected behavior of creating a sequence, with respect to autocommit and locking? Is the create done in the nested tr nsaction and autocommitted? Nothing has changed in the CREATE SEQUENCE logic. That work happens in an ordinary user transaction, just as it did in 10.8. At the end of this comment, I attach a script which shows that if you create a sequence and a table but don't commit that work, then another thread will timeout trying to use those objects. This is the behavior which I thought we agreed: DDL happens in ordinary user transactions. It is just the NEXT VALUE FOR which happens in an unusual transaction. Note that SYSCS_PEEK_AT_SEQUENCE doesn't actually do any transactional work, it just looks at an in-memory value via a synchronized method. >A comment I meant to make before, is that DERBY-5495 requires crash testing, and now there is a framework to do that in junit tests - see store/OCRecoveryTest.java, basically there is a routine to fork out a process to run a java test procedure and then you just exit which will run through the "unclean" shutdown and next test connects running through whatever recovery will do on that unclean shutdown. Great! I will look into making a proper JUnit test out of java/testing/org/apache/derbyTesting/functionTests/tests/lang/t_5494.sh. Thanks, -Rick ------------------ Here is the script which demonstrates the behavior of uncommitted ddl: connect 'jdbc:derby:memory:db;create=true' as dbo; call syscs_util.syscs_set_database_property( 'derby.locks.waitTimeout', '2' ); autocommit off; create sequence foo; create table t( a int ); connect 'jdbc:derby:memory:db' as otheruser; – times out waiting for the other thread's ddl to commit values next value for foo; select * from t; set connection dbo; commit; set connection otheruser; – succeeds values next value for foo; select * from t;
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-ae-simplerApproachWithCrashJUnitTest.diff. This is identical to the previous patch except that it adds a JUnit test case to verify the fix to DERBY-5494, using assertLaunchedJUnitTestMethod as Mike suggested. This test case could be added to the fix for DERBY-5494 which Mike is working on.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-ae-simplerApproachWithCrashJUnitTest.diff. This is identical to the previous patch except that it adds a JUnit test case to verify the fix to DERBY-5494 , using assertLaunchedJUnitTestMethod as Mike suggested. This test case could be added to the fix for DERBY-5494 which Mike is working on.
          Hide
          Rick Hillegas added a comment -

          The test case was added to SequenceGeneratorTest. It is called test_13_5494(). I have run it successfully on the following platforms:

          o Java 6 on Mac OS X

          o Java 6 on XP

          o PhoneME emulator on an Ubuntu guest operating system on Mac OS X.

          Show
          Rick Hillegas added a comment - The test case was added to SequenceGeneratorTest. It is called test_13_5494(). I have run it successfully on the following platforms: o Java 6 on Mac OS X o Java 6 on XP o PhoneME emulator on an Ubuntu guest operating system on Mac OS X.
          Hide
          Mike Matrigali added a comment -

          rick, should I move forward and fix DERBY-5494 separate from the patch in this issue? I have been spending time looking at it rather than reviewing this change? I think i can code rest of that fix today and if tests pass tonight be ready to checkin tommorow.

          I think both my proposed fix and your patch (by using a user level transaction) are both causing the log to be flushed to avoid the crash problem. For sequences I assume this is necessary and we will have to pay the perfomance cost. I don't think there is anyway to solve the crash problem without forcing the log at commit time of the system catalog update.

          Show
          Mike Matrigali added a comment - rick, should I move forward and fix DERBY-5494 separate from the patch in this issue? I have been spending time looking at it rather than reviewing this change? I think i can code rest of that fix today and if tests pass tonight be ready to checkin tommorow. I think both my proposed fix and your patch (by using a user level transaction) are both causing the log to be flushed to avoid the crash problem. For sequences I assume this is necessary and we will have to pay the perfomance cost. I don't think there is anyway to solve the crash problem without forcing the log at commit time of the system catalog update.
          Hide
          Rick Hillegas added a comment -

          Hi Mike,

          Yes, I think that your fix for DERBY-5494 should be pursued independently of the patch in this issue. I believe that your fix will make nested transactions behave in a way which is less surprising. Am I right that your fix will address the correctness problem with identity columns? If so, that is an extra reason to pursue your fix independently, since this issue does not touch the identity logic. Thanks.

          Show
          Rick Hillegas added a comment - Hi Mike, Yes, I think that your fix for DERBY-5494 should be pursued independently of the patch in this issue. I believe that your fix will make nested transactions behave in a way which is less surprising. Am I right that your fix will address the correctness problem with identity columns? If so, that is an extra reason to pursue your fix independently, since this issue does not touch the identity logic. Thanks.
          Hide
          Rick Hillegas added a comment -

          I have run a quick concurrency test against the patch, using the test introduced on derby-4565:

          java org.apache.derbyTesting.perf.clients.Runner \
          -driver org.apache.derby.jdbc.EmbeddedDriver \
          -init \
          -load seq_gen \
          -load_opts debugging=0,numberOfGenerators=1,tablesPerGenerator=0,insertsPerTransaction=100 \
          -gen b2b \
          -threads 10 \

          On my dual-core Mac I'm getting slightly better numbers (average of 291 tx/seconds) than I recorded for this test case on derby-4565. I don't think the improvement is due to this patch, it is more likely due to the fact that I have a faster machine this time around. However, it encourages me to think that the patch has not harmed the concurrency of sequence generators. I'm looking for a beefier machine with more processors now.

          Show
          Rick Hillegas added a comment - I have run a quick concurrency test against the patch, using the test introduced on derby-4565: java org.apache.derbyTesting.perf.clients.Runner \ -driver org.apache.derby.jdbc.EmbeddedDriver \ -init \ -load seq_gen \ -load_opts debugging=0,numberOfGenerators=1,tablesPerGenerator=0,insertsPerTransaction=100 \ -gen b2b \ -threads 10 \ On my dual-core Mac I'm getting slightly better numbers (average of 291 tx/seconds) than I recorded for this test case on derby-4565. I don't think the improvement is due to this patch, it is more likely due to the fact that I have a faster machine this time around. However, it encourages me to think that the patch has not harmed the concurrency of sequence generators. I'm looking for a beefier machine with more processors now.
          Hide
          Mike Matrigali added a comment -

          ok, i will move forward on DERBY-5494. I will not be changing behavior of identity columns. I think we should look at that as a separate issue, and see if standard is open to interpretation for identity columns. If at all possible I would like to keep the performance optimization currently used with identity columns. I will add comments in the code to make it obvious what is going on, rather than hidden in store as it is now. With identity columns I believe the problem is much less vs sequences, at least in normal application expectations. It seems reasonable to me for applications to not "remember" an identity column value until the inserting transaction commits, or to just use it in same transaction that inserted and produced the value making its use transactional . I think the current behaviour never results in duplicate identity column values being produced for use in a row.

          Show
          Mike Matrigali added a comment - ok, i will move forward on DERBY-5494 . I will not be changing behavior of identity columns. I think we should look at that as a separate issue, and see if standard is open to interpretation for identity columns. If at all possible I would like to keep the performance optimization currently used with identity columns. I will add comments in the code to make it obvious what is going on, rather than hidden in store as it is now. With identity columns I believe the problem is much less vs sequences, at least in normal application expectations. It seems reasonable to me for applications to not "remember" an identity column value until the inserting transaction commits, or to just use it in same transaction that inserted and produced the value making its use transactional . I think the current behaviour never results in duplicate identity column values being produced for use in a row.
          Hide
          Mike Matrigali added a comment -

          i don't know anything about mac, do you happen to know if writes are properly sync'd on the machine you are using? It is pretty obvious if you run a simple derby program that does a commit per very small insert single threaded. A properly synced machine will quickly become I/O bound somewhere around
          30-100 i/o's per second on regular ide or scsi hardware. maybe just posting results of 1 thread vs 10 threads would be enough in your test, i don't know much about the other configurations.

          Show
          Mike Matrigali added a comment - i don't know anything about mac, do you happen to know if writes are properly sync'd on the machine you are using? It is pretty obvious if you run a simple derby program that does a commit per very small insert single threaded. A properly synced machine will quickly become I/O bound somewhere around 30-100 i/o's per second on regular ide or scsi hardware. maybe just posting results of 1 thread vs 10 threads would be enough in your test, i don't know much about the other configurations.
          Hide
          Rick Hillegas added a comment -

          I don't think that there is any wiggle room in the Standard, but I also think that the correctness problem with identity columns is not one that most users are going to care about. As you say, users are most likely going to be interested in never seeing duplicates in primary key columns. The only edge case I can think of is an application which was coded before we introduced sequences and which created a table with one column, an identity column, used to simulate a unique id generator for the whole application. That edge case would be concerned about the correctness problem with identity columns. Thanks.

          Show
          Rick Hillegas added a comment - I don't think that there is any wiggle room in the Standard, but I also think that the correctness problem with identity columns is not one that most users are going to care about. As you say, users are most likely going to be interested in never seeing duplicates in primary key columns. The only edge case I can think of is an application which was coded before we introduced sequences and which created a table with one column, an identity column, used to simulate a unique id generator for the whole application. That edge case would be concerned about the correctness problem with identity columns. Thanks.
          Hide
          Rick Hillegas added a comment -

          Thanks for that warning about the write-cache on Mac OSX, Mike. I believe that the write-cache is enabled by default, so that would account for the high transaction rate.

          I have run a similar experiment on a beefier machine (32 processor, Solaris). The transaction rate is in the range which you would expect for a disabled write-cache. This experiment ran 32 threads:

          java org.apache.derbyTesting.perf.clients.Runner \
          -driver org.apache.derby.jdbc.EmbeddedDriver \
          -init \
          -load seq_gen \
          -load_opts debugging=0,numberOfGenerators=1,tablesPerGenerator=0,insertsPerTransaction=100 \
          -gen b2b \
          -threads 32 \

          I ran the experiment 3 times against the current trunk and then 3 times against the patch (averaging the results). With the old sequence management (what's currently in the trunk), I got 49 tx/sec.. With the patch, I got 75 tx/sec..

          Show
          Rick Hillegas added a comment - Thanks for that warning about the write-cache on Mac OSX, Mike. I believe that the write-cache is enabled by default, so that would account for the high transaction rate. I have run a similar experiment on a beefier machine (32 processor, Solaris). The transaction rate is in the range which you would expect for a disabled write-cache. This experiment ran 32 threads: java org.apache.derbyTesting.perf.clients.Runner \ -driver org.apache.derby.jdbc.EmbeddedDriver \ -init \ -load seq_gen \ -load_opts debugging=0,numberOfGenerators=1,tablesPerGenerator=0,insertsPerTransaction=100 \ -gen b2b \ -threads 32 \ I ran the experiment 3 times against the current trunk and then 3 times against the patch (averaging the results). With the old sequence management (what's currently in the trunk), I got 49 tx/sec.. With the patch, I got 75 tx/sec..
          Hide
          Mike Matrigali added a comment -

          meant to take ownership of DERBY-5494, not this one.

          Show
          Mike Matrigali added a comment - meant to take ownership of DERBY-5494 , not this one.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-af-usersubtran.diff. This patch fixes the concurrency problems with the previous patch which were disclosed by the experiments described on DERBY-5471.

          The fix is to go back to updating SYSSEQUENCES in a subtransaction of the user's execution transaction. This means that this patch does not fix DERBY-5494. So I won't commit this patch until Mike commits his work on DERBY-5494. At that point, I will adjust the patch to account for the fact that Mike is going to include the regression test case for that issue which was introduced by the earlier rev of this patch.

          With this version of the patch the tx/sec results are slightly better than the comparable experiments on 10.8.2.2 and trunk:

          o 10 threads with preallocation=20: 99 vs 92 on trunk

          o 10 threads with preallocation=5: 78 vs 75 on 10.8.2.2

          o 1 thread with preallocation=20: 45 vs 44 on trunk

          o 1 thread with preallocation=5: 36 vs 35 on 10.8.2.2

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-af-usersubtran.diff. This patch fixes the concurrency problems with the previous patch which were disclosed by the experiments described on DERBY-5471 . The fix is to go back to updating SYSSEQUENCES in a subtransaction of the user's execution transaction. This means that this patch does not fix DERBY-5494 . So I won't commit this patch until Mike commits his work on DERBY-5494 . At that point, I will adjust the patch to account for the fact that Mike is going to include the regression test case for that issue which was introduced by the earlier rev of this patch. With this version of the patch the tx/sec results are slightly better than the comparable experiments on 10.8.2.2 and trunk: o 10 threads with preallocation=20: 99 vs 92 on trunk o 10 threads with preallocation=5: 78 vs 75 on 10.8.2.2 o 1 thread with preallocation=20: 45 vs 44 on trunk o 1 thread with preallocation=5: 36 vs 35 on 10.8.2.2
          Hide
          Mike Matrigali added a comment - - edited

          I committed a fix for DERBY-5494. I was going to try to do some more performance tests, but turns out my windowsXP laptop has write cache enabled, and I can't seem to get it turned off as the control box is greyed out. I am going to find another machine
          but figured since all tests pass, it would be better to get code in trunk so you can work off it, and we can see full effect of both
          changes. I expect the 5494 change to have a
          negative affect on performance of sequences. The worst case will be 1 thread, preallocation=0 (or 1 I don't know which gives you no preallocation). I would assume less and less affect the larger the preallocation, or more users.

          I would not be surprised if the throughput goes back to same problem you were seeing with user transaction vs nested transaction, now that the nested user transaction will wait on the commit like the user transaction. If all threads need to wait for the preallocation unit of work step, and we now have added a synchonous I/O to that process, then would expect throughput to drop.

          Show
          Mike Matrigali added a comment - - edited I committed a fix for DERBY-5494 . I was going to try to do some more performance tests, but turns out my windowsXP laptop has write cache enabled, and I can't seem to get it turned off as the control box is greyed out. I am going to find another machine but figured since all tests pass, it would be better to get code in trunk so you can work off it, and we can see full effect of both changes. I expect the 5494 change to have a negative affect on performance of sequences. The worst case will be 1 thread, preallocation=0 (or 1 I don't know which gives you no preallocation). I would assume less and less affect the larger the preallocation, or more users. I would not be surprised if the throughput goes back to same problem you were seeing with user transaction vs nested transaction, now that the nested user transaction will wait on the commit like the user transaction. If all threads need to wait for the preallocation unit of work step, and we now have added a synchonous I/O to that process, then would expect throughput to drop.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-01-ag-mergedWith5494.diff. This is the patch after syncing with the trunk and merging with Mike's fix to DERBY-5494.

          At this point the tests run cleanly. However, as Mike suspected, concurrency has returned to the situation with the 01-ae patch, what was reported on DERBY-5471.

          In a follow-on patch, I think I will boost the pre-allocation size to 100 unless someone objects.

          Committed at subversion revision 1327471.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-01-ag-mergedWith5494.diff. This is the patch after syncing with the trunk and merging with Mike's fix to DERBY-5494 . At this point the tests run cleanly. However, as Mike suspected, concurrency has returned to the situation with the 01-ae patch, what was reported on DERBY-5471 . In a follow-on patch, I think I will boost the pre-allocation size to 100 unless someone objects. Committed at subversion revision 1327471.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-5493-02-aa-boostPreallocationTo100.diff. This boosts the preallocation size to 100 in order to get slightly better concurrency than we saw in 10.8. Tests passed cleanly for me. Committed at subversion revision 1327682.

          Touches the following files:

          --------------

          M java/engine/org/apache/derby/impl/sql/catalog/SequenceRange.java

          The actual change.

          --------------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java

          Boosting the preallocation size caused a combinatorial explosion in the number of cases stressed in the boundary test for sequences. I pared that test back to a manageble set of cases.

          Show
          Rick Hillegas added a comment - Attaching derby-5493-02-aa-boostPreallocationTo100.diff. This boosts the preallocation size to 100 in order to get slightly better concurrency than we saw in 10.8. Tests passed cleanly for me. Committed at subversion revision 1327682. Touches the following files: -------------- M java/engine/org/apache/derby/impl/sql/catalog/SequenceRange.java The actual change. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/SequenceGeneratorTest.java Boosting the preallocation size caused a combinatorial explosion in the number of cases stressed in the boundary test for sequences. I pared that test back to a manageble set of cases.
          Hide
          Rick Hillegas added a comment -

          Attaching the first rev of a release note for this issue.

          Show
          Rick Hillegas added a comment - Attaching the first rev of a release note for this issue.
          Hide
          Rick Hillegas added a comment -

          I believe the work on this issue is done.

          Show
          Rick Hillegas added a comment - I believe the work on this issue is done.

            People

            • Assignee:
              Rick Hillegas
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development