Derby
  1. Derby
  2. DERBY-1799

SUR: current row not locked when renavigating to it, in queries with indexes

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      Repro attached

      Description

      This problem is detected in transactions with isolation level read-committed/read-uncommitted.

      We have a table (T) which has a primary key (a), and a query which does "select A from T" (an indexed select)

      If the result set is scrollable updatable, we expect the current row to be locked with an update lock. This does not seem to happen when repositioning to a row which has been already been fetched previously.

      The result is that either the wrong row is locked, or if the result set has been on after last position, no row is locked.

      Output from ij:
      ij> get scroll insensitive cursor c1 as 'select a from t for update';
      ij> next c1;
      A
      -----------
      1
      ij> select * from SYSCS_DIAG.LOCK_TABLE;
      XID |TYPE |MODE|TABLENAME |LOCKNAME |STATE|TABLETYPE|LOCK&|INDEXNAME
      ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      243 |ROW |U |T |(1,7) |GRANT|T |1 |NULL
      243 |ROW |S |T |(1,1) |GRANT|T |1 |SQL060901103455010
      243 |TABLE|IX |T |Tablelock |GRANT|T |4 |NULL

      3 rows selected

      ij> after last c1;
      No current row
      ij> previous c1;
      A
      -----------
      3
      ij> select * from SYSCS_DIAG.LOCK_TABLE;
      XID |TYPE |MODE|TABLENAME |LOCKNAME |STATE|TABLETYPE|LOCK&|INDEXNAME
      ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      243 |TABLE|IX |T |Tablelock |GRANT|T |4 |NULL

      1 row selected

      The last select shows that no row is locked at this point, however we expect one row to be locked.

      1. derby-1799.diff
        36 kB
        Fernanda Pizzorno
      2. derby-1799.stat
        1 kB
        Fernanda Pizzorno

        Issue Links

          Activity

          Hide
          Fernanda Pizzorno added a comment -

          The attached patch (derby-1799.diff) implements a similar mechanism for BTreeScan to the positionAtRowLocation mechanism for HeapScan. As the ScrollInsensitiveResultSet is populated, the positionKey(key + rowLocation) of each row is stored in the HashTable. This positionKey is used when scrolling back to a provious visited row in order to position the BTreeScan, and set locks on the row if necessary.

          The following changes where made to the ScanController interface:

          • the method positionAtRowLocation(RowLocation) was changed to positionAtRow(DataValueDescriptor[]) so that it can be implemented both by HeapScan and BTreeScan.
          • a new method getScanPosition was added. This method is also implemented both for HeapScan abd BTreeScan. It returns an array of type DataValueDescriptior. This array has size 1 for HeapScan and contains the RowLocation, and contains the positionKey (key + RowLocation) for BTreeScan.

          The following changes where made to NoPutResultSet interface:

          • the method positionScanAtRowLocation(RowLocation) was changed to positionScanAtRow(DataValueDescriptor[]) so that it can be used both for RowLocation and positionKey.
          • a new method getScanPosition() was added. It returns an array of type DataValueDescriptior, and therefore can return either a RowLocation or positionKey depending on whether a HeapScan or BTreeScan is being used.

          A new test case was added to ConcurrencyTest. Derbyall is being run with this patch at the moment, and is has successfully run this patch + the patch for DERBY-1676. Can someone please review this patch?

          Show
          Fernanda Pizzorno added a comment - The attached patch (derby-1799.diff) implements a similar mechanism for BTreeScan to the positionAtRowLocation mechanism for HeapScan. As the ScrollInsensitiveResultSet is populated, the positionKey(key + rowLocation) of each row is stored in the HashTable. This positionKey is used when scrolling back to a provious visited row in order to position the BTreeScan, and set locks on the row if necessary. The following changes where made to the ScanController interface: the method positionAtRowLocation(RowLocation) was changed to positionAtRow(DataValueDescriptor[]) so that it can be implemented both by HeapScan and BTreeScan. a new method getScanPosition was added. This method is also implemented both for HeapScan abd BTreeScan. It returns an array of type DataValueDescriptior. This array has size 1 for HeapScan and contains the RowLocation, and contains the positionKey (key + RowLocation) for BTreeScan. The following changes where made to NoPutResultSet interface: the method positionScanAtRowLocation(RowLocation) was changed to positionScanAtRow(DataValueDescriptor[]) so that it can be used both for RowLocation and positionKey. a new method getScanPosition() was added. It returns an array of type DataValueDescriptior, and therefore can return either a RowLocation or positionKey depending on whether a HeapScan or BTreeScan is being used. A new test case was added to ConcurrencyTest. Derbyall is being run with this patch at the moment, and is has successfully run this patch + the patch for DERBY-1676 . Can someone please review this patch?
          Hide
          Mike Matrigali added a comment -

          I am planning on reviewing this change, but probably won't get to it until end of business monday 9/18 , PST. From the description I am a little worried that the wrong stuff is being pushed into the store, but
          won't know for sure until I can review.

          Show
          Mike Matrigali added a comment - I am planning on reviewing this change, but probably won't get to it until end of business monday 9/18 , PST. From the description I am a little worried that the wrong stuff is being pushed into the store, but won't know for sure until I can review.
          Hide
          Mike Matrigali added a comment -

          I don't understand why the new interfaces were added to the access interface. Positiioning by rowlocation in a btree is tricky as the row location for a given row can change anytime a split occurs. Handing that rowlocation out of store and expecting it to be useful in the future seems like a bad
          direction to take the code. I believe what should be happening is that the scan user should be maintaining the key of the record if they need to
          position to that place. Then existing interfaces already exist to reopen a scan by key.

          The postion a scan by RowLocation interfaces were added as an afterthought for heaps, basically to be used when doing probes from an index to heap, where it was known that the rowlocation was protectected by the lock held by the caller on the index row.

          Could you explain or provide a reference to how, before this change SUR expects to maintain the necessary locks. There may be a bug in locking in the existing reopen logic, I would like to better
          understand what store interface you are calling when "previous c1" is called and you expect a lock
          to be held in read uncommitted mode.

          The positionAtRow interface looks like it provides the same functionality as existing reopenScan() interface.

          Show
          Mike Matrigali added a comment - I don't understand why the new interfaces were added to the access interface. Positiioning by rowlocation in a btree is tricky as the row location for a given row can change anytime a split occurs. Handing that rowlocation out of store and expecting it to be useful in the future seems like a bad direction to take the code. I believe what should be happening is that the scan user should be maintaining the key of the record if they need to position to that place. Then existing interfaces already exist to reopen a scan by key. The postion a scan by RowLocation interfaces were added as an afterthought for heaps, basically to be used when doing probes from an index to heap, where it was known that the rowlocation was protectected by the lock held by the caller on the index row. Could you explain or provide a reference to how, before this change SUR expects to maintain the necessary locks. There may be a bug in locking in the existing reopen logic, I would like to better understand what store interface you are calling when "previous c1" is called and you expect a lock to be held in read uncommitted mode. The positionAtRow interface looks like it provides the same functionality as existing reopenScan() interface.
          Hide
          Fernanda Pizzorno added a comment -

          Thank you for reviewing the patch.

          > Mike Matrigali updated DERBY-1799:
          > ----------------------------------

          > I don't understand why the new interfaces were added to the access
          > interface. Positiioning by rowlocation in a btree is tricky as the
          > row location for a given row can change anytime a split occurs.
          > Handing that rowlocation out of store and expecting it to be useful
          > in the future seems like a bad direction to take the code. I believe
          > what should be happening is that the scan user should be maintaining
          > the key of the record if they need to position to that place. Then
          > existing interfaces already exist to reopen a scan by key.

          I don't understand your comment here. The changes to the interface
          were made in order to enable to scan user to mantain the key of the
          record and use it to position to that place.

          Before this patch SUR would store the rowLocation and use it to
          reposition to that place. This patch adds the following method to
          the ScanController interface:

          DataValueDescriptor[] getCurrentPosition() throws StandardException;

          The HeapScan implementation of this method will return the
          RowLocation (the logic for HeapScan has not been changed), and the
          BTreeScan implementation of this method will return the key of the
          record.

          This patch also changes the following method in the ScanController
          interface:

          • boolean positionAtRowLocation(RowLocation rl)
            + boolean positionAtRow(DataValueDescriptor[] rowPosition)
            throws StandardException;

          This has also been done to able the scan user to mantain the key of
          the record and use it to position back to the same place. The method
          was renamed and the parameter was changed from RowLocation to
          DataValueDescriptor[]. The HeapScan implementation of this method
          will still take the RowLocation as parameter (the only change is that
          the RowLocation will be in the first position of the array), and the
          BTreeScan implementation of this method will take the key of the
          record as paramenter and use that key to reposition.

          To sum up, I believe that we are actually doing what you suggested
          above. I don't think I am taking care of the case when one of the
          indexed columns is updated causing the key to change. I will look
          into that when I submit an updated patch.

          [...]

          > Could you explain or provide a reference to how, before this change
          > SUR expects to maintain the necessary locks. There may be a bug in
          > locking in the existing reopen logic, I would like to better
          > understand what store interface you are calling when "previous c1"
          > is called and you expect a lock to be held in read uncommitted mode.

          Before this change, when positioning to a previously visited row, if
          the scan was a HeapScan, SUR would use the method positionAtRowLocation
          to maintain locks, and for a BTreeScan nothing would be done. Locks
          where maintained correctly for HeapScan but not for BTreeScan. The aim
          of this patch is to fix this problem in BTreeScan. You can find more
          information about SUR in the write-up in DERBY-690.

          > The positionAtRow interface looks like it provides the same
          > functionality as existing reopenScan() interface.

          I agree that the new method reopenScanByPositionKeyAndSetLock does
          exactly the same as reopenScan. I will change that in a next patch.

          Show
          Fernanda Pizzorno added a comment - Thank you for reviewing the patch. > Mike Matrigali updated DERBY-1799 : > ---------------------------------- > I don't understand why the new interfaces were added to the access > interface. Positiioning by rowlocation in a btree is tricky as the > row location for a given row can change anytime a split occurs. > Handing that rowlocation out of store and expecting it to be useful > in the future seems like a bad direction to take the code. I believe > what should be happening is that the scan user should be maintaining > the key of the record if they need to position to that place. Then > existing interfaces already exist to reopen a scan by key. I don't understand your comment here. The changes to the interface were made in order to enable to scan user to mantain the key of the record and use it to position to that place. Before this patch SUR would store the rowLocation and use it to reposition to that place. This patch adds the following method to the ScanController interface: DataValueDescriptor[] getCurrentPosition() throws StandardException; The HeapScan implementation of this method will return the RowLocation (the logic for HeapScan has not been changed), and the BTreeScan implementation of this method will return the key of the record. This patch also changes the following method in the ScanController interface: boolean positionAtRowLocation(RowLocation rl) + boolean positionAtRow(DataValueDescriptor[] rowPosition) throws StandardException; This has also been done to able the scan user to mantain the key of the record and use it to position back to the same place. The method was renamed and the parameter was changed from RowLocation to DataValueDescriptor[]. The HeapScan implementation of this method will still take the RowLocation as parameter (the only change is that the RowLocation will be in the first position of the array), and the BTreeScan implementation of this method will take the key of the record as paramenter and use that key to reposition. To sum up, I believe that we are actually doing what you suggested above. I don't think I am taking care of the case when one of the indexed columns is updated causing the key to change. I will look into that when I submit an updated patch. [...] > Could you explain or provide a reference to how, before this change > SUR expects to maintain the necessary locks. There may be a bug in > locking in the existing reopen logic, I would like to better > understand what store interface you are calling when "previous c1" > is called and you expect a lock to be held in read uncommitted mode. Before this change, when positioning to a previously visited row, if the scan was a HeapScan, SUR would use the method positionAtRowLocation to maintain locks, and for a BTreeScan nothing would be done. Locks where maintained correctly for HeapScan but not for BTreeScan. The aim of this patch is to fix this problem in BTreeScan. You can find more information about SUR in the write-up in DERBY-690 . > The positionAtRow interface looks like it provides the same > functionality as existing reopenScan() interface. I agree that the new method reopenScanByPositionKeyAndSetLock does exactly the same as reopenScan. I will change that in a next patch.
          Hide
          Knut Anders Hatlen added a comment -

          Triaged for 10.5.2.

          Show
          Knut Anders Hatlen added a comment - Triaged for 10.5.2.

            People

            • Assignee:
              Unassigned
              Reporter:
              Andreas Korneliussen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development