Derby
  1. Derby
  2. DERBY-5749

Implicit cast of variable length values, e.g. as arguments to stored methods and generated columns values, silently truncate if too long

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • 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, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0, 10.5.1.1, 10.5.2.0, 10.5.3.0, 10.6.1.0, 10.6.2.1, 10.7.1.1, 10.8.1.2, 10.8.2.2
    • Fix Version/s: 10.9.1.0
    • Component/s: JDBC, SQL
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Data corruption, Deviation from standard

      Description

      Cf the attached repro SilentVarcharArgTruncation.
      It seem implicit casts are inserted here, cf discussion on derby-dev thread starting here: http://mail-archives.apache.org/mod_mbox/db-derby-dev/201205.mbox/%3Cx67gwm7hir.fsf%40oracle.com%3E

      1. SilentVarcharArgTruncation.java
        1 kB
        Dag H. Wanvik
      2. SilentVarcharArgTruncation.java
        2 kB
        Dag H. Wanvik
      3. DERBY-5749.diff
        6 kB
        Dag H. Wanvik
      4. DERBY-5749.stat
        0.2 kB
        Dag H. Wanvik
      5. DERBY-5749b.diff
        14 kB
        Dag H. Wanvik
      6. DERBY-5749b.stat
        0.9 kB
        Dag H. Wanvik
      7. DERBY-5749-2.diff
        5 kB
        Dag H. Wanvik
      8. DERBY-5749-2.stat
        0.3 kB
        Dag H. Wanvik
      9. DERBY-5749-2b.diff
        5 kB
        Dag H. Wanvik
      10. DERBY-5749-2b.stat
        0.3 kB
        Dag H. Wanvik
      11. releaseNote.html
        5 kB
        Dag H. Wanvik
      12. releaseNote.html
        8 kB
        Knut Anders Hatlen
      13. releaseNote.html
        8 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          This is my reading of the SQL Standard in this area:

          According to part 2, section 10.4 (<routine invocation>), Syntax Rule 8.c, argument passing is supposed to satisfy the same rules as storing data in a column.

          Those rule are declared in part 2, section 9.2 (Store assignment). There, General Rule 2.b.v.2 says that the database should raise an exception if truncation occurs when stuffing a string value into a varchar.

          Derby may be deliberately inserting implicit CASTs in order to fix some other bug. The CASTs downgrade the exception to a warning. The truncation behavior of CASTs is described in part 2, section 6.12 (<cast specification>), General Rules 10.c.ii and 11.c.ii.

          Show
          Rick Hillegas added a comment - This is my reading of the SQL Standard in this area: According to part 2, section 10.4 (<routine invocation>), Syntax Rule 8.c, argument passing is supposed to satisfy the same rules as storing data in a column. Those rule are declared in part 2, section 9.2 (Store assignment). There, General Rule 2.b.v.2 says that the database should raise an exception if truncation occurs when stuffing a string value into a varchar. Derby may be deliberately inserting implicit CASTs in order to fix some other bug. The CASTs downgrade the exception to a warning. The truncation behavior of CASTs is described in part 2, section 6.12 (<cast specification>), General Rules 10.c.ii and 11.c.ii.
          Hide
          Dag H. Wanvik added a comment - - edited

          Thanks, Rick, that reading seems right to me. Without the patch for DERBY-5537, there is no warning issued, though, even in this case.

          Show
          Dag H. Wanvik added a comment - - edited Thanks, Rick, that reading seems right to me. Without the patch for DERBY-5537 , there is no warning issued, though, even in this case.
          Hide
          Dag H. Wanvik added a comment -

          The code inserts a cast on the actual argument string here if dynamic parameter: StaticMethodCallNode#585:

          boolean needCast = false;
          if (!isParameterMarker)
          {
          :
          }
          else
          {
          // any variable length type will need a cast from the
          // Java world (the ? parameter) to the SQL type. This
          // ensures values like CHAR(10) are passed into the procedure
          // correctly as 10 characters long.
          if (parameterTypeId.variableLength())

          { if (parameterMode != JDBC30Translation.PARAMETER_MODE_OUT) ---> needCast = true; }

          }

          Show
          Dag H. Wanvik added a comment - The code inserts a cast on the actual argument string here if dynamic parameter: StaticMethodCallNode#585: boolean needCast = false; if (!isParameterMarker) { : } else { // any variable length type will need a cast from the // Java world (the ? parameter) to the SQL type. This // ensures values like CHAR(10) are passed into the procedure // correctly as 10 characters long. if (parameterTypeId.variableLength()) { if (parameterMode != JDBC30Translation.PARAMETER_MODE_OUT) ---> needCast = true; } }
          Hide
          Dag H. Wanvik added a comment -

          The issue also exists for non-dynamic arguments, like "call p('123456')" StaticMethodCallNode#585:

          :
          // if it's not an exact length match then some cast will be needed.
          if (!paramdtd.isExactTypeAndLengthMatch(dts))
          needCast = true;

          Show
          Dag H. Wanvik added a comment - The issue also exists for non-dynamic arguments, like "call p('123456')" StaticMethodCallNode#585: : // if it's not an exact length match then some cast will be needed. if (!paramdtd.isExactTypeAndLengthMatch(dts)) needCast = true;
          Hide
          Dag H. Wanvik added a comment -

          Uploading an extended repro to show the non-dynamic case as well.

          Show
          Dag H. Wanvik added a comment - Uploading an extended repro to show the non-dynamic case as well.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a patch which makes the casts used here use the assignment semantics, which leads to SQLChar#setWidth method is called with argument errorOnTrunc == true.
          This will make the cast throw if one or more non-blanks are truncated.

          Added a new test RoutineTest#test_5749 which contains the test cases from the repro.

          Running regressions.

          Show
          Dag H. Wanvik added a comment - Attaching a patch which makes the casts used here use the assignment semantics, which leads to SQLChar#setWidth method is called with argument errorOnTrunc == true. This will make the cast throw if one or more non-blanks are truncated. Added a new test RoutineTest#test_5749 which contains the test cases from the repro. Running regressions.
          Hide
          Dag H. Wanvik added a comment -

          Btw, the change will make truncation throw for other datatypes as well, cf. the setWidth methods of SQLBit, SQLBlob, SQLChar and SQLVarbit.

          Show
          Dag H. Wanvik added a comment - Btw, the change will make truncation throw for other datatypes as well, cf. the setWidth methods of SQLBit, SQLBlob, SQLChar and SQLVarbit.
          Hide
          Dag H. Wanvik added a comment -

          This changes behavior, so it could be argued we need a release note if this goes in. On the other hand, we believe the old behavior was erroneous, so by our usual rule, we do not need a release note.
          What do you think? I guess I always prefer to err on the side of including too much...

          Show
          Dag H. Wanvik added a comment - This changes behavior, so it could be argued we need a release note if this goes in. On the other hand, we believe the old behavior was erroneous, so by our usual rule, we do not need a release note. What do you think? I guess I always prefer to err on the side of including too much...
          Hide
          Knut Anders Hatlen added a comment -

          The patch looks fine to me. I had a look at the wording in the standard, and I agree with your interpretation. Since we'll now throw an exception in cases where we used to be silent, I think we should add a release note for this issue.

          Show
          Knut Anders Hatlen added a comment - The patch looks fine to me. I had a look at the wording in the standard, and I agree with your interpretation. Since we'll now throw an exception in cases where we used to be silent, I think we should add a release note for this issue.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut. Checking other implicit cast usage, I found this, which is probably also wrong since it's definitely an assignment:

          > s.executeUpdate("create table tt(c varchar(5) generated always as ('--' || b), b varchar(5))");
          > s.executeUpdate("insert into tt values (default, '12345')");

          No error is thrown here either.

          > select * from tt;
          C |A
          -----------
          --123|12345

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. Checking other implicit cast usage, I found this, which is probably also wrong since it's definitely an assignment: > s.executeUpdate("create table tt(c varchar(5) generated always as ('--' || b), b varchar(5))"); > s.executeUpdate("insert into tt values (default, '12345')"); No error is thrown here either. > select * from tt; C |A ----------- --123|12345
          Hide
          Dag H. Wanvik added a comment -

          We may not want to put a fix for this issue in for 10.9, since it introduces change in type checking/conversion apparatus late in the game.

          I do believe fixing this is warranted, since it represents a bug that corrupts (loses) data, but we may want to commit only the fix for DERBY-5537 for now, and write up a release note that people can vet their application by checking for warnings after calling stored methods or inserting/updating rows in tables with generated columns, and that this will be changed to throwing a truncation error in the next release. (The fix for DERBY-5537 will generate a SQLWarning in lieu of the SQLException truncation error.)

          What do you think?

          Show
          Dag H. Wanvik added a comment - We may not want to put a fix for this issue in for 10.9, since it introduces change in type checking/conversion apparatus late in the game. I do believe fixing this is warranted, since it represents a bug that corrupts (loses) data, but we may want to commit only the fix for DERBY-5537 for now, and write up a release note that people can vet their application by checking for warnings after calling stored methods or inserting/updating rows in tables with generated columns, and that this will be changed to throwing a truncation error in the next release. (The fix for DERBY-5537 will generate a SQLWarning in lieu of the SQLException truncation error.) What do you think?
          Hide
          Dag H. Wanvik added a comment -

          Regressions showed some test errors due to the stricter checking. Uploading a new version of the patch which also fixes the tests, DERBY-5749b. Regressions ran ok with this patch.

          Show
          Dag H. Wanvik added a comment - Regressions showed some test errors due to the stricter checking. Uploading a new version of the patch which also fixes the tests, DERBY-5749 b. Regressions ran ok with this patch.
          Hide
          Knut Anders Hatlen added a comment -

          The impact of the current fix is quite limited, as it only affects arguments passed to stored procedures, so I think it would be fine to include the fix as long as it's accompanied by a release note. Those who want the old behaviour with truncation could get that by using explicit casts, I suppose.

          Show
          Knut Anders Hatlen added a comment - The impact of the current fix is quite limited, as it only affects arguments passed to stored procedures, so I think it would be fine to include the fix as long as it's accompanied by a release note. Those who want the old behaviour with truncation could get that by using explicit casts, I suppose.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut.

          Uploading an additive patch DERBY-5749-2 (it requires DERBY-5749b), which adds truncation check to generated columns as well, and adds a test case (plus inserts some explicit casts to make the old generated column tests work; there were cases where silent truncation was assumed).

          Running regressions on the two patches together.

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. Uploading an additive patch DERBY-5749 -2 (it requires DERBY-5749 b), which adds truncation check to generated columns as well, and adds a test case (plus inserts some explicit casts to make the old generated column tests work; there were cases where silent truncation was assumed). Running regressions on the two patches together.
          Hide
          Dag H. Wanvik added a comment -

          Regressions went OK.

          Show
          Dag H. Wanvik added a comment - Regressions went OK.
          Hide
          Knut Anders Hatlen added a comment -

          The patches look good to me.

          Nits:

          • There's a typo in the comment in DMLModStatementNode: "implicit case" -> "implicit case".
          • You might want to test an update statement too in test_derby_5749(). One that increases the length of a value in column B so that the auto-generated value in column C gets too long.
          Show
          Knut Anders Hatlen added a comment - The patches look good to me. Nits: There's a typo in the comment in DMLModStatementNode: "implicit case" -> "implicit case". You might want to test an update statement too in test_derby_5749(). One that increases the length of a value in column B so that the auto-generated value in column C gets too long.
          Hide
          Knut Anders Hatlen added a comment -

          The patches look good to me.

          Nits:

          • There's a typo in the comment in DMLModStatementNode: "implicit case" -> "implicit case".
          • You might want to test an update statement too in test_derby_5749(). One that increases the length of a value in column B so that the auto-generated value in column C gets too long.
          Show
          Knut Anders Hatlen added a comment - The patches look good to me. Nits: There's a typo in the comment in DMLModStatementNode: "implicit case" -> "implicit case". You might want to test an update statement too in test_derby_5749(). One that increases the length of a value in column B so that the auto-generated value in column C gets too long.
          Hide
          Kristian Waagan added a comment -

          (the typo is in patch DERBY-5749-2.diff, "implict case" - had to check since I couldn't see the typo in the comment from Knut Anders )

          Show
          Kristian Waagan added a comment - (the typo is in patch DERBY-5749 -2.diff, "implict case" - had to check since I couldn't see the typo in the comment from Knut Anders )
          Hide
          Knut Anders Hatlen added a comment -

          Right, it should have said "implicit case" -> "implicit cast". Thanks, Kristian.

          Show
          Knut Anders Hatlen added a comment - Right, it should have said "implicit case" -> "implicit cast". Thanks, Kristian.
          Hide
          Dag H. Wanvik added a comment -

          Uploading DERBY-57492b, which fixes the two typos and adds an additional test case for update as suggested. WIll commit both patches soon (DERBY-5749b & DERBY-5749-2b).

          Show
          Dag H. Wanvik added a comment - Uploading DERBY-57492b, which fixes the two typos and adds an additional test case for update as suggested. WIll commit both patches soon ( DERBY-5749 b & DERBY-5749 -2b).
          Hide
          Dag H. Wanvik added a comment -

          Uploaded a releaseNote.html for this issue, please have a look.

          Show
          Dag H. Wanvik added a comment - Uploaded a releaseNote.html for this issue, please have a look.
          Hide
          Dag H. Wanvik added a comment -

          Committed the two patches as svn 1339281, resolving, as I didn't find other cases.

          Show
          Dag H. Wanvik added a comment - Committed the two patches as svn 1339281, resolving, as I didn't find other cases.
          Hide
          Dag H. Wanvik added a comment -

          Since the patches committed changes Derby behavior, I don't think they are suitable for back-ports, so closing.

          Show
          Dag H. Wanvik added a comment - Since the patches committed changes Derby behavior, I don't think they are suitable for back-ports, so closing.
          Hide
          Knut Anders Hatlen added a comment -

          Changing from closed to resolved to allow updating the release note.

          Show
          Knut Anders Hatlen added a comment - Changing from closed to resolved to allow updating the release note.
          Hide
          Knut Anders Hatlen added a comment -

          Uploading an updated release note with these changes:

          • adds information about the truncation warning added in DERBY-129
          • moves information from summary (which is supposed to be a one-liner) to the symptoms section
          • moves <ul> out of <p> to pass HTML validation checks
          • adds information on how to downgrade the new exceptions to warnings using casts in the examples shown in the Application Changes Required section

          The release note passed the checks in ReleaseNoteReader and HTML validation on http://validator.w3.org/.

          Show
          Knut Anders Hatlen added a comment - Uploading an updated release note with these changes: adds information about the truncation warning added in DERBY-129 moves information from summary (which is supposed to be a one-liner) to the symptoms section moves <ul> out of <p> to pass HTML validation checks adds information on how to downgrade the new exceptions to warnings using casts in the examples shown in the Application Changes Required section The release note passed the checks in ReleaseNoteReader and HTML validation on http://validator.w3.org/ .
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the consolidated release note, Knut. I uploaded another version to fix bugs in the error code: 22011 -> 22001 in some locations.

          Show
          Dag H. Wanvik added a comment - Thanks for the consolidated release note, Knut. I uploaded another version to fix bugs in the error code: 22011 -> 22001 in some locations.
          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
          Mike Matrigali added a comment -

          fix is a minor behavior change that may require user app changes, which are documented in release note. best not backported to existing release.

          Show
          Mike Matrigali added a comment - fix is a minor behavior change that may require user app changes, which are documented in release note. best not backported to existing release.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development