Derby
  1. Derby
  2. DERBY-4685

Dead/unreachable code in OpenConglomerate.lockPositionForWrite()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.7.1.1
    • Component/s: Store
    • Labels:
      None

      Description

      OpenConglomerate.lockPositionForWrite() contains this code twice:

      if (!waitForLock)

      { // throw lock timeout error. throw StandardException.newException(SQLState.LOCK_TIMEOUT); }

      The second occurrence of this code can never end up throwing an exception, since waitForLock is guaranteed to be true there because of the identical check a couple of lines above. (Judging by the similar method lockPositionForRead(), it is probably the first check that should be removed, so that the latch on the page is released before the exception is thrown.)

      Also, the method is always called with forInsert==false, so the forInsert parameter can be removed. (I also suspect that the method doesn't work correctly if ever used in an insert operation, since it calls latchPage(RowPosition) which will unlatch the page if the row isn't found on the page, and I assume that a row that is about to be inserted does not exist yet.)

      1. d4685.diff
        0.8 kB
        Knut Anders Hatlen
      2. d4685-2.diff
        3 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Here's a patch that removes the first check for !waitForLock, so that the latch release code isn't hidden. The second identical check right after latch release will make sure that the exception is still raised.

        All the regression tests ran cleanly with the patch.

        Show
        Knut Anders Hatlen added a comment - Here's a patch that removes the first check for !waitForLock, so that the latch release code isn't hidden. The second identical check right after latch release will make sure that the exception is still raised. All the regression tests ran cleanly with the patch.
        Hide
        Knut Anders Hatlen added a comment -

        Committed revision 1028639.

        Show
        Knut Anders Hatlen added a comment - Committed revision 1028639.
        Hide
        Knut Anders Hatlen added a comment -

        Attaching a patch (#2) that addresses the other issue mentioned in the description. The patch removes the second parameter (forInsert) in lockPositionForWrite() because it is always false in the current code, and because the method is not believed to work for inserts anyhow.

        Show
        Knut Anders Hatlen added a comment - Attaching a patch (#2) that addresses the other issue mentioned in the description. The patch removes the second parameter (forInsert) in lockPositionForWrite() because it is always false in the current code, and because the method is not believed to work for inserts anyhow.
        Hide
        Knut Anders Hatlen added a comment -

        Regression tests ran cleanly with the #2 patch.

        Show
        Knut Anders Hatlen added a comment - Regression tests ran cleanly with the #2 patch.
        Hide
        Kristian Waagan added a comment -

        Looks good to me, +1 to commit.

        Show
        Kristian Waagan added a comment - Looks good to me, +1 to commit.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Kristian.
        Committed revision 1028712.

        Show
        Knut Anders Hatlen added a comment - Thanks, Kristian. Committed revision 1028712.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development