Derby
  1. Derby
  2. DERBY-5459

Result set metadata are out of sync on client after underlying table is altered

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 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
    • Fix Version/s: 10.8.3.3, 10.9.1.0
    • Component/s: JDBC
    • Labels:
      None
    • Issue & fix info:
      High Value Fix, Repro attached
    • Bug behavior facts:
      Embedded/Client difference, Wrong query result

      Description

      Cf the discussion on DERBY-3823.

      The enclosed repro program shows what happens. When I run it with
      client/server and embedded respectively we see these two differing
      results:

      client/server:

      $ java -cp .:$CLASSPATH Repo
      Done loading data
      executing alter
      execp.getResultDescription: select * from t1
      2. PS#getMetaData: char column length is 8
      Reexecuting ps on changed table...
      3. RS#getMetadata: char column length is 8
      data:1 12345678

      dag@T61pOS:~/java/sb/apps/derby3823$ !rm
      rm -rf DERBY3823DB

      embedded:

      dag@T61pOS:~/java/sb/apps/derby3823$ java -cp .:$CLASSPATH Repro 2
      execp.getResultDescription: insert into t1 values(?,'aaaaa')
      execp.getResultDescription: insert into t1 values(?,'aaaaa')
      Done loading data
      execp.getResultDescription: select * from t1
      execp.getResultDescription: select * from t1
      executing alter
      2. PS#getMetaData: char column length is 5
      Reexecuting ps on changed tableh...
      3. RS#getMetadata: char column length is 5
      data:1 12345678

      As we can see, the metadata results are different after the ALTER
      TABLE. The trace from EmbedPreparedData
      ("execp.getResultDescription:") lines (see repro-patch.diff) show that
      after ALTER, the metadata are not refreshed on the server side.

      1. repro-patch.diff
        0.6 kB
        Dag H. Wanvik
      2. Repro.java
        3 kB
        Dag H. Wanvik
      3. derby-5459-3.stat
        0.8 kB
        Dag H. Wanvik
      4. derby-5459-3.diff
        12 kB
        Dag H. Wanvik
      5. derby-5459-2.stat
        0.7 kB
        Dag H. Wanvik
      6. derby-5459-2.diff
        11 kB
        Dag H. Wanvik
      7. derby-5459-1.stat
        0.7 kB
        Dag H. Wanvik
      8. derby-5459-1.diff
        9 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Note: both the result of the PreparedStatement#getMetaData and the result set's ResultSet#getMetaData are wrong.

          Show
          Dag H. Wanvik added a comment - Note: both the result of the PreparedStatement#getMetaData and the result set's ResultSet#getMetaData are wrong.
          Hide
          Dag H. Wanvik added a comment -

          Linking to two other issues with differences between embedded and client/server which may or may not be related (different result set handling is a common denominator). See also discussion on DERBY-3823.

          Show
          Dag H. Wanvik added a comment - Linking to two other issues with differences between embedded and client/server which may or may not be related (different result set handling is a common denominator). See also discussion on DERBY-3823 .
          Hide
          Dag H. Wanvik added a comment -

          When this issues is resolved, the test added to AlterTableTest in svn 1182570 should be modified to assert the now corrected behavior.

          Show
          Dag H. Wanvik added a comment - When this issues is resolved, the test added to AlterTableTest in svn 1182570 should be modified to assert the now corrected behavior.
          Hide
          Dag H. Wanvik added a comment -

          From looking at the code in client PreparedStatement#getMetaData, which calls getMetadataX, there is no attempt to verify that the prepared statement's metadata is still up-to-date. Repreparing the ps does bring it up to date, though. Perhaps we should just disabled this caching and reprepare whenever PreparedStatement#getMetaData is called. On the server this will not necessarily lead to a recompilation, of course. It does, however, incur a client server round-trip, but since we don't have any push service, I think polling is the only way to get the correct behavior.

          But it doesn't stop there: if we (re)execute the prepared statement (for which the cached metadata on the client is out of date), a new result set resulting from a query using that ps will inherit the stale metadata from the ps, cf. Statement#completeOpenQuery ca line 1493:

          resultSet.resultSetMetaData_ = resultSetMetaData_;

          so the metadata for the newly retrieved result set will also be stale (since the execution doesn't presently signal that the metadata has changed in any way). Thinking aloud, maybe we could version the metadata and attach the version number to the query result. If the cached version were out of date the client would need to retrieve the result set's metadata before closing the result set (so it would be guaranteed to be correct: the server does keep the info as long as the rs is open I think).

          Show
          Dag H. Wanvik added a comment - From looking at the code in client PreparedStatement#getMetaData, which calls getMetadataX, there is no attempt to verify that the prepared statement's metadata is still up-to-date. Repreparing the ps does bring it up to date, though. Perhaps we should just disabled this caching and reprepare whenever PreparedStatement#getMetaData is called. On the server this will not necessarily lead to a recompilation, of course. It does, however, incur a client server round-trip, but since we don't have any push service, I think polling is the only way to get the correct behavior. But it doesn't stop there: if we (re)execute the prepared statement (for which the cached metadata on the client is out of date), a new result set resulting from a query using that ps will inherit the stale metadata from the ps, cf. Statement#completeOpenQuery ca line 1493: resultSet.resultSetMetaData_ = resultSetMetaData_; so the metadata for the newly retrieved result set will also be stale (since the execution doesn't presently signal that the metadata has changed in any way). Thinking aloud, maybe we could version the metadata and attach the version number to the query result. If the cached version were out of date the client would need to retrieve the result set's metadata before closing the result set (so it would be guaranteed to be correct: the server does keep the info as long as the rs is open I think).
          Hide
          Dag H. Wanvik added a comment -

          I confirmed that something like:

          private ColumnMetaData getMetaDataX() throws SqlException

          { super.checkForClosedStatement(); + initResetPreparedStatement(); + prepare(); return resultSetMetaData_; }

          did give the correct result for the case of PreparedStatement#getMetaData in the repro. But how to fix ResultSet#getMetaData is probably not so easy. I'll try to find out of the protocol even allows metadata transfer outside or doing a prepare...

          Show
          Dag H. Wanvik added a comment - I confirmed that something like: private ColumnMetaData getMetaDataX() throws SqlException { super.checkForClosedStatement(); + initResetPreparedStatement(); + prepare(); return resultSetMetaData_; } did give the correct result for the case of PreparedStatement#getMetaData in the repro. But how to fix ResultSet#getMetaData is probably not so easy. I'll try to find out of the protocol even allows metadata transfer outside or doing a prepare...
          Hide
          Dag H. Wanvik added a comment -

          Note to self (may not be relevant): Found this little comment in NetStatementReply#parseSQLDTARDarray:

          // add a quick check to see if the table is altered (columns are added or deleted)
          // before reusing the cached cursor. note: this check does not catch the case
          // where the number of columns stay the same, but the column type or length changes,
          // i.e. from integer to char.

          Show
          Dag H. Wanvik added a comment - Note to self (may not be relevant): Found this little comment in NetStatementReply#parseSQLDTARDarray: // add a quick check to see if the table is altered (columns are added or deleted) // before reusing the cached cursor. note: this check does not catch the case // where the number of columns stay the same, but the column type or length changes, // i.e. from integer to char.
          Hide
          Kathey Marsden added a comment -

          Is this issue a duplicate of DERBY-2402 or are the issues different problems related to alter table and client?

          Show
          Kathey Marsden added a comment - Is this issue a duplicate of DERBY-2402 or are the issues different problems related to alter table and client?
          Hide
          Dag H. Wanvik added a comment -

          Yes, Kathey, I believe DERBY-2402 is the same problem. When I have a fix to prove it, I'll make them duplicates. For now, I link it as related.

          Show
          Dag H. Wanvik added a comment - Yes, Kathey, I believe DERBY-2402 is the same problem. When I have a fix to prove it, I'll make them duplicates. For now, I link it as related.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a trial patch, derby-5459-1, which fixes the problem in the following way: Each time we (recompile) a statement, a counter is incremented. When we first explicitly prepare a statement from the client, we make a note in DRDAStatement to which version of the compiled statement the prepare corresponds. Later, when executing the statement, if it returns data, we compare the current version of the prepared statement with the one current when the stament was compiled. If the versions differ we can be sure the client has stale metadata for the prepared statement, and we attach a SQLDARD block to the retiurned data. This is picked up by the client ok, it seems, cf the code in NetStatementReply#parseOpenQuery:

          if (peekCP == CodePoint.SQLDARD)

          The patch also updates the test PrepStmtMetaDataTest to assert metadata for the result set after an ALTER statement.

          Note, DERBY-2402 also notes that before executing a query after the ALTER, the client's ps metadata is out of synch. The patch does not address this problem: it is fairly easy make the client's getMetaData method always do a reprepare but this may not be what we want.

          Comments welcome on the approach as well as on the implementation: particularily, should we perhaps also do a reprepare when PS.getmetadata is called or is that too heavy handed..

          Running regressions.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a trial patch, derby-5459-1, which fixes the problem in the following way: Each time we (recompile) a statement, a counter is incremented. When we first explicitly prepare a statement from the client, we make a note in DRDAStatement to which version of the compiled statement the prepare corresponds. Later, when executing the statement, if it returns data, we compare the current version of the prepared statement with the one current when the stament was compiled. If the versions differ we can be sure the client has stale metadata for the prepared statement, and we attach a SQLDARD block to the retiurned data. This is picked up by the client ok, it seems, cf the code in NetStatementReply#parseOpenQuery: if (peekCP == CodePoint.SQLDARD) The patch also updates the test PrepStmtMetaDataTest to assert metadata for the result set after an ALTER statement. Note, DERBY-2402 also notes that before executing a query after the ALTER, the client's ps metadata is out of synch. The patch does not address this problem: it is fairly easy make the client's getMetaData method always do a reprepare but this may not be what we want. Comments welcome on the approach as well as on the implementation: particularily, should we perhaps also do a reprepare when PS.getmetadata is called or is that too heavy handed.. Running regressions.
          Hide
          Dag H. Wanvik added a comment -

          The regressions showed that one more test, AlterTableTest#testAddColumn, contained code to work around this issue for the client case. Uploading a patch that removes that special client code in the test, rerunning regressions.

          Show
          Dag H. Wanvik added a comment - The regressions showed that one more test, AlterTableTest#testAddColumn, contained code to work around this issue for the client case. Uploading a patch that removes that special client code in the test, rerunning regressions.
          Hide
          Dag H. Wanvik added a comment -

          Thinking more about this, I don't believe we should force a reprepare when (on the client) we do ps.getMetaData: since it may be become stale at any time between after the prepare and the execute, I think it is sufficient to re-send the metadata with the execute (current patch): it can't be fully trusted to predict the actual query results in the general case. Technically, this is in break of the Javadoc wording:

          "Retrieves a ResultSetMetaData object that contains information about the columns of the ResultSet object that will be returned when this PreparedStatement object is executed."

          If this were to always hold, we'd need to lock all involved tables from DDL operations till all prepared statements are closed. I don't think we want to go there. Or?

          Show
          Dag H. Wanvik added a comment - Thinking more about this, I don't believe we should force a reprepare when (on the client) we do ps.getMetaData: since it may be become stale at any time between after the prepare and the execute, I think it is sufficient to re-send the metadata with the execute (current patch): it can't be fully trusted to predict the actual query results in the general case. Technically, this is in break of the Javadoc wording: "Retrieves a ResultSetMetaData object that contains information about the columns of the ResultSet object that will be returned when this PreparedStatement object is executed." If this were to always hold, we'd need to lock all involved tables from DDL operations till all prepared statements are closed. I don't think we want to go there. Or?
          Hide
          Dag H. Wanvik added a comment -

          Regressions passed. I'll await a review before I commit this this change. Experienced DRDA folks might want to check if what I am doing here violates the protocol: although the client code accepts the SQLDARD block in parseOpenQuery, that could be "accidental" (intended for other cases). I tried to grok the standard on this point; it seemed to me that for this block to be returned, the app should ask for it. However, I have not changed to client code to do this. Obviously, it's nice not to have the change the client code for compatibility reasons. And this the block is sent as a result of a server event, it would be impossible to have the client ask for it, unless we choose to always return it.

          Show
          Dag H. Wanvik added a comment - Regressions passed. I'll await a review before I commit this this change. Experienced DRDA folks might want to check if what I am doing here violates the protocol: although the client code accepts the SQLDARD block in parseOpenQuery, that could be "accidental" (intended for other cases). I tried to grok the standard on this point; it seemed to me that for this block to be returned, the app should ask for it. However, I have not changed to client code to do this. Obviously, it's nice not to have the change the client code for compatibility reasons. And this the block is sent as a result of a server event, it would be impossible to have the client ask for it, unless we choose to always return it.
          Hide
          Knut Anders Hatlen added a comment -

          I haven't checked what the DRDA spec says, but it looks like a reasonable way to fix this problem to me. I agree that we shouldn't force a re-prepare when calling getMetaData(), as there's no guarantee that the meta-data won't change again before execution. (That said, if the underlying tables have changed in a way so that the types and names of the returned columns are no longer the same, it could possibly be argued that it's more appropriate to let the execution fail and force the user to recompile manually, since she might be executing a completely different query that the one she initially compiled, which may not be what she wants. Outside the scope of this issue, though.)

          Show
          Knut Anders Hatlen added a comment - I haven't checked what the DRDA spec says, but it looks like a reasonable way to fix this problem to me. I agree that we shouldn't force a re-prepare when calling getMetaData(), as there's no guarantee that the meta-data won't change again before execution. (That said, if the underlying tables have changed in a way so that the types and names of the returned columns are no longer the same, it could possibly be argued that it's more appropriate to let the execution fail and force the user to recompile manually, since she might be executing a completely different query that the one she initially compiled, which may not be what she wants. Outside the scope of this issue, though.)
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut! As for your parenthesis comment, I wonder how other databases treat this case. I'll have a look.

          Show
          Dag H. Wanvik added a comment - Thanks, Knut! As for your parenthesis comment, I wonder how other databases treat this case. I'll have a look.
          Hide
          Knut Anders Hatlen added a comment -

          I agree with Dag's reading of the DRDA spec. DRDA v4, vol 3 says that an OPNQRY command may return an SQLDARD object (see last paragraph on page 541). However, there are some variables in the OPNQRY command that controls whether or not to return it, so the opportunistic approach in the patch is probably not strictly adhering to the spec. Sounds like a reasonable extension, though, but it might warrant a code comment saying it is an extension (possibly also on the client side, so that no one accidentally removes the code that reads the SQLDARD object because the client code hasn't requested it).

          Before checking in the changes, it might be useful to run the regression tests with a modified patch that sends an SQLDARD unconditionally, and see if that smokes out hidden problems.

          Show
          Knut Anders Hatlen added a comment - I agree with Dag's reading of the DRDA spec. DRDA v4, vol 3 says that an OPNQRY command may return an SQLDARD object (see last paragraph on page 541). However, there are some variables in the OPNQRY command that controls whether or not to return it, so the opportunistic approach in the patch is probably not strictly adhering to the spec. Sounds like a reasonable extension, though, but it might warrant a code comment saying it is an extension (possibly also on the client side, so that no one accidentally removes the code that reads the SQLDARD object because the client code hasn't requested it). Before checking in the changes, it might be useful to run the regression tests with a modified patch that sends an SQLDARD unconditionally, and see if that smokes out hidden problems.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for reading the spec on this, Knut! Adding a comment that this is an extension sounds prudent. I'll also run the smoke test you propose, good idea.

          Show
          Dag H. Wanvik added a comment - Thanks for reading the spec on this, Knut! Adding a comment that this is an extension sounds prudent. I'll also run the smoke test you propose, good idea.
          Hide
          Dag H. Wanvik added a comment -

          Uploaded patch derby-5459-3 which updates comments. The stress test did not blow any fuses. Committed at svn 1205426, resolving.

          Show
          Dag H. Wanvik added a comment - Uploaded patch derby-5459-3 which updates comments. The stress test did not blow any fuses. Committed at svn 1205426, resolving.
          Hide
          Dag H. Wanvik added a comment -

          This is a backport candidate, but I am not doing it for now, so closing. Feel free to reopen and backport it if you think its valuable.

          Show
          Dag H. Wanvik added a comment - This is a backport candidate, but I am not doing it for now, so closing. Feel free to reopen and backport it if you think its valuable.
          Hide
          Bryan Pendleton added a comment -

          Hi Dag, thanks for working on this; it's been very interesting to follow along on your analysis.

          What is the (approximate, general) overhead of sending the result set metadata to the client?

          I guess I'm wondering if we should just always send that data. Such an approach might be
          simpler, and I'm always interested in raising the question of whether a simpler implementation
          might work adequately.

          Show
          Bryan Pendleton added a comment - Hi Dag, thanks for working on this; it's been very interesting to follow along on your analysis. What is the (approximate, general) overhead of sending the result set metadata to the client? I guess I'm wondering if we should just always send that data. Such an approach might be simpler, and I'm always interested in raising the question of whether a simpler implementation might work adequately.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for you interest in this one, Bryan!
          I did briefly consider it, it but I went for the more complex versioning approach since it does save some payload and seemed not unduly complex to me. I imagine the overhead isn't all that great so what you are suggesting may have merit. I'll see if I can measure it.

          Show
          Dag H. Wanvik added a comment - Thanks for you interest in this one, Bryan! I did briefly consider it, it but I went for the more complex versioning approach since it does save some payload and seemed not unduly complex to me. I imagine the overhead isn't all that great so what you are suggesting may have merit. I'll see if I can measure it.
          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
          ASF subversion and git services added a comment -

          Commit 1505734 from Mamta A. Satoor in branch 'code/branches/10'
          [ https://svn.apache.org/r1505734 ]

          DERBY-5459(Result set metadata are out of sync on client after underlying table is altered)

          Backporting to 10.8. Fix contributed by Dag.

          Show
          ASF subversion and git services added a comment - Commit 1505734 from Mamta A. Satoor in branch 'code/branches/10' [ https://svn.apache.org/r1505734 ] DERBY-5459 (Result set metadata are out of sync on client after underlying table is altered) Backporting to 10.8. Fix contributed by Dag.
          Hide
          ASF subversion and git services added a comment -

          Commit 1513639 from Myrna van Lunteren in branch 'code/branches/10.8'
          [ https://svn.apache.org/r1513639 ]

          DERBY-5459; Result set metadata are out of sync on client after underlying table is altered
          fix javadoc warning; merge of revision 1205757 from trunk.

          Show
          ASF subversion and git services added a comment - Commit 1513639 from Myrna van Lunteren in branch 'code/branches/10.8' [ https://svn.apache.org/r1513639 ] DERBY-5459 ; Result set metadata are out of sync on client after underlying table is altered fix javadoc warning; merge of revision 1205757 from trunk.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development