Derby
  1. Derby
  2. DERBY-4822

DropSchemaConstantAction: could reuse the current connection provided by the available activation object

    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: SQL
    • Labels:
      None
    • Issue & fix info:
      Newcomer

      Description

      In DropSchemaConstantAction.executeConstantAction, getSchemaDescriptor is called with a null parameter. Instead, one could reuse the current transaction controller directly available with "activation.getLanguageConnectionContext().getTransactionExecute()" as it's done with other subclasses of DDLConstantAction.

      Regards, --Martin

      Index: java/engine/org/apache/derby/impl/sql/execute/DropSchemaConstantAction.java
      ===================================================================
      — java/engine/org/apache/derby/impl/sql/execute/DropSchemaConstantAction.java (revision 1001658)
      +++ java/engine/org/apache/derby/impl/sql/execute/DropSchemaConstantAction.java (working copy)
      @@ -27,6 +27,7 @@
      import org.apache.derby.iapi.sql.dictionary.DataDictionary;
      import org.apache.derby.iapi.sql.dictionary.SchemaDescriptor;
      import org.apache.derby.iapi.sql.execute.ConstantAction;
      +import org.apache.derby.iapi.store.access.TransactionController;

      /**

      • This class describes actions that are ALWAYS performed for a
        @@ -82,6 +83,7 @@
        {
        LanguageConnectionContext lcc = activation.getLanguageConnectionContext();
        DataDictionary dd = lcc.getDataDictionary();
        + TransactionController tc = lcc.getTransactionExecute();

      /*

        • Inform the data dictionary that we are about to write to it.
          @@ -94,7 +96,7 @@
          */
          dd.startWriting(lcc);
      • SchemaDescriptor sd = dd.getSchemaDescriptor(schemaName, null, true);
        + SchemaDescriptor sd = dd.getSchemaDescriptor(schemaName, tc, true);

      sd.drop(lcc, activation);

      1. patch.txt
        1 kB
        Martin Monperrus

        Activity

        Martin Monperrus created issue -
        Hide
        Martin Monperrus added a comment -

        Hi,

        Is the patch correct?

        Thanks, --Martin

        Show
        Martin Monperrus added a comment - Hi, Is the patch correct? Thanks, --Martin
        Hide
        Kristian Waagan added a comment -

        Hi Martin,

        Thanks for providing the patch.
        It looks correct, but it's hard to say without further investigation. I'll apply the patch and run the tests as a first check.

        The potential difference introduced by the patch, is whether a nested transaction is used or not when accessing the data dictionary. This part of the API is confusing, as reported by Dan in DERBY-2490.
        I'm not 100% sure if it is necessary, but it would be nice if you could attach the patch as a file and tick the box for granting the license to ASF.

        Regards,

        Show
        Kristian Waagan added a comment - Hi Martin, Thanks for providing the patch. It looks correct, but it's hard to say without further investigation. I'll apply the patch and run the tests as a first check. The potential difference introduced by the patch, is whether a nested transaction is used or not when accessing the data dictionary. This part of the API is confusing, as reported by Dan in DERBY-2490 . I'm not 100% sure if it is necessary, but it would be nice if you could attach the patch as a file and tick the box for granting the license to ASF. Regards,
        Hide
        Kristian Waagan added a comment -

        derbyall and suites.All passed without failures with the patch applied.

        Show
        Kristian Waagan added a comment - derbyall and suites.All passed without failures with the patch applied.
        Hide
        Martin Monperrus added a comment -

        Hi Kristian,

        Thanks for the answer. Here is the patch as file+license granting.

        --Martin

        Show
        Martin Monperrus added a comment - Hi Kristian, Thanks for the answer. Here is the patch as file+license granting. --Martin
        Martin Monperrus made changes -
        Field Original Value New Value
        Attachment patch.txt [ 12456489 ]
        Hide
        Martin Monperrus added a comment -

        Hi Kristian,

        Regarding the API and DERBY-2490, I noticed in DataDictionaryImpl that:

        • some methods require a TransactionController as parameter
        • others use the internal methods getTransactionExecute(), getTransactionCompile() which both use getLCC() internally

        In which case could it be harmful/less efficient to use the default transaction controller provided by getLCC() instead of using the one passed as parameter (if present)?

        Best regards,

        --Martin

        Show
        Martin Monperrus added a comment - Hi Kristian, Regarding the API and DERBY-2490 , I noticed in DataDictionaryImpl that: some methods require a TransactionController as parameter others use the internal methods getTransactionExecute(), getTransactionCompile() which both use getLCC() internally In which case could it be harmful/less efficient to use the default transaction controller provided by getLCC() instead of using the one passed as parameter (if present)? Best regards, --Martin
        Hide
        Kristian Waagan added a comment -

        I don't think this is about efficiency, but about correctness for a few special cases - where a nested transaction has been created.
        Derby might get into trouble if some of the work is performed using a nested transaction and some by the parent/main transaction. The nested transaction was introduced to be able to commit some work immediately without committing the work done by the parent transaction, or to release locks as early as possible where appropriate. I.e. typically do work with parent tx, do some (short duration) special work with nested tx, commit nested tx, continue with work using the parent tx.

        In terms of code, the difference boils down to the following:
        return tran;
        vs
        return (readOnlyNestedTransaction != null) ? readOnlyNestedTransaction : tran;

        In a scenario as mentioned above, it is important that the correct transaction is being used at all times.
        From what I know about this issue, the patch looks correct - I'm +1 to commit it.

        I will wait for a few more days to see if anyone with more knowledge about this pitches in.

        Show
        Kristian Waagan added a comment - I don't think this is about efficiency, but about correctness for a few special cases - where a nested transaction has been created. Derby might get into trouble if some of the work is performed using a nested transaction and some by the parent/main transaction. The nested transaction was introduced to be able to commit some work immediately without committing the work done by the parent transaction, or to release locks as early as possible where appropriate. I.e. typically do work with parent tx, do some (short duration) special work with nested tx, commit nested tx, continue with work using the parent tx. In terms of code, the difference boils down to the following: return tran; vs return (readOnlyNestedTransaction != null) ? readOnlyNestedTransaction : tran; In a scenario as mentioned above, it is important that the correct transaction is being used at all times. From what I know about this issue, the patch looks correct - I'm +1 to commit it. I will wait for a few more days to see if anyone with more knowledge about this pitches in.
        Hide
        Kristian Waagan added a comment -

        I committed the patch to trunk with revision 1023824.

        Thanks for providing the patch, Martin

        Show
        Kristian Waagan added a comment - I committed the patch to trunk with revision 1023824. Thanks for providing the patch, Martin
        Kristian Waagan made changes -
        Assignee Kristian Waagan [ kristwaa ]
        Fix Version/s 10.7.0.0 [ 12314971 ]
        Affects Version/s 10.7.0.0 [ 12314971 ]
        Issue & fix info [Patch Available, Newcomer] [Newcomer]
        Component/s SQL [ 11408 ]
        Component/s Network Server [ 11410 ]
        Kristian Waagan made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Rick Hillegas added a comment -

        Removing the [patch] tag from the issue description. When those square brackets turn up in the release notes they will confuse the Forrest tool which creates a download page from our release notes.

        Show
        Rick Hillegas added a comment - Removing the [patch] tag from the issue description. When those square brackets turn up in the release notes they will confuse the Forrest tool which creates a download page from our release notes.
        Rick Hillegas made changes -
        Summary [patch] DropSchemaConstantAction: could reuse the current connection provided by the available activation object DropSchemaConstantAction: could reuse the current connection provided by the available activation object
        Rick Hillegas made changes -
        Affects Version/s 10.7.1.1 [ 12315564 ]
        Affects Version/s 10.7.1.0 [ 12314971 ]
        Fix Version/s 10.7.1.1 [ 12315564 ]
        Fix Version/s 10.7.1.0 [ 12314971 ]
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow jira [ 12521707 ] Default workflow, editable Closed status [ 12802744 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        20d 1h 26m 1 Kristian Waagan 18/Oct/10 15:52
        Resolved Resolved Closed Closed
        972d 18h 26m 1 Knut Anders Hatlen 17/Jun/13 10:19

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development