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

        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.
        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.
        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
        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
        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
        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
        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
        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
        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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development