Derby
  1. Derby
  2. DERBY-2380

A statement plan holds onto resources such as its generated class even after it has been invalidated.

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 10.0.2.0, 10.0.2.1, 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6, 10.2.2.0, 10.3.1.4
    • Fix Version/s: None
    • Component/s: SQL
    • Urgency:
      Normal

      Description

      An internal plan (instance of GenericPreparedStatement) can be invalidated by other SQL operations such as DROP TABLE or DROP INDEX.
      When this happens the references to objects that are no longer useful such as the generated class and saved objects are held onto and thus use memory.
      If the statement is re-compiled then these objects will be handled by garbage collection.

      If the statement is not recompiled though, then these objects will remain until the plan (GenericPreparedStatement) is garbage collected.
      The plan being garbage collected can be held up for two reasons:
      1) The plan is in the statement cache. Note that only in some cases does it make sense to remove an invalid plan from the statement cache, e.g. a DROP TABLE should remove any plan that uses that table, but a DROP TRIGGER should not remove an INSERT from the cache, as it is likely the plan will be re-used and re-compiled. This is a separate issue given that the memory usage can occur even if the plan is not in the cache.
      2) The application holds onto a JDBC PreparedStatement that uses the plan.

      Given an application should not be able to affect memory usage like this then the GenericPreparedStatement.makeInvalid() call should null out fields that hold references to objects that have become invalid.

        Issue Links

          Activity

          Hide
          ASF subversion and git services added a comment -

          Commit 1566635 from Knut Anders Hatlen in branch 'code/trunk'
          [ https://svn.apache.org/r1566635 ]

          DERBY-2380: Make the generated class eligible for gc once the statement is invalidated

          Show
          ASF subversion and git services added a comment - Commit 1566635 from Knut Anders Hatlen in branch 'code/trunk' [ https://svn.apache.org/r1566635 ] DERBY-2380 : Make the generated class eligible for gc once the statement is invalidated
          Hide
          Knut Anders Hatlen added a comment -

          One possible incremental improvement is to clear the activation class in GenericPreparedStatement when the statement is invalidated. GenericStatement does this before repreparing a GPS, so the code is already prepared to handle the case where the activation class is null.

          The attached patch d2380-clear-activation-class.diff makes this change, and also refactors the code so that it's shared between reprepare and invalidate.

          All regression tests ran cleanly. I also hand-tested to see that the activation classes of invalidated statements could be garbage collected immediately. Without the patch, one had to wait until they had either been recompiled or evicted from the statement cache before they could be garbage collected.

          Show
          Knut Anders Hatlen added a comment - One possible incremental improvement is to clear the activation class in GenericPreparedStatement when the statement is invalidated. GenericStatement does this before repreparing a GPS, so the code is already prepared to handle the case where the activation class is null. The attached patch d2380-clear-activation-class.diff makes this change, and also refactors the code so that it's shared between reprepare and invalidate. All regression tests ran cleanly. I also hand-tested to see that the activation classes of invalidated statements could be garbage collected immediately. Without the patch, one had to wait until they had either been recompiled or evicted from the statement cache before they could be garbage collected.
          Hide
          Rick Hillegas added a comment -

          Triaged for 10.5.2: assigned normal urgency.

          Show
          Rick Hillegas added a comment - Triaged for 10.5.2: assigned normal urgency.
          Hide
          Rick Hillegas added a comment -

          Unassigning this issue since there has been no activity in more than a year.

          Show
          Rick Hillegas added a comment - Unassigning this issue since there has been no activity in more than a year.
          Hide
          Daniel John Debrunner added a comment -

          Yes - XCL07 can be removed, wasn't sure how to do it at the time with the recent change to (half) an xml format.

          Show
          Daniel John Debrunner added a comment - Yes - XCL07 can be removed, wasn't sure how to do it at the time with the recent change to (half) an xml format.
          Hide
          Mamta A. Satoor added a comment -

          Dan, one of the checkins that went for this Jira entry is 516286 and it says that there is no such state as a closed cursor, only open or non-existent. Does that mean that we don't need the SQLState XCL07 (Cursor '

          {0}

          ' is closed. Verify that autocommit is OFF.) anymore?

          Show
          Mamta A. Satoor added a comment - Dan, one of the checkins that went for this Jira entry is 516286 and it says that there is no such state as a closed cursor, only open or non-existent. Does that mean that we don't need the SQLState XCL07 (Cursor ' {0} ' is closed. Verify that autocommit is OFF.) anymore?
          Hide
          Daniel John Debrunner added a comment -

          In looking more at this issue (along with DERBY-2397) I think the dependency manager would be much cleaner if only persistent objects could be Providers.
          Persistent objects are much better suited to the DependencyManager because there is a definite termination of the object, the DROP statement.

          This would mean GenericPreparedStatement (GPS) would not be a Provider, i.e. no -one could create a dependency on a compiled plan. This causes problems because now some action must be taken when a GPS is no longer in use, to invalidate anything that depends on it. Not performing the invalidation would lead to a memory leak in the dependency manager. This need to know who is using a GPS has lead to the usage count, the partially valid state, generally not a clean way of handling its lifecycle. Not having GPS be a Provider would mean GPS would be like a typical java object, having a reference to the object allows it to be used.
          GPS is also (I think) the only non-persistent object that is a Provider.

          The only case where one plan depends on another today is when a positioned update/delete plan (GPS) depends on the plan (GPS) for the open cursor.

          I don't think this dependency is needed because the positioned update/delete will depend on the table being modified during its compilation.
          Thus if the cursor changes due to any change in the base table, then the positioned statement will be invalidated anyway.
          The positioned code already has a different mechanism to handle when the cursor changes to a different plan (which isn't triggered by an invalidation on the original cursor, since the original cursor plan may still be valid).
          Cursor change/invalidations are tested for in CurrentOfTest. I also added some new test fixtures to cover additional situations where the positioned statement needs to be invalidated or work against a different cursor.

          I have changes where this GenericPreparedStatement is no longer implements Provider and thus the positioned update/delete - cursor dependency does not exist. This basically works though various errors change from "cursor not found" to "cursor is closed" and vice-versa. This would be a step to cleanup the life-cycle state of GenericPreparedStatement, thus leading to being able to null out its compile objects once it becomes invalid (ie. current plan is invalid but the object could be reprepared to make it valid again).

          The change in errors is interesting, it's basically because the old code always threw 'cursor is closed' at runtime if the positioned update/delete could not find a matching cursor (having successfully compiled against one). I'm not sure this is correct, I tried to mimic the old behaviour by throwing 'cursor not found' if the connection does not have any open activations with that name, and 'cursor is closed' if the connection has an open activation (ie. java.sql.PreparedStatement) with that name, but no open result set. But I'm not sure if that is valid, one viewpoint could be that if there is no open cursor then the cursor doesn't exist and thus there is no such error as 'cursor is closed'. I couldn't see from the SQL spec any specific guidance on this (looking at DECLARE/OPEN and positioned UPDATE & DELETE), if anyone has any thoughts ...

          Show
          Daniel John Debrunner added a comment - In looking more at this issue (along with DERBY-2397 ) I think the dependency manager would be much cleaner if only persistent objects could be Providers. Persistent objects are much better suited to the DependencyManager because there is a definite termination of the object, the DROP statement. This would mean GenericPreparedStatement (GPS) would not be a Provider, i.e. no -one could create a dependency on a compiled plan. This causes problems because now some action must be taken when a GPS is no longer in use, to invalidate anything that depends on it. Not performing the invalidation would lead to a memory leak in the dependency manager. This need to know who is using a GPS has lead to the usage count, the partially valid state, generally not a clean way of handling its lifecycle. Not having GPS be a Provider would mean GPS would be like a typical java object, having a reference to the object allows it to be used. GPS is also (I think) the only non-persistent object that is a Provider. The only case where one plan depends on another today is when a positioned update/delete plan (GPS) depends on the plan (GPS) for the open cursor. I don't think this dependency is needed because the positioned update/delete will depend on the table being modified during its compilation. Thus if the cursor changes due to any change in the base table, then the positioned statement will be invalidated anyway. The positioned code already has a different mechanism to handle when the cursor changes to a different plan (which isn't triggered by an invalidation on the original cursor, since the original cursor plan may still be valid). Cursor change/invalidations are tested for in CurrentOfTest. I also added some new test fixtures to cover additional situations where the positioned statement needs to be invalidated or work against a different cursor. I have changes where this GenericPreparedStatement is no longer implements Provider and thus the positioned update/delete - cursor dependency does not exist. This basically works though various errors change from "cursor not found" to "cursor is closed" and vice-versa. This would be a step to cleanup the life-cycle state of GenericPreparedStatement, thus leading to being able to null out its compile objects once it becomes invalid (ie. current plan is invalid but the object could be reprepared to make it valid again). The change in errors is interesting, it's basically because the old code always threw 'cursor is closed' at runtime if the positioned update/delete could not find a matching cursor (having successfully compiled against one). I'm not sure this is correct, I tried to mimic the old behaviour by throwing 'cursor not found' if the connection does not have any open activations with that name, and 'cursor is closed' if the connection has an open activation (ie. java.sql.PreparedStatement) with that name, but no open result set. But I'm not sure if that is valid, one viewpoint could be that if there is no open cursor then the cursor doesn't exist and thus there is no such error as 'cursor is closed'. I couldn't see from the SQL spec any specific guidance on this (looking at DECLARE/OPEN and positioned UPDATE & DELETE), if anyone has any thoughts ...
          Hide
          Daniel John Debrunner added a comment -

          A GenericPreparedStatement instance can used in various situations and the resuling handling of its state in these situations is awkward.
          The various situations are in the statement cache and unused, in the cache and in use, referenced in a JDBC prepared statement with an activation, attached to an SPSDescriptor without an activation, in-use in a trigger.
          The state handling involves a use-count (for existing activations), the check to see if the plan is being cache and the "partially valid" state for the SPS mentioned in the previous comment.

          Ideally just having a reference to a GenericPreparedStatement would keep it valid.

          Looking at ways to clean this up it seems that the root cause is the DependencyManager. The DependencyManager is implementing the Observer pattern but in a non-java way.
          http://www.research.umbc.edu/~tarr/dp/lectures/Observer.pdf

          Since the GenericPreparedStatement instance is held onto in a central DependencyManager object, then some state is needed as to when it should be removed from that list,handling dependencies the "java way" would instead mean the list of dependents is held in the object itself, not a central list.
          Now the Java Observable class has some issues, but I wonder if the pattern should be applied here. Persistent dependencies cause some issues but I just wanted to throw my thoughts out there.

          Show
          Daniel John Debrunner added a comment - A GenericPreparedStatement instance can used in various situations and the resuling handling of its state in these situations is awkward. The various situations are in the statement cache and unused, in the cache and in use, referenced in a JDBC prepared statement with an activation, attached to an SPSDescriptor without an activation, in-use in a trigger. The state handling involves a use-count (for existing activations), the check to see if the plan is being cache and the "partially valid" state for the SPS mentioned in the previous comment. Ideally just having a reference to a GenericPreparedStatement would keep it valid. Looking at ways to clean this up it seems that the root cause is the DependencyManager. The DependencyManager is implementing the Observer pattern but in a non-java way. http://www.research.umbc.edu/~tarr/dp/lectures/Observer.pdf Since the GenericPreparedStatement instance is held onto in a central DependencyManager object, then some state is needed as to when it should be removed from that list,handling dependencies the "java way" would instead mean the list of dependents is held in the object itself, not a central list. Now the Java Observable class has some issues, but I wonder if the pattern should be applied here. Persistent dependencies cause some issues but I just wanted to throw my thoughts out there.
          Hide
          Daniel John Debrunner added a comment -

          First, some cleanup is needed for stored prepared statements (which are used for JDBC meta-data queries and triggers).
          ExecSPSNode has this code (generate)

          /*

            • The following does a prepare on the underlying
            • statement if necessary. The returned statement
            • is valid and its class is loaded up.
              */
              ps = spsd.getPreparedStatement();

          However the returned statement is "partially valid", it's isValid field is set to false because SPSDescriptor performed a makeInvalid() on it when it compiles the stored plan the first time. SPSDescriptor then holds onto this "partially valid" to provide the compiled information for the real running of the statement through the internal EXECUTE STATEMENT command. Applying the obvious fix for this bug, nulling out the compiled information, then leaves the prepared statement used by SPS in a fully invalid state, causing NullPointerExceptions as all the compiled information has been removed.

          Having a "partially valid" state is not a good design, it complicates the already hard to understand SPS code. Will be looking at ways to avoid this so that a makeInvalid() on a language prepared statement (plan) can always make the object invalid.

          Show
          Daniel John Debrunner added a comment - First, some cleanup is needed for stored prepared statements (which are used for JDBC meta-data queries and triggers). ExecSPSNode has this code (generate) /* The following does a prepare on the underlying statement if necessary. The returned statement is valid and its class is loaded up. */ ps = spsd.getPreparedStatement(); However the returned statement is "partially valid", it's isValid field is set to false because SPSDescriptor performed a makeInvalid() on it when it compiles the stored plan the first time. SPSDescriptor then holds onto this "partially valid" to provide the compiled information for the real running of the statement through the internal EXECUTE STATEMENT command. Applying the obvious fix for this bug, nulling out the compiled information, then leaves the prepared statement used by SPS in a fully invalid state, causing NullPointerExceptions as all the compiled information has been removed. Having a "partially valid" state is not a good design, it complicates the already hard to understand SPS code. Will be looking at ways to avoid this so that a makeInvalid() on a language prepared statement (plan) can always make the object invalid.

            People

            • Assignee:
              Unassigned
              Reporter:
              Daniel John Debrunner
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development