Derby
  1. Derby
  2. DERBY-4772

Data truncation error with XPLAIN-functionality enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.6.2.1, 10.7.1.1
    • Fix Version/s: 10.6.2.4, 10.7.1.1
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Seen in production

      Description

      When running a modified version of lang.OrderByAndSortAvoidance I get the following error:

      java.sql.SQLDataException: A truncation error was encountered trying to shrink CHAR 'Thread[DRDAConnThread_3,5,derby.daemons]' to length 32.
      at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:79)
      at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:256)
      at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:391)
      at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346)
      at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2269)
      at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81)
      at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1321)
      at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673)
      at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:303)
      at org.apache.derby.impl.sql.execute.xplain.XPLAINSystemTableVisitor.addStmtDescriptorsToSystemCatalog(XPLAINSystemTableVisitor.java:390)
      at org.apache.derby.impl.sql.execute.xplain.XPLAINSystemTableVisitor.doXPLAIN(XPLAINSystemTableVisitor.java:317)
      at org.apache.derby.impl.sql.execute.NoPutResultSetImpl.close(NoPutResultSetImpl.java:179)
      at org.apache.derby.impl.sql.execute.SortResultSet.close(SortResultSet.java:467)
      at org.apache.derby.impl.jdbc.EmbedResultSet.close(EmbedResultSet.java:575)
      at org.apache.derby.impl.drda.DRDAResultSet.close(DRDAResultSet.java:338)
      at org.apache.derby.impl.drda.DRDAStatement.rsClose(DRDAStatement.java:995)
      at org.apache.derby.impl.drda.DRDAConnThread.doneData(DRDAConnThread.java:7446)
      at org.apache.derby.impl.drda.DRDAConnThread.writeFDODTA(DRDAConnThread.java:7026)
      at org.apache.derby.impl.drda.DRDAConnThread.writeQRYDTA(DRDAConnThread.java:6910)
      at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:870)
      at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:294)
      Caused by: java.sql.SQLException: A truncation error was encountered trying to shrink CHAR 'Thread[DRDAConnThread_3,5,derby.daemons]' to length 32.
      at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
      at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119)
      at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
      ... 20 more
      Caused by: ERROR 22001: A truncation error was encountered trying to shrink CHAR 'Thread[DRDAConnThread_3,5,derby.daemons]' to length 32.
      at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:343)
      at org.apache.derby.iapi.types.SQLChar.hasNonBlankChars(SQLChar.java:1767)
      at org.apache.derby.iapi.types.SQLChar.normalize(SQLChar.java:1743)
      at org.apache.derby.iapi.types.SQLChar.normalize(SQLChar.java:1695)
      at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648)
      at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329)
      at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373)
      at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188)
      at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127)
      at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:504)
      at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)
      at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)
      at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232)
      ... 14 more

      I suspect the error can be triggered easily in client/server, but for convenience I'll attach the patch for the test where I see the issue.

      1. releaseNote.html
        4 kB
        Kristian Waagan
      2. releaseNote.html
        4 kB
        Kristian Waagan
      3. derby-4772-1b-increase_max_len.diff
        8 kB
        Kristian Waagan
      4. derby-4772-1a-increase_max_len.diff
        2 kB
        Kristian Waagan

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Attaching patch which causes the error when running the test lang.OrderByAndSortAvoidance.
          Note that the patch uses the new PlanExporter code, and since this code is in movement you may have to apply the patch to the exact revision used by the diff.

          FYI, I ran the test like this:
          java -Dderby.language.disableIndexStatsUpdate=true -Djava.security.policy="<NONE>" -cp ../classes:../tools/java/junit.jar junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.OrderByAndSortAvoidance

          Show
          Kristian Waagan added a comment - Attaching patch which causes the error when running the test lang.OrderByAndSortAvoidance. Note that the patch uses the new PlanExporter code, and since this code is in movement you may have to apply the patch to the exact revision used by the diff. FYI, I ran the test like this: java -Dderby.language.disableIndexStatsUpdate=true -Djava.security.policy="<NONE>" -cp ../classes:../tools/java/junit.jar junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.OrderByAndSortAvoidance
          Hide
          Bryan Pendleton added a comment -

          I suspect that the column is the XPLAIN_THREAD_ID column in SYSXPLAIN_STATEMENTS,
          and something like the following would help:

          Index: java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java
          ===================================================================
          — java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java(revision 985069)
          +++ java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java(working copy)
          @@ -126,7 +126,7 @@
          SystemColumnImpl.getColumn("OS_IDENTIFIER", Types.CHAR, false, 30),
          SystemColumnImpl.getColumn("XPLAIN_MODE", Types.CHAR, true, 1),
          SystemColumnImpl.getColumn("XPLAIN_TIME", Types.TIMESTAMP, true),

          • SystemColumnImpl.getColumn("XPLAIN_THREAD_ID", Types.CHAR, false, 32),
            + SystemColumnImpl.getColumn("XPLAIN_THREAD_ID", Types.CHAR, false, 128),
            SystemColumnImpl.getColumn("TRANSACTION_ID", Types.CHAR, false, 32),
            SystemColumnImpl.getColumn("SESSION_ID", Types.CHAR, false, 32),
            SystemColumnImpl.getIdentifierColumn("DATABASE_NAME", false),
          Show
          Bryan Pendleton added a comment - I suspect that the column is the XPLAIN_THREAD_ID column in SYSXPLAIN_STATEMENTS, and something like the following would help: Index: java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java =================================================================== — java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java(revision 985069) +++ java/engine/org/apache/derby/impl/sql/catalog/XPLAINStatementDescriptor.java(working copy) @@ -126,7 +126,7 @@ SystemColumnImpl.getColumn("OS_IDENTIFIER", Types.CHAR, false, 30), SystemColumnImpl.getColumn("XPLAIN_MODE", Types.CHAR, true, 1), SystemColumnImpl.getColumn("XPLAIN_TIME", Types.TIMESTAMP, true), SystemColumnImpl.getColumn("XPLAIN_THREAD_ID", Types.CHAR, false, 32), + SystemColumnImpl.getColumn("XPLAIN_THREAD_ID", Types.CHAR, false, 128), SystemColumnImpl.getColumn("TRANSACTION_ID", Types.CHAR, false, 32), SystemColumnImpl.getColumn("SESSION_ID", Types.CHAR, false, 32), SystemColumnImpl.getIdentifierColumn("DATABASE_NAME", false),
          Hide
          Kristian Waagan added a comment -

          Yes, but some quick debugging showed that it's not only XPLAIN_THREAD_ID that has this issue.
          Besides from the approach you mentioned, I guess we can also either, or in addition, insert only parts of the value.
          Sounds like 128 characters should be enough for most cases though, but if users can set both thread name and thread group name, you never can tell.

          Show
          Kristian Waagan added a comment - Yes, but some quick debugging showed that it's not only XPLAIN_THREAD_ID that has this issue. Besides from the approach you mentioned, I guess we can also either, or in addition, insert only parts of the value. Sounds like 128 characters should be enough for most cases though, but if users can set both thread name and thread group name, you never can tell.
          Hide
          Brett Wooldridge added a comment -

          The obvious 'fix', and I think minimum that should be done, is to increase the THREAD_ID column and TRANSACTION_ID column sizes to the already suggested 128 characters. Though is there some reason we are using CHAR rather than VARCHAR here? It seems like a waste of space. Maybe not so much an issue when the size was 32, but I think if we increase the column size we should consider converting it to VARCHAR.

          A step beyond the above fixes would be to truncate the columns to ensure that they fit. Though not a likely a problem in the THREAD_ID case, there is the possibility that if some transaction manager uses extremely long XID, that truncating could result in two transactions that are actually distinct being mistaken for the same transaction. I think this is unlikely, and should be addressed as a bug should it ever occur.

          Show
          Brett Wooldridge added a comment - The obvious 'fix', and I think minimum that should be done, is to increase the THREAD_ID column and TRANSACTION_ID column sizes to the already suggested 128 characters. Though is there some reason we are using CHAR rather than VARCHAR here? It seems like a waste of space. Maybe not so much an issue when the size was 32, but I think if we increase the column size we should consider converting it to VARCHAR. A step beyond the above fixes would be to truncate the columns to ensure that they fit. Though not a likely a problem in the THREAD_ID case, there is the possibility that if some transaction manager uses extremely long XID, that truncating could result in two transactions that are actually distinct being mistaken for the same transaction. I think this is unlikely, and should be addressed as a bug should it ever occur.
          Hide
          Knut Anders Hatlen added a comment -

          A similar problem was reported in DERBY-4673. There, the problem was with the DRDA_ID column.

          Show
          Knut Anders Hatlen added a comment - A similar problem was reported in DERBY-4673 . There, the problem was with the DRDA_ID column.
          Hide
          Kristian Waagan added a comment -

          Do we need to add upgrade logic if we change the column type from CHAR(32) to, say, VARCHAR(128)?

          Show
          Kristian Waagan added a comment - Do we need to add upgrade logic if we change the column type from CHAR(32) to, say, VARCHAR(128)?
          Hide
          Knut Anders Hatlen added a comment -

          I don't think any upgrade logic is needed, since the XPLAIN tables are not system tables and won't be present unless they've been explicitly created. I vaguely recall some discussions about this when XPLAIN was implemented, and I think the conclusion was that schema changes in the the XPLAIN tables between releases would be acceptable, and if they caused incompatibilities, users should be able to resolve that by dropping the old tables so that new ones with the new schema would be created after the upgrade.

          Show
          Knut Anders Hatlen added a comment - I don't think any upgrade logic is needed, since the XPLAIN tables are not system tables and won't be present unless they've been explicitly created. I vaguely recall some discussions about this when XPLAIN was implemented, and I think the conclusion was that schema changes in the the XPLAIN tables between releases would be acceptable, and if they caused incompatibilities, users should be able to resolve that by dropping the old tables so that new ones with the new schema would be created after the upgrade.
          Hide
          Kristian Waagan added a comment -

          Thanks, Knut.
          That sounds reasonable, and another point supporting that approach is my assumption that not a lot of people have started using the feature yet.

          I need XPLAIN to work to be able to write a test for a different issue, so I'll go ahead and write a patch.
          My first step was to clean up some code in XPLAINSystemTableVisitor (unused imports, unused variables, removed printStackTrace). Committed to trunk with revision 1027921.

          Show
          Kristian Waagan added a comment - Thanks, Knut. That sounds reasonable, and another point supporting that approach is my assumption that not a lot of people have started using the feature yet. I need XPLAIN to work to be able to write a test for a different issue, so I'll go ahead and write a patch. My first step was to clean up some code in XPLAINSystemTableVisitor (unused imports, unused variables, removed printStackTrace). Committed to trunk with revision 1027921.
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1a, which increases the maximum length of (most of) the string values from 32 to 128 characters, and also switches the column type from CHAR to VARCHAR.
          This should take care of most cases, but as mentioned elsewhere, there is still a possibility that a user will be able to feed longer values into the system. This will cause the XPLAIN feature to crash.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Attaching patch 1a, which increases the maximum length of (most of) the string values from 32 to 128 characters, and also switches the column type from CHAR to VARCHAR. This should take care of most cases, but as mentioned elsewhere, there is still a possibility that a user will be able to feed longer values into the system. This will cause the XPLAIN feature to crash. Patch ready for review.
          Hide
          Knut Anders Hatlen added a comment - - edited

          Hi Kristian,

          The patch looks like a good improvement (both increasing the length and changing the type to VARCHAR), but is there any particular reason why we should limit the strings to 128 and not to the maximum length of a VARCHAR (32672)?

          [comment edited: added missing 'not']

          Show
          Knut Anders Hatlen added a comment - - edited Hi Kristian, The patch looks like a good improvement (both increasing the length and changing the type to VARCHAR), but is there any particular reason why we should limit the strings to 128 and not to the maximum length of a VARCHAR (32672)? [comment edited: added missing 'not']
          Hide
          Kristian Waagan added a comment -

          No reason except that it looks like overkill to allow 32672 characters for something that is currently one character long
          The only other drawback I can think of is when formatting data for display, but that's another story...

          Looking at XPLAINScanPropsDescriptor, I think it makes sense to use the maximum allowed length there (Stephan van Loendersloot experienced that 512 characters weren't enough).
          Shall we just use the maximum length for the string fields in XPLAINStatementDescriptor too, then?

          Show
          Kristian Waagan added a comment - No reason except that it looks like overkill to allow 32672 characters for something that is currently one character long The only other drawback I can think of is when formatting data for display, but that's another story... Looking at XPLAINScanPropsDescriptor, I think it makes sense to use the maximum allowed length there (Stephan van Loendersloot experienced that 512 characters weren't enough). Shall we just use the maximum length for the string fields in XPLAINStatementDescriptor too, then?
          Hide
          Knut Anders Hatlen added a comment -

          Apart from the potentially odd formatting in ij with the maximum length (would that be much different with 128 characters, though?), I don't see any negative sides with allowing the maximum length. But, of course, if a field is known never to be longer than one character (or some specific number of characters), there wouldn't be any point in increasing the limit.

          Show
          Knut Anders Hatlen added a comment - Apart from the potentially odd formatting in ij with the maximum length (would that be much different with 128 characters, though?), I don't see any negative sides with allowing the maximum length. But, of course, if a field is known never to be longer than one character (or some specific number of characters), there wouldn't be any point in increasing the limit.
          Hide
          Bryan Pendleton added a comment -

          The behavior of ij with long column widths is definitely annoying, but I don't know any easy way to fix that.

          It seems like, with other values (guids, identifiers, etc.) there are well-known system-wide max lengths,
          and it seems like some of these other internal identifiers need to have a known max length
          established, at which point the xplain tables can use those limits.

          Show
          Bryan Pendleton added a comment - The behavior of ij with long column widths is definitely annoying, but I don't know any easy way to fix that. It seems like, with other values (guids, identifiers, etc.) there are well-known system-wide max lengths, and it seems like some of these other internal identifiers need to have a known max length established, at which point the xplain tables can use those limits.
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1b.
          It replaces CHAR with VARCHAR where there is no hard limit defined on the maximum length of the value, and it uses the maximum allowed length for the VARCHAR type.

          It would be nice if someone with more knowledge in this area of the code could have a quick look and say if any of the changes should be discarded, because the value in question actually has a defined/known maximum length.
          This patch should also address the second error mentioned in DERBY-4673.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Attaching patch 1b. It replaces CHAR with VARCHAR where there is no hard limit defined on the maximum length of the value, and it uses the maximum allowed length for the VARCHAR type. It would be nice if someone with more knowledge in this area of the code could have a quick look and say if any of the changes should be discarded, because the value in question actually has a defined/known maximum length. This patch should also address the second error mentioned in DERBY-4673 . Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Committed patch 1b to trunk with revision 1033864.

          I ticked the Release Note Needed flag, since I suppose a release note should be added to inform users they have to delete existing XPLAIN tables when upgrading. I will write a first revision and attach it shortly.

          Do people feel this fix is appropriate for a backport to 10.6?

          Show
          Kristian Waagan added a comment - Committed patch 1b to trunk with revision 1033864. I ticked the Release Note Needed flag, since I suppose a release note should be added to inform users they have to delete existing XPLAIN tables when upgrading. I will write a first revision and attach it shortly. Do people feel this fix is appropriate for a backport to 10.6?
          Hide
          Kristian Waagan added a comment -

          Attached first revision of 'releaseNote.html'.

          Please review.

          It seems the change doesn't really cause any incompatibilities, but the XPLAIN tables should be recreated to avoid the data truncation errors.

          Show
          Kristian Waagan added a comment - Attached first revision of 'releaseNote.html'. Please review. It seems the change doesn't really cause any incompatibilities, but the XPLAIN tables should be recreated to avoid the data truncation errors.
          Hide
          Bryan Pendleton added a comment -

          Thanks Kristian, for fixing this problem.

          The release note looks fine to me.

          I'm not sure if another release of 10.6 is going to happen, but
          I think this would be a nice fix to backport if you have the time.
          More than one user has encountered the problem.

          Show
          Bryan Pendleton added a comment - Thanks Kristian, for fixing this problem. The release note looks fine to me. I'm not sure if another release of 10.6 is going to happen, but I think this would be a nice fix to backport if you have the time. More than one user has encountered the problem.
          Hide
          Dag H. Wanvik added a comment -

          I think a release note is warranted, since there is a behavior
          change, if not an upgrade issue.

          Release notes looks good. Typo: accomodate-> accommodate

          Show
          Dag H. Wanvik added a comment - I think a release note is warranted, since there is a behavior change, if not an upgrade issue. Release notes looks good. Typo: accomodate-> accommodate
          Hide
          Dag H. Wanvik added a comment -

          Back port? I think that's fine.

          Show
          Dag H. Wanvik added a comment - Back port? I think that's fine.
          Hide
          Kristian Waagan added a comment -

          Thanks guys!

          Attaching a new version of the release note (fixed two typos).

          Show
          Kristian Waagan added a comment - Thanks guys! Attaching a new version of the release note (fixed two typos).
          Hide
          Kristian Waagan added a comment -

          Backported fix to 10.6 with revision 1034384.

          Show
          Kristian Waagan added a comment - Backported fix to 10.6 with revision 1034384.
          Hide
          Kristian Waagan added a comment -

          Closing issue.

          Show
          Kristian Waagan added a comment - Closing issue.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development