Derby
  1. Derby
  2. DERBY-2641

Convert lang/staleplans.sql to JUnit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None
    1. derby-2641-1.diff
      14 kB
      Knut Anders Hatlen
    2. derby-2641-1.stat
      0.2 kB
      Knut Anders Hatlen

      Activity

      Hide
      Knut Anders Hatlen added a comment -

      The attached patch adds a JUnit test (StalePlansTest) which tests the same things as lang/staleplans.sql. The patch does not remove the old test.

      One thing that puzzled me with the original test, was that three of the calls to SYSCS_GETRUNTIMESTATISTICS() had comments which said they expected table scans, but the query plans in the master file showed that index scans were used. For now, I left the comments which said we expected table scans and added a note saying that we actually get index scans.

      For those interested, we get the unexpected query plans on the first, second and fourth call to SYSCS_GETRUNTIMESTATISTICS() in the old staleplans.sql test. In the new JUnit test, all the queries with unexpected query plans are found in testStalePlansOnSmallTable(). I would say it is reasonable to expect a table scan in these cases since the table only contains one row, but I don't know enough about this area to say whether it's a bug. If someone thinks it's a bug, I will file a JIRA for it.

      Show
      Knut Anders Hatlen added a comment - The attached patch adds a JUnit test (StalePlansTest) which tests the same things as lang/staleplans.sql. The patch does not remove the old test. One thing that puzzled me with the original test, was that three of the calls to SYSCS_GETRUNTIMESTATISTICS() had comments which said they expected table scans, but the query plans in the master file showed that index scans were used. For now, I left the comments which said we expected table scans and added a note saying that we actually get index scans. For those interested, we get the unexpected query plans on the first, second and fourth call to SYSCS_GETRUNTIMESTATISTICS() in the old staleplans.sql test. In the new JUnit test, all the queries with unexpected query plans are found in testStalePlansOnSmallTable(). I would say it is reasonable to expect a table scan in these cases since the table only contains one row, but I don't know enough about this area to say whether it's a bug. If someone thinks it's a bug, I will file a JIRA for it.
      Hide
      A B added a comment -

      > I would say it is reasonable to expect a table scan in these cases since the table only contains one row, but
      > I don't know enough about this area to say whether it's a bug. If someone thinks it's a bug, I will file a JIRA
      > for it.

      I took a quick look at this and noticed the following comment in the "estimateCost(...)" method of FromBaseTable:

      /* we prefer index than table scan for concurrency reason, by a small

      • adjustment on estimated row count. This affects optimizer's decision
      • especially when few rows are in table. beetle 5006. This makes sense
      • since the plan may stay long before we actually check and invalidate it.
      • And new rows may be inserted before we check and invalidate the plan.
      • Here we only prefer index that has start/stop key from predicates. Non-
      • constant start/stop key case is taken care of by selectivity later.
        */

      I think this why the optimizer is choosing an index scan. It seems intentional, so my guess is that this is not a bug. Probably the case that the comments in statelplans.sql were not updated when the above comment (and associated code) was added...?

      Show
      A B added a comment - > I would say it is reasonable to expect a table scan in these cases since the table only contains one row, but > I don't know enough about this area to say whether it's a bug. If someone thinks it's a bug, I will file a JIRA > for it. I took a quick look at this and noticed the following comment in the "estimateCost(...)" method of FromBaseTable: /* we prefer index than table scan for concurrency reason, by a small adjustment on estimated row count. This affects optimizer's decision especially when few rows are in table. beetle 5006. This makes sense since the plan may stay long before we actually check and invalidate it. And new rows may be inserted before we check and invalidate the plan. Here we only prefer index that has start/stop key from predicates. Non- constant start/stop key case is taken care of by selectivity later. */ I think this why the optimizer is choosing an index scan. It seems intentional, so my guess is that this is not a bug. Probably the case that the comments in statelplans.sql were not updated when the above comment (and associated code) was added...?
      Hide
      Knut Anders Hatlen added a comment -

      Thanks for investigating this, Army! It's amazing how you could dig up that comment from the middle of a method that spans 1000 lines!
      I will update the comments in the test to make it clear that the index scans are intended.

      Show
      Knut Anders Hatlen added a comment - Thanks for investigating this, Army! It's amazing how you could dig up that comment from the middle of a method that spans 1000 lines! I will update the comments in the test to make it clear that the index scans are intended.
      Hide
      A B added a comment -

      > I will update the comments in the test to make it clear that the index scans are intended.

      Next question then would be: is the test still testing what it's trying test? I.e. if it uses an index scan with 1 row and it uses an index scan with 10 rows (I think that's what it's doing), then will we be able to see the changed (or not changed) plans as the test expects? I haven't looked in detail, it's just something that occurred to me while scanning the old .sql script...

      Show
      A B added a comment - > I will update the comments in the test to make it clear that the index scans are intended. Next question then would be: is the test still testing what it's trying test? I.e. if it uses an index scan with 1 row and it uses an index scan with 10 rows (I think that's what it's doing), then will we be able to see the changed (or not changed) plans as the test expects? I haven't looked in detail, it's just something that occurred to me while scanning the old .sql script...
      Hide
      Knut Anders Hatlen added a comment -

      > Next question then would be: is the test still testing what it's trying test?

      No, the first part of the test (testStalePlansOnSmallTable() in the JUnit test) does not change the plans. The second part (testStalePlansOnLargeTable()) does however change plans exactly as described in the comments, so I think we are covered there. I don't have any good ideas for how to make the small table test change plans (other than increasing the size of the table, which makes the test more or less identical to the large table test). Do you have any ideas? We could of course leave the test as it is and explain in the comments that the first test case no longer tests stale plans.

      Show
      Knut Anders Hatlen added a comment - > Next question then would be: is the test still testing what it's trying test? No, the first part of the test (testStalePlansOnSmallTable() in the JUnit test) does not change the plans. The second part (testStalePlansOnLargeTable()) does however change plans exactly as described in the comments, so I think we are covered there. I don't have any good ideas for how to make the small table test change plans (other than increasing the size of the table, which makes the test more or less identical to the large table test). Do you have any ideas? We could of course leave the test as it is and explain in the comments that the first test case no longer tests stale plans.
      Hide
      A B added a comment -

      > Do you have any ideas?

      Nothing comes to mind, no. The easiest way to change a plan is via DDL (ex. add/drop an index), but I think those will invalidate a plan, which would defeat the purpose of the stale plans test.

      > We could of course leave the test as it is and explain in the comments that the first test case no
      > longer tests stale plans.

      Given that the goal of this issue is to "convert" the test and not necessarily to "fix" it, I think this is fine. If further work is required to make the first part of the test more meaningful, that could easily be a separate Jira...

      Show
      A B added a comment - > Do you have any ideas? Nothing comes to mind, no. The easiest way to change a plan is via DDL (ex. add/drop an index), but I think those will invalidate a plan, which would defeat the purpose of the stale plans test. > We could of course leave the test as it is and explain in the comments that the first test case no > longer tests stale plans. Given that the goal of this issue is to "convert" the test and not necessarily to "fix" it, I think this is fine. If further work is required to make the first part of the test more meaningful, that could easily be a separate Jira...
      Hide
      Mike Matrigali added a comment -

      The intent in derby is definitely to try to pick index scan over table scan for tables with small numbers of rows - EVEN if the performance might be better doing the table scan. The actual use problems were:
      1) when you pick heap vs. index scan depending on isolation level often one had to set a TABLE lock rather then individual row locks,
      this is a particular problem with serializable isolation.
      2) more subtle is that if the index scan eliminates rows then we lock less rows than the heap case where minimally we will row lock
      every row of the heap, this is especially problem with repeatable read and serializable.
      Especially in case where derby needed to pick table locking to do correct isolation level customers were seeing applications fail
      on derby which would run otherwise unchanged. Also this behavior met what most customers expected. They created an index,
      the query used the index.

      This is a case where we choose to weigh concurrency over execution cost in the cost based optimizer. The hope is:
      1) usually small number of row table performance does not matter.
      2) often the small number of rows is actually growing but we happen to compile at app startup.

      Show
      Mike Matrigali added a comment - The intent in derby is definitely to try to pick index scan over table scan for tables with small numbers of rows - EVEN if the performance might be better doing the table scan. The actual use problems were: 1) when you pick heap vs. index scan depending on isolation level often one had to set a TABLE lock rather then individual row locks, this is a particular problem with serializable isolation. 2) more subtle is that if the index scan eliminates rows then we lock less rows than the heap case where minimally we will row lock every row of the heap, this is especially problem with repeatable read and serializable. Especially in case where derby needed to pick table locking to do correct isolation level customers were seeing applications fail on derby which would run otherwise unchanged. Also this behavior met what most customers expected. They created an index, the query used the index. This is a case where we choose to weigh concurrency over execution cost in the cost based optimizer. The hope is: 1) usually small number of row table performance does not matter. 2) often the small number of rows is actually growing but we happen to compile at app startup.
      Hide
      Knut Anders Hatlen added a comment -

      Thanks Army and Mike for explaining this. I updated the comments and committed revision 538441.

      Show
      Knut Anders Hatlen added a comment - Thanks Army and Mike for explaining this. I updated the comments and committed revision 538441.
      Hide
      Knut Anders Hatlen added a comment -

      Removed the old test (revision 538444).

      Show
      Knut Anders Hatlen added a comment - Removed the old test (revision 538444).

        People

        • Assignee:
          Knut Anders Hatlen
          Reporter:
          Knut Anders Hatlen
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development