Derby
  1. Derby
  2. DERBY-3790

Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.9.1.0
    • Component/s: Store
    • Labels:
      None
    • Bug behavior facts:
      Performance

      Description

      DERBY-269 provided a manual way to update the statisitcs. There was some discussion in that jira entry for possibly optimizing the cases where there is no need to update the statistics. I will enter the related comments from that jira entry here for reference.

      **************************
      Knut Anders Hatlen - 18/Jul/08 12:39 AM
      If I have understood correctly, unique indexes always have up to date cardinality statistics because cardinality == row count. If that's the case, one possible optimization is to skip the unique indexes when SYSCS_UPDATE_STATISTICS is called.
      **************************

      **************************
      Mike Matrigali - 18/Jul/08 09:48 AM
      is the cardinality of a unique index 1 or is it row count?

      It is also more complicated than just skipping unique indexes, it depends on the number of columns in the index because
      in a multi-column index, multiple cardinalities are calculated. So for instance on an index on columns A,B,C there are
      actually 3 cardinalities calculated:
      A
      A,B
      A,B,C

      I agree that the calculation of cardinality of A,B,C could/should be short circuited for a unique index.
      **************************

      **************************
      Knut Anders Hatlen - 18/Jul/08 03:25 PM
      Mike,
      It looks to me as if the cardinality is the number of unique values, so I think the cardinality of a unique index is equal to its row count (for the full key, that is). You're right that we can't short circuit it if we have a multi-column index. I don't know if it's worth the extra complexity to short circuit the A,B,C case, since we'd have to scan the entire index anyway. For a single-column unique index it sounds like a good idea, though.
      **************************

      1. derby-3790-1a-skip_stats_scui.diff
        9 kB
        Kristian Waagan
      2. derby-3790-1b-skip_stats_scui.diff
        55 kB
        Kristian Waagan
      3. derby-3790-1c-skip_stats_scui.diff
        55 kB
        Kristian Waagan
      4. derby-3790-2a-minor_test_improvements.diff
        7 kB
        Kristian Waagan

        Issue Links

          Activity

          Hide
          ASF subversion and git services added a comment -

          Commit 1498173 from Mamta A. Satoor
          [ https://svn.apache.org/r1498173 ]

          DERBY-5680(indexStat daemon processing tables over and over even when there are no changes in the tables)

          KeepDisposableStatsPropertyTest test is meant for DERBY-3790 which is not in in 10.8 codeline and hence should not be run in 10.8

          Show
          ASF subversion and git services added a comment - Commit 1498173 from Mamta A. Satoor [ https://svn.apache.org/r1498173 ] DERBY-5680 (indexStat daemon processing tables over and over even when there are no changes in the tables) KeepDisposableStatsPropertyTest test is meant for DERBY-3790 which is not in in 10.8 codeline and hence should not be run in 10.8
          Hide
          ASF subversion and git services added a comment -

          Commit 1497868 from Mamta A. Satoor
          [ https://svn.apache.org/r1497868 ]

          DERBY-5680( indexStat daemon processing tables over and over even when there are no changes in the tables )

          Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes.

          The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are
          1)DERBY-5681(When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3
          2)DERBY-3790(Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher

          A junit test was added for this new property but it went in as part of DERBY-3790. The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.

          Show
          ASF subversion and git services added a comment - Commit 1497868 from Mamta A. Satoor [ https://svn.apache.org/r1497868 ] DERBY-5680 ( indexStat daemon processing tables over and over even when there are no changes in the tables ) Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes. The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are 1) DERBY-5681 (When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3 2) DERBY-3790 (Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher A junit test was added for this new property but it went in as part of DERBY-3790 . The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.
          Hide
          ASF subversion and git services added a comment -

          Commit 1497868 from Mamta A. Satoor
          [ https://svn.apache.org/r1497868 ]

          DERBY-5680( indexStat daemon processing tables over and over even when there are no changes in the tables )

          Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes.

          The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are
          1)DERBY-5681(When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3
          2)DERBY-3790(Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher

          A junit test was added for this new property but it went in as part of DERBY-3790. The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.

          Show
          ASF subversion and git services added a comment - Commit 1497868 from Mamta A. Satoor [ https://svn.apache.org/r1497868 ] DERBY-5680 ( indexStat daemon processing tables over and over even when there are no changes in the tables ) Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes. The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are 1) DERBY-5681 (When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3 2) DERBY-3790 (Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher A junit test was added for this new property but it went in as part of DERBY-3790 . The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.
          Hide
          ASF subversion and git services added a comment -

          Commit 1497868 from Mamta A. Satoor
          [ https://svn.apache.org/r1497868 ]

          DERBY-5680( indexStat daemon processing tables over and over even when there are no changes in the tables )

          Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes.

          The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are
          1)DERBY-5681(When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3
          2)DERBY-3790(Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher

          A junit test was added for this new property but it went in as part of DERBY-3790. The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.

          Show
          ASF subversion and git services added a comment - Commit 1497868 from Mamta A. Satoor [ https://svn.apache.org/r1497868 ] DERBY-5680 ( indexStat daemon processing tables over and over even when there are no changes in the tables ) Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes. The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are 1) DERBY-5681 (When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3 2) DERBY-3790 (Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher A junit test was added for this new property but it went in as part of DERBY-3790 . The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.
          Hide
          ASF subversion and git services added a comment -

          Commit 1497868 from Mamta A. Satoor
          [ https://svn.apache.org/r1497868 ]

          DERBY-5680( indexStat daemon processing tables over and over even when there are no changes in the tables )

          Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes.

          The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are
          1)DERBY-5681(When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3
          2)DERBY-3790(Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher

          A junit test was added for this new property but it went in as part of DERBY-3790. The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.

          Show
          ASF subversion and git services added a comment - Commit 1497868 from Mamta A. Satoor [ https://svn.apache.org/r1497868 ] DERBY-5680 ( indexStat daemon processing tables over and over even when there are no changes in the tables ) Backporting the 3 commits that went in for DERBY-5680 to 10.8. The 3 commits were 1340549, 1341622, 1341629. The first two commits were easy to backport using svn merge command but the third commit 1341629 ran into conflicts. For that backport, hand made the changes since there were not too many changes. The changes for this jira has added a new property derby.storage.indexStats.debug.keepDisposableStats. The intention of the property is that if the property is set to true, we do not delete the orphaned/disposable stats. If the property is set to false, the orphaned/disposable stats will get dropped by the index stats daemon. Currently known reasons for orphaned/disposable stats are 1) DERBY-5681 (When a foreign key constraint on a table is dropped, the associated statistics row for the conglomerate is not removed). Fix for this has been backported all the way to 10.3 2) DERBY-3790 (Investigate if request for update statistics can be skipped for certain kind of indexes, one instance may be unique indexes based on one column.) Fix for this is in 10.9 and higher A junit test was added for this new property but it went in as part of DERBY-3790 . The name of the junit test is store.KeepDisposableStatsPropertyTest. Had to make changes to this test to backport it to 10.8 but without the fix for DEBRY-3790 and with the absence of drop statistics procedure, the test really does not make much sense for 10.8 codeline. The test uses drop statistics procedure and it is mainly testing DERBY-3790 to make sure that the orphaned stats are being deleted or left behind based on whether the property is set to true or false. But since we do not have drop statistics procedure and we do not have DERBY-3790 fixed in 10.8, we can't really meaningfully run the KeepDisposableStatsPropertyTest in 10.8. In any case, I have changed the test so that atleast it will not fail in 10.8 but it is not able to truly test the property. May be we can test this property through upgrade suite where we will create orphaned stats because of DERBY-5681 on older releases and we will find that when the property is set to true, even after upgrade, we will have orphaned stats but when property is set to false, after upgrade, orphaned stats are deleted.
          Hide
          Kristian Waagan added a comment -

          Committed patch 2a to trunk with revision 1341830.
          I have no more work planned for this issue, resolving.

          Show
          Kristian Waagan added a comment - Committed patch 2a to trunk with revision 1341830. I have no more work planned for this issue, resolving.
          Hide
          Kristian Waagan added a comment -

          Corrected typo in patch file name.
          Also added another comment in one of the tests.

          Show
          Kristian Waagan added a comment - Corrected typo in patch file name. Also added another comment in one of the tests.
          Hide
          Kristian Waagan added a comment -

          Dag, I've addressed most of your suggestions.
          Some comments below:

          • DiposableIndexStatistics#insertData
          • Added comments, skipped the reduced row count. This was a leftover from when I used far more rows in the tables.
          • Commented constants, they're used to get different numbers of unique values in various columns.
          • The sleeps ...
            I have tried to address the one which is most likely to fail intermittently by adding a new method to IndexStatsUtil: getNewStatsTable. It will first wait for the current stats to go away, then it expects to get the same number of updated statistics entries (with a timeout).
          • Call "JDBC.assertDrainResultsHasData...
            I removed this, since the checkpoint invocation is missing and the operation is not required for the test to work. Adding a checkpoint, you can observe the following change:

          Tue May 22 23:22:01 CEST 2012 Thread[TestRunner-Thread,5,main]

          {istat} "APP"."STAT_SCUI": update scheduled, reason=[t-est=1039, i-est=20 => cmp=3.9502817175452365] (queueSize=1)

          becomes (note t-est)

          Tue May 22 23:24:52 CEST 2012 Thread[TestRunner-Thread,5,main] {istat}

          "APP"."STAT_SCUI": update scheduled, reason=[t-est=2001, i-est=20 => cmp=4.605670061029743] (queueSize=1)

          I intend to commit this patch, which I hope is the last one, tomorrow.
          Patch ready for review.

          Show
          Kristian Waagan added a comment - Dag, I've addressed most of your suggestions. Some comments below: DiposableIndexStatistics#insertData Added comments, skipped the reduced row count. This was a leftover from when I used far more rows in the tables. Commented constants, they're used to get different numbers of unique values in various columns. The sleeps ... I have tried to address the one which is most likely to fail intermittently by adding a new method to IndexStatsUtil: getNewStatsTable. It will first wait for the current stats to go away, then it expects to get the same number of updated statistics entries (with a timeout). Call "JDBC.assertDrainResultsHasData... I removed this, since the checkpoint invocation is missing and the operation is not required for the test to work. Adding a checkpoint, you can observe the following change: Tue May 22 23:22:01 CEST 2012 Thread [TestRunner-Thread,5,main] {istat} "APP"."STAT_SCUI": update scheduled, reason= [t-est=1039, i-est=20 => cmp=3.9502817175452365] (queueSize=1) becomes (note t-est) Tue May 22 23:24:52 CEST 2012 Thread [TestRunner-Thread,5,main] {istat} "APP"."STAT_SCUI": update scheduled, reason= [t-est=2001, i-est=20 => cmp=4.605670061029743] (queueSize=1) I intend to commit this patch, which I hope is the last one, tomorrow. Patch ready for review.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Kristian! I have gone though 1b, and it looks good: +1

          Small (optional) comments:

          • DiposableIndexStatistics#insertData
          • "int reducedRowNumber = ROW_COUNT / 3": Would be nice to have a
            comment why this table receives fewer rows? Arbitrary or not?
            Same for ROW_COUNT really: does it need to be chosen within an
            interval currently for the test to work?
          • Magic constants: 2000, 19, 10?
          • DiposableIndexStatistics#createAndPopulateTables
          • space after "+" (several instances):
            Assert.assertTrue(stats.getStatsTable(tbl).length == preFkAddition +1);
          • The sleeps in KeepDiposableStatsPropertyTest and
            AutomaticIndexStatisticsTest#testNoUpdateTriggeredBySingleColumnUniqueIndex
            could be removed (future) if we augment the current
            WAIT_FOR_POST_COMMIT method to also wait for the istat daemon to
            complete its thing.
          • Call "JDBC.assertDrainResultsHasData(stmt.executeQuery("select
            count from " + TAB))" in KeepDiposableStatsPropertyTest might
            have a comment to say its there to trigger daemon to work.
          Show
          Dag H. Wanvik added a comment - Thanks, Kristian! I have gone though 1b, and it looks good: +1 Small (optional) comments: DiposableIndexStatistics#insertData "int reducedRowNumber = ROW_COUNT / 3": Would be nice to have a comment why this table receives fewer rows? Arbitrary or not? Same for ROW_COUNT really: does it need to be chosen within an interval currently for the test to work? Magic constants: 2000, 19, 10? DiposableIndexStatistics#createAndPopulateTables space after "+" (several instances): Assert.assertTrue(stats.getStatsTable(tbl).length == preFkAddition +1); The sleeps in KeepDiposableStatsPropertyTest and AutomaticIndexStatisticsTest#testNoUpdateTriggeredBySingleColumnUniqueIndex could be removed (future) if we augment the current WAIT_FOR_POST_COMMIT method to also wait for the istat daemon to complete its thing. Call "JDBC.assertDrainResultsHasData(stmt.executeQuery("select count from " + TAB))" in KeepDiposableStatsPropertyTest might have a comment to say its there to trigger daemon to work.
          Hide
          Kristian Waagan added a comment -

          Dag, I have changed the method name in version 1c of the patch.
          I committed it to trunk with revision 1341481.

          Sorry for committing it a bit early, but I chose to do this to get the code tested in tonight's test runs. At least one patch is outstanding, so I will address any comments you have on patch 1c in a separate patch.

          Show
          Kristian Waagan added a comment - Dag, I have changed the method name in version 1c of the patch. I committed it to trunk with revision 1341481. Sorry for committing it a bit early, but I chose to do this to get the code tested in tonight's test runs. At least one patch is outstanding, so I will address any comments you have on patch 1c in a separate patch.
          Hide
          Dag H. Wanvik added a comment -

          Looking at the new patch. Meanwhile, I forgot to mention that the name getIndexCount could be made descriptive: e.g. getQualifiedNumberOfIndexes

          Show
          Dag H. Wanvik added a comment - Looking at the new patch. Meanwhile, I forgot to mention that the name getIndexCount could be made descriptive: e.g. getQualifiedNumberOfIndexes
          Hide
          Kristian Waagan added a comment -

          Thanks for spotting that mistake, Knut Anders.
          I just uploaded a new version of the patch with the same name, but where I fixed the licsense header.

          Show
          Kristian Waagan added a comment - Thanks for spotting that mistake, Knut Anders. I just uploaded a new version of the patch with the same name, but where I fixed the licsense header.
          Hide
          Knut Anders Hatlen added a comment -

          You might want to update the class name in KeepDisposableStatsPropertyTest.java's license header too.

          Show
          Knut Anders Hatlen added a comment - You might want to update the class name in KeepDisposableStatsPropertyTest.java's license header too.
          Hide
          Kristian Waagan added a comment -

          suites.All (15729 tests) and derbyall (166 tests) passed.

          Show
          Kristian Waagan added a comment - suites.All (15729 tests) and derbyall (166 tests) passed.
          Hide
          Kristian Waagan added a comment -

          Thanks, Dag.

          I have addressed your two comments in patch 1b, and also added the tests. To keep the old behavior I had to modify FromBaseTable, this was missing in patch 1a.

          Patch ready from review.
          The test passed suites.All, but since I made one small modification I'm rerunning it now. I'll also run derbyall.

          Show
          Kristian Waagan added a comment - Thanks, Dag. I have addressed your two comments in patch 1b, and also added the tests. To keep the old behavior I had to modify FromBaseTable, this was missing in patch 1a. Patch ready from review. The test passed suites.All, but since I made one small modification I'm rerunning it now. I'll also run derbyall.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Kristian. The patch looks good to me. +1

          Optional nits:

          • in createindexconstantaction, the parameter to addStatistics is CardinalityCounter. This makes the calling site have to redo the call CardinalityCounter#getRowCount.
            Might as well do that before the call to addStatistics and pass in the long, since CardinalityCounter is not otherwise used inside addStatistics. This line could then be removed

          if (addStatistics(dd, indexRowGenerator, cCount))
          {
          long numRows = cCount.getRowCount(); <------
          into:

          long numRows = cCount.getRowCount;
          if (addStatistics(dd, irg, numRows) {
          long[] c = cCount.getCardinality();

          • The boolean tested for dd version is sometimes negative: dbIsPre10_9, other times positive: dd.checkVersion(DataDictionary.DD_VERSION_DERBY_10_9
            This is slightly confusing. Maybe we should stick to a positive semantic, i.e. "10_9 or newer"? It would also be more readable, e.g. here:

          // Skip single-column unique indexes unless we're told not to,
          // or we are running in soft-upgrade-mode on a pre 10.9 db.
          if (!dbIsPre10_9 && !FORCE_OLD_BEHAVIOR) { <-- note negation !dbIsPre10

          Show
          Dag H. Wanvik added a comment - Thanks, Kristian. The patch looks good to me. +1 Optional nits: in createindexconstantaction, the parameter to addStatistics is CardinalityCounter. This makes the calling site have to redo the call CardinalityCounter#getRowCount. Might as well do that before the call to addStatistics and pass in the long, since CardinalityCounter is not otherwise used inside addStatistics. This line could then be removed if (addStatistics(dd, indexRowGenerator, cCount)) { long numRows = cCount.getRowCount(); <------ into: long numRows = cCount.getRowCount; if (addStatistics(dd, irg, numRows) { long[] c = cCount.getCardinality(); The boolean tested for dd version is sometimes negative: dbIsPre10_9, other times positive: dd.checkVersion(DataDictionary.DD_VERSION_DERBY_10_9 This is slightly confusing. Maybe we should stick to a positive semantic, i.e. "10_9 or newer"? It would also be more readable, e.g. here: // Skip single-column unique indexes unless we're told not to, // or we are running in soft-upgrade-mode on a pre 10.9 db. if (!dbIsPre10_9 && !FORCE_OLD_BEHAVIOR) { <-- note negation !dbIsPre10
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1a, which adds the functionality to skip generating statistics for single-column unique indexes.

          Tests will be attached as a separate patch, as I'm waiting for a commit to avoid conflicts.

          • impl/sql/compile/FromBaseTable and
            iapi/sql/dictionary/TableDescriptor
            Added a new method to count indexes to TD, in which you can specify the minimum number of ordered column and whether non-unique indexes is exempt from that restriction.
            Used the new method to avoid scheduling work with the istat daemon for tables which has only one index and that index is a single-column unique index.
          • impl/sql/execute/CreateIndexConstantAction
            Added helper method addStatistics, which tells if statistics should be added for an index. The pre-10.9 behavior was to add statistics as long as there was at least one row in the index, the new behavior is to only add statistics if the index has more than one column or is non-unique.
            There's an override available in case this change causes the optimizer to generate bad/worse plans.
            Removed unused method statementExceptionCleanup.
          • impl/services/daemon/IndexStatisticsDaemonImpl
            Added code to skip generating statistics for single-column unique indexes.

          Patch ready for review.
          Patch 1a will be committed together with the test patch (not yet posted).

          Show
          Kristian Waagan added a comment - Attaching patch 1a, which adds the functionality to skip generating statistics for single-column unique indexes. Tests will be attached as a separate patch, as I'm waiting for a commit to avoid conflicts. impl/sql/compile/FromBaseTable and iapi/sql/dictionary/TableDescriptor Added a new method to count indexes to TD, in which you can specify the minimum number of ordered column and whether non-unique indexes is exempt from that restriction. Used the new method to avoid scheduling work with the istat daemon for tables which has only one index and that index is a single-column unique index. impl/sql/execute/CreateIndexConstantAction Added helper method addStatistics, which tells if statistics should be added for an index. The pre-10.9 behavior was to add statistics as long as there was at least one row in the index, the new behavior is to only add statistics if the index has more than one column or is non-unique. There's an override available in case this change causes the optimizer to generate bad/worse plans. Removed unused method statementExceptionCleanup. impl/services/daemon/IndexStatisticsDaemonImpl Added code to skip generating statistics for single-column unique indexes. Patch ready for review. Patch 1a will be committed together with the test patch (not yet posted).
          Hide
          martin added a comment -

          comment from Mike Matrigali on #DERBY-3937:

          > The current implementations of indexes ... do not maintain an exact count of rows.

          Show
          martin added a comment - comment from Mike Matrigali on # DERBY-3937 : > The current implementations of indexes ... do not maintain an exact count of rows.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Mamta A. Satoor
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development