Derby
  1. Derby
  2. DERBY-4563

Avoid unnecessary use of getStream and getStreamWithDescriptor

    Details

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

      Description

      A pattern in Derby is to use the following piece of code to determine if the data value has a stream or not:
      if (dvd.getStream() != null) ...

      Since the stream has mutable state, obtaining a reference to it just to check if it is not null is sub-optimal for several reasons:

      • it may throw an exception (data types not supporting streams)
      • the stream reference is leaked, which is unfortunately if we are / will be required to guarantee something about the stream state (for instance that the descriptor is in sync with the stream).
      • in cases where we have to investigate the state of the stream, we're doing unnecessary work
      • makes it harder to write debug code (i.e., is a stream reference obtained from the data value descriptor more than once?)

      I plan to introduce the method DataValueDescriptor.hasStream, returning a boolean.
      In addition to the obvious check if the stream variable is non-null, it can also be used to instruct Derby to treat certain data values as non-streams even though the underlying value is currently a stream. One example is CHAR and VARCHAR, whose maximum lengths are so small that they should always be materialized to avoid the added complexity coming with streams (stream state, isolation levels - extra lock to keep stream stable?, cloning).

      1. derby-4563-1a-dvd_hasStream.diff
        21 kB
        Kristian Waagan
      2. derby-4563-1a-dvd_hasStream.stat
        1.0 kB
        Kristian Waagan
      3. derby-4563-1b-dvd_hasStream.diff
        21 kB
        Kristian Waagan
      4. derby-4563-2a-replace_streamstorable_in_jdbclayer.diff
        2 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        Attached patch 1a.

        The most notable change is that Derby will thrown an exception if a call to getStream or getStreamWithDescriptor is made on a data type that supports streams (i.e. SQLClob) but that doesn't have a stream. This indicates that hasStream wasn't called up front to see if the value should be represented as a stream or not.
        Note that streams for values going in to Derby (inserts) will be obtained through the Resetable-interface, not DataValueDescriptor.

        Regression tests passed, patch ready for review.

        Show
        Kristian Waagan added a comment - Attached patch 1a. The most notable change is that Derby will thrown an exception if a call to getStream or getStreamWithDescriptor is made on a data type that supports streams (i.e. SQLClob) but that doesn't have a stream. This indicates that hasStream wasn't called up front to see if the value should be represented as a stream or not. Note that streams for values going in to Derby (inserts) will be obtained through the Resetable-interface, not DataValueDescriptor. Regression tests passed, patch ready for review.
        Hide
        Dag H. Wanvik added a comment -

        Looks like a good cleanup to me. +1

        This comment might be made more clear: it was not obvious just by reading the code what optimization you are referring to here:

        + * Note that some data types may choose to return

        {@code false}

        here even
        + * if their source is a stream. This is an optimization.

        Show
        Dag H. Wanvik added a comment - Looks like a good cleanup to me. +1 This comment might be made more clear: it was not obvious just by reading the code what optimization you are referring to here: + * Note that some data types may choose to return {@code false} here even + * if their source is a stream. This is an optimization.
        Hide
        Kristian Waagan added a comment -

        Thanks for reviewing the patch, Dag.
        I decided to remove the unclear comment for now, as I realized it isn't entirely clear how this will play out yet. I do plan to revisit the issue (at latest when optimizing the cloneValue methods for BLOB/CLOB).
        I'm also having some doubts about the call to hasStream in getStream / getStreamWithDescriptor, and I did consider simply changing the implementation to do just 'stream != null'. Again, I'll have to revisit this when / if more substantial logic is added to the hasStream-method.

        Committed the newly attached patch 1b to trunk with revision 916261.

        I will post another patch shortly, replacing the use of StreamStorable in the JDBC-layer.

        Show
        Kristian Waagan added a comment - Thanks for reviewing the patch, Dag. I decided to remove the unclear comment for now, as I realized it isn't entirely clear how this will play out yet. I do plan to revisit the issue (at latest when optimizing the cloneValue methods for BLOB/CLOB). I'm also having some doubts about the call to hasStream in getStream / getStreamWithDescriptor, and I did consider simply changing the implementation to do just 'stream != null'. Again, I'll have to revisit this when / if more substantial logic is added to the hasStream-method. Committed the newly attached patch 1b to trunk with revision 916261. I will post another patch shortly, replacing the use of StreamStorable in the JDBC-layer.
        Hide
        Kristian Waagan added a comment -

        Patch 2a replaces the two occurrences of StreamStorable in the JDBC layer, using DVD.getStream() instead.
        I also observe that the use of setupContextStack() is inconsistent. In some places it is invoked regardless of whether the value is a stream or not, in other cases a stack is pushed only when the value is represented by a stream.

        I expect to commit this tomorrow, I'm just waiting for the regression tests to finish.

        Patch ready for review.

        Show
        Kristian Waagan added a comment - Patch 2a replaces the two occurrences of StreamStorable in the JDBC layer, using DVD.getStream() instead. I also observe that the use of setupContextStack() is inconsistent. In some places it is invoked regardless of whether the value is a stream or not, in other cases a stack is pushed only when the value is represented by a stream. I expect to commit this tomorrow, I'm just waiting for the regression tests to finish. Patch ready for review.
        Hide
        Kristian Waagan added a comment -

        Regression tests passed.

        Committed patch 2a to trunk with revision 916640.
        I don't plan to do more work on this issue.

        Show
        Kristian Waagan added a comment - Regression tests passed. Committed patch 2a to trunk with revision 916640. I don't plan to do more work on this issue.
        Hide
        Kristian Waagan added a comment -

        Closing.

        Show
        Kristian Waagan added a comment - Closing.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development