Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.0.2.2, 10.1.1.0, 10.1.2.1, 10.1.3.1
    • Fix Version/s: 10.2.2.1, 10.3.1.4
    • Component/s: Network Server
    • Labels:
      None
    • Urgency:
      Low
    • Bug behavior facts:
      Performance

      Description

      Reported by Kathey Marsden in DERBY-212:

      In reviewing the Network Server Code and profiling there were several
      areas that showed potential for providing performance
      improvement. Functions that need optimizing to prevent unneeded object
      creation and excessive decoding: parseSQLDTA_work()

      1. d815_regress.diff
        0.7 kB
        Dyre Tjeldvoll
      2. d815_regress.java
        3 kB
        A B
      3. derby-815_2.diff
        35 kB
        Dyre Tjeldvoll
      4. derby-815.diff
        30 kB
        Dyre Tjeldvoll
      5. derby-815.html
        3 kB
        Dyre Tjeldvoll
      6. derby-815.v3.diff
        36 kB
        Dyre Tjeldvoll
      7. derby-815.v3.html
        3 kB
        Dyre Tjeldvoll
      8. derby-815.v3.stat
        0.7 kB
        Dyre Tjeldvoll
      9. derby-815.v4.diff
        21 kB
        Dyre Tjeldvoll
      10. derby-815.v4.stat
        0.8 kB
        Dyre Tjeldvoll
      11. derby-815.v5.diff
        21 kB
        Dyre Tjeldvoll
      12. derby-815.v5.stat
        0.9 kB
        Dyre Tjeldvoll
      13. derby-815.v6.diff
        21 kB
        Dyre Tjeldvoll
      14. derbyall_report_with_jdk1.4.v3.txt
        13 kB
        Dyre Tjeldvoll
      15. derbyall_report.v4.txt
        45 kB
        Dyre Tjeldvoll

        Activity

        Hide
        Dyre Tjeldvoll added a comment -

        From profiling I have seen that this method has some sub-optimal use of Vectors, and I am close to submitting a patch for this. But I have not been able to see any unnecessary "decoding". At first I thought that CodePoint.FDODSC could be skipped when re-executing a statement that has already been exceuted, but running derbynet(client)mats with suitable instrumentation showed that an existing statement could receive parameters with different types, and different lengths. I also observed that these "type transitions" were different when running with the DB2 driver and the Derby driver (for the same test).

        Show
        Dyre Tjeldvoll added a comment - From profiling I have seen that this method has some sub-optimal use of Vectors, and I am close to submitting a patch for this. But I have not been able to see any unnecessary "decoding". At first I thought that CodePoint.FDODSC could be skipped when re-executing a statement that has already been exceuted, but running derbynet(client)mats with suitable instrumentation showed that an existing statement could receive parameters with different types, and different lengths. I also observed that these "type transitions" were different when running with the DB2 driver and the Derby driver (for the same test).
        Hide
        Dyre Tjeldvoll added a comment -

        Ran derbyall with the usual failures. I'll let the committers decide if it is ok to check in...

        Show
        Dyre Tjeldvoll added a comment - Ran derbyall with the usual failures. I'll let the committers decide if it is ok to check in...
        Hide
        Dyre Tjeldvoll added a comment -

        New patch that has been approved by Knut Anders. (New derbyall run also attached)

        Show
        Dyre Tjeldvoll added a comment - New patch that has been approved by Knut Anders. (New derbyall run also attached)
        Hide
        Dyre Tjeldvoll added a comment -

        Commited by Bernt Johnsen (r370806)

        Show
        Dyre Tjeldvoll added a comment - Commited by Bernt Johnsen (r370806)
        Hide
        Dyre Tjeldvoll added a comment -

        Actually, this fix has not been oommitted yet. The commit message mentioned the wrong issue number

        Show
        Dyre Tjeldvoll added a comment - Actually, this fix has not been oommitted yet. The commit message mentioned the wrong issue number
        Hide
        Bernt M. Johnsen added a comment -

        Committed revision 371897.

        Show
        Bernt M. Johnsen added a comment - Committed revision 371897.
        Hide
        A B added a comment -

        I have a test that runs using the DB2 client; before the patch for this issue was checked in, the test passed; now it hangs. To make sure, I synced my client at revision 371896 and the test passed; I then synced it to 371897 and it hung--so it does indeed appear to be caused by this checkin.

        I'm still trying to figure out what part of the test is causing the issue, and am trying to create a repro using either the Derby Client or the JCC. I will post more when I find out. In the meantime, I'm reopening this issue for tracking...

        Show
        A B added a comment - I have a test that runs using the DB2 client; before the patch for this issue was checked in, the test passed; now it hangs. To make sure, I synced my client at revision 371896 and the test passed; I then synced it to 371897 and it hung--so it does indeed appear to be caused by this checkin. I'm still trying to figure out what part of the test is causing the issue, and am trying to create a repro using either the Derby Client or the JCC. I will post more when I find out. In the meantime, I'm reopening this issue for tracking...
        Hide
        Dyre Tjeldvoll added a comment -

        Interesting... If you post your repro I'll run it and try to figure out what is wrong

        Show
        Dyre Tjeldvoll added a comment - Interesting... If you post your repro I'll run it and try to figure out what is wrong
        Hide
        A B added a comment -

        I was finally able to get a repro for this issue against Derby Client.

        For what it's worth, there's one comment in your patch that looks suspicious, namely, the last three lines of the following code block:

        + if (cp.drdaType != drdaType ||
        + cp.drdaLength != drdaLength) {
        + if (SanityManager.DEBUG)

        { + trace(ps+" param " +j+ + " type: "+ + drdaTypeName(cp.drdaType)+ + "->"+drdaTypeName(drdaType)+ + " length: "+ + cp.drdaLength+"->"+drdaLength); + }

        + // Trust that this is OK...
        + cp.drdaType = drdaType;
        + cp.drdaLength = drdaLength;

        When I turned tracing on and ran the repro, the output from the Sanity.DEBUG block was printed, which perhaps means that the assignment is not okay...?

        To run the repro, start the server on port 1527, then run

        > java d815_regress

        Please let me know if that doesn't reproduce the problem for you. Note that, for whatever reason, I had to do a full ant clobber and rebuild in order for the repro to actually work...

        Show
        A B added a comment - I was finally able to get a repro for this issue against Derby Client. For what it's worth, there's one comment in your patch that looks suspicious, namely, the last three lines of the following code block: + if (cp.drdaType != drdaType || + cp.drdaLength != drdaLength) { + if (SanityManager.DEBUG) { + trace(ps+" param " +j+ + " type: "+ + drdaTypeName(cp.drdaType)+ + "->"+drdaTypeName(drdaType)+ + " length: "+ + cp.drdaLength+"->"+drdaLength); + } + // Trust that this is OK... + cp.drdaType = drdaType; + cp.drdaLength = drdaLength; When I turned tracing on and ran the repro, the output from the Sanity.DEBUG block was printed, which perhaps means that the assignment is not okay...? To run the repro, start the server on port 1527, then run > java d815_regress Please let me know if that doesn't reproduce the problem for you. Note that, for whatever reason, I had to do a full ant clobber and rebuild in order for the repro to actually work...
        Hide
        A B added a comment -

        Re-attaching the repro, without excess "garbage" that I had sitting there from other tests...

        Show
        A B added a comment - Re-attaching the repro, without excess "garbage" that I had sitting there from other tests...
        Hide
        Dyre Tjeldvoll added a comment -

        I've tried the repro, and I see the same hang. I'm looking into it.

        Show
        Dyre Tjeldvoll added a comment - I've tried the repro, and I see the same hang. I'm looking into it.
        Hide
        Dyre Tjeldvoll added a comment -

        AB, your comment about the patch almost hit the nail on the head, but not quite. The CliParam struct has field called isExt that indicates that the parameter is external (in its own DSS). Just like the drdaType and the drdaLength filelds can change (to my surprise), the isExt can go from true to false for the same parameter in the same statement. In this case param 7 had been marked as external, but was internal in this message. Hence the attempt to read the separate DSS left the server waiting for more data, data which the client was never going to send.

        With the following change your repro goes through:

        Index: java/drda/org/apache/derby/impl/drda/DRDAConnThread.java
        ===================================================================
        — java/drda/org/apache/derby/impl/drda/DRDAConnThread.java (revision 373136)
        +++ java/drda/org/apache/derby/impl/drda/DRDAConnThread.java (working copy)
        @@ -3889,6 +3889,8 @@
        }
        final DRDAStatement.CliParam cp =
        stmt.getCliParam(j);
        + cp.isExt = false; // <--- FIX
        +
        if (cp.drdaType != drdaType ||
        cp.drdaLength != drdaLength) {
        if (SanityManager.DEBUG) {

        I'll polish up a proper patch and attach it when I've run derbyall. Sorry about all the problems...

        Show
        Dyre Tjeldvoll added a comment - AB, your comment about the patch almost hit the nail on the head, but not quite. The CliParam struct has field called isExt that indicates that the parameter is external (in its own DSS). Just like the drdaType and the drdaLength filelds can change (to my surprise), the isExt can go from true to false for the same parameter in the same statement. In this case param 7 had been marked as external, but was internal in this message. Hence the attempt to read the separate DSS left the server waiting for more data, data which the client was never going to send. With the following change your repro goes through: Index: java/drda/org/apache/derby/impl/drda/DRDAConnThread.java =================================================================== — java/drda/org/apache/derby/impl/drda/DRDAConnThread.java (revision 373136) +++ java/drda/org/apache/derby/impl/drda/DRDAConnThread.java (working copy) @@ -3889,6 +3889,8 @@ } final DRDAStatement.CliParam cp = stmt.getCliParam(j); + cp.isExt = false; // <--- FIX + if (cp.drdaType != drdaType || cp.drdaLength != drdaLength) { if (SanityManager.DEBUG) { I'll polish up a proper patch and attach it when I've run derbyall. Sorry about all the problems...
        Hide
        Dyre Tjeldvoll added a comment -

        Patch for the regression reported by AB.

        Show
        Dyre Tjeldvoll added a comment - Patch for the regression reported by AB.
        Hide
        A B added a comment -

        [ including my comments on the regression patch, copied from the derby-dev mailing list ]

        Dyre Tjeldvoll (JIRA) wrote:
        >
        > AB, your comment about the patch almost hit the nail on the head, but not quite.
        > The CliParam struct has field called isExt that indicates that the parameter is
        > external (in its own DSS). Just like the drdaType and the drdaLength filelds can
        > change (to my surprise), the isExt can go from true to false for the same parameter
        > in the same statement. In this case param 7 had been marked as external, but was
        > internal in this message. Hence the attempt to read the separate DSS left the
        > server waiting for more data, data which the client was never going to send.

        Thanks for looking into this regression. I (AB) applied the patch and ran my
        ODBC tests again, and the hang I reported is now gone. That said, I'm now
        seeing a different, intermittent hang in one of the multi-threaded tests that I
        have (again for ODBC). I can't say for sure if this intermittent hang is the
        result of the DERBY-815 changes or just a problem with the test--I'm still
        trying to figure that out. While I investigate the matter, I'm wondering if, by
        chance, you can think of anything in your DERBY-815 patch that might be
        affecting the server when running with a multi-threaded client? I'd be grateful
        for any ideas or suggestions that you might have there...

        Regarding your proposed fix for the regression:

        > With the following change your repro goes through:
        >
        > final DRDAStatement.CliParam cp =
        > stmt.getCliParam(j);
        > + cp.isExt = false; // <--- FIX
        > +
        > if (cp.drdaType != drdaType ||
        > cp.drdaLength != drdaLength) {
        > if (SanityManager.DEBUG) {
        >
        > I'll polish up a proper patch and attach it when I've run derbyall. Sorry about all the problems...

        Three things regarding the d815_regress.diff patch that you posted:

        1) The need for this fix is apparently non-intuitive--you yourself mentioned
        that you were "surprised" by that fact that "isExt can go from true to false for
        the same parameter in the same statement". So it'd be nice if you could add
        comments to the code indicating why this particular line (cp.isExt = false) is
        required, and when such a scenario can happen. If you were surprised, you can
        bet others will be, too Three or four years from now, when you or I or
        someone who's never looked at this code before sees this line, they could very
        well go "Ummm...what's that for?" and then subsequently delete it, or maybe even
        do worse--and the derbyall tests wouldn't catch it, and thus the regression
        would yet again be reintroduced. Which leads me to #2...

        2) It would be great if you could take the repro for the regression and
        incorporate it into one of the tests in the derbyall suite, so that it runs
        every night. That way, if someone does go through and delete the above line
        from the code, the nightlies will catch it. It's always desirable to have a
        test case submitted with code changes; the original patch for DERBY-815 was sort
        of an exception, since the theory was that it wasn't changing/adding any
        functionality and thus existing tests would be sufficient. But for the
        regression, we have a definite test case for "broken" and "fixed", so I think we
        should have that test case as part of the nightlies.

        3) Okay, so it looks like my guess regarding the following code was wrong:

        + // Trust that this is OK...
        + cp.drdaType = drdaType;
        + cp.drdaLength = drdaLength;

        That said, I have to admit that I'm uncomfortable with "Trust that this is
        OK..." as a comment in a final patch. The comment itself makes it seem like
        this is a bit of guess-work coding--sort of a "well I don't know why we need to
        do this, but let's try it and cross our fingers" thing. I'm assuming that
        that's not the case, so it would be extremely helpful to reword the comment to
        explain why we need to do this assignment. When I ran into the regression and
        started looking at the patch, I sat there and thought "Okay, what does that
        mean? Why was this code added? What happens if I change it? What am I
        'trusting' in? What did the developer have in mind when he added this code?"
        And that's why I pointed it out in my comment when I posted the repro. So at
        the very least, it's a comment that can cause developers to spend time
        scratching their heads and wondering whether or not this particular code might
        be the cause of some server problem that they're having (like, say, a hang with
        an ODBC client...

        Thanks again for addressing this regression so quickly. I don't mean to
        disregard your efforts nor your fix--I appreciate both! I just think it might
        be helpful for future developers if the new code contained some additional
        explanatory comments...

        Show
        A B added a comment - [ including my comments on the regression patch, copied from the derby-dev mailing list ] Dyre Tjeldvoll (JIRA) wrote: > > AB, your comment about the patch almost hit the nail on the head, but not quite. > The CliParam struct has field called isExt that indicates that the parameter is > external (in its own DSS). Just like the drdaType and the drdaLength filelds can > change (to my surprise), the isExt can go from true to false for the same parameter > in the same statement. In this case param 7 had been marked as external, but was > internal in this message. Hence the attempt to read the separate DSS left the > server waiting for more data, data which the client was never going to send. Thanks for looking into this regression. I (AB) applied the patch and ran my ODBC tests again, and the hang I reported is now gone. That said, I'm now seeing a different, intermittent hang in one of the multi-threaded tests that I have (again for ODBC). I can't say for sure if this intermittent hang is the result of the DERBY-815 changes or just a problem with the test--I'm still trying to figure that out. While I investigate the matter, I'm wondering if, by chance, you can think of anything in your DERBY-815 patch that might be affecting the server when running with a multi-threaded client? I'd be grateful for any ideas or suggestions that you might have there... Regarding your proposed fix for the regression: > With the following change your repro goes through: > > final DRDAStatement.CliParam cp = > stmt.getCliParam(j); > + cp.isExt = false; // <--- FIX > + > if (cp.drdaType != drdaType || > cp.drdaLength != drdaLength) { > if (SanityManager.DEBUG) { > > I'll polish up a proper patch and attach it when I've run derbyall. Sorry about all the problems... Three things regarding the d815_regress.diff patch that you posted: 1) The need for this fix is apparently non-intuitive--you yourself mentioned that you were "surprised" by that fact that "isExt can go from true to false for the same parameter in the same statement". So it'd be nice if you could add comments to the code indicating why this particular line (cp.isExt = false) is required, and when such a scenario can happen. If you were surprised, you can bet others will be, too Three or four years from now, when you or I or someone who's never looked at this code before sees this line, they could very well go "Ummm...what's that for?" and then subsequently delete it, or maybe even do worse--and the derbyall tests wouldn't catch it, and thus the regression would yet again be reintroduced. Which leads me to #2... 2) It would be great if you could take the repro for the regression and incorporate it into one of the tests in the derbyall suite, so that it runs every night. That way, if someone does go through and delete the above line from the code, the nightlies will catch it. It's always desirable to have a test case submitted with code changes; the original patch for DERBY-815 was sort of an exception, since the theory was that it wasn't changing/adding any functionality and thus existing tests would be sufficient. But for the regression, we have a definite test case for "broken" and "fixed", so I think we should have that test case as part of the nightlies. 3) Okay, so it looks like my guess regarding the following code was wrong: + // Trust that this is OK... + cp.drdaType = drdaType; + cp.drdaLength = drdaLength; That said, I have to admit that I'm uncomfortable with "Trust that this is OK..." as a comment in a final patch. The comment itself makes it seem like this is a bit of guess-work coding--sort of a "well I don't know why we need to do this, but let's try it and cross our fingers" thing. I'm assuming that that's not the case, so it would be extremely helpful to reword the comment to explain why we need to do this assignment. When I ran into the regression and started looking at the patch, I sat there and thought "Okay, what does that mean? Why was this code added? What happens if I change it? What am I 'trusting' in? What did the developer have in mind when he added this code?" And that's why I pointed it out in my comment when I posted the repro. So at the very least, it's a comment that can cause developers to spend time scratching their heads and wondering whether or not this particular code might be the cause of some server problem that they're having (like, say, a hang with an ODBC client... Thanks again for addressing this regression so quickly. I don't mean to disregard your efforts nor your fix--I appreciate both! I just think it might be helpful for future developers if the new code contained some additional explanatory comments...
        Hide
        Dyre Tjeldvoll added a comment -

        AB, the patch substitutes ArrayLists (not synchronized) for Vectors (synchronized), so with your MT-problems in mind I suggest we get a committer to revert the patch. Maybe I can look at it again when you have committed your work.

        I must say that it seems very unlikey that DRDAStatements (which originally contained both Vectors and ArrayLists) can be shared between threads. If it worked before it must be by accident...

        Show
        Dyre Tjeldvoll added a comment - AB, the patch substitutes ArrayLists (not synchronized) for Vectors (synchronized), so with your MT-problems in mind I suggest we get a committer to revert the patch. Maybe I can look at it again when you have committed your work. I must say that it seems very unlikey that DRDAStatements (which originally contained both Vectors and ArrayLists) can be shared between threads. If it worked before it must be by accident...
        Hide
        A B added a comment -

        > AB, the patch substitutes ArrayLists (not synchronized) for Vectors (synchronized), so with
        > your MT-problems in mind I suggest we get a committer to revert the patch.

        Note that, as I mentioned in my previous comment, I am not able to say with certainty that the DERBY-815 patch is responsible for the intermittent hang--that has yet to be proven one way or the other. But that said, if you're okay with reverting the patch, I think I would prefer that since I have a lot of things on my plate right now and further investigation of this intermittent hang may not happen for another couple of days.

        > Maybe I can look at it again when you have committed your work.

        I'm not actively working on any changes related to this issue, so I don't think there's much point in waiting for me to "commit" something here I do have a lot of things in progress, but none address this part of the code: I simply noticed that the ODBC tests I have are acting up after DERBY-815 was committed, and I was just following up in an attempt to find out why...

        If you are willing, I think the best thing here is to revert the patch for now, and re-submit it with 1) the fix for the regression, 2) the test case for the regression included as part of derbyall, and 3) some additional explanatory comments (per my last post). If you have time to investigate the multithreaded angle a bit , that would be great; if not, I think we could still commit an updated patch and I then can follow-up with more on the multithreaded test case when I have time. If it turns out that the DERBY-815 change are the cause (which we don't know yet), I can create another Jira issue with more details...

        Does that seem reasonable?

        > I must say that it seems very unlikey that DRDAStatements (which originally contained
        > both Vectors and ArrayLists) can be shared between threads. If it worked before it must be
        > by accident...

        That could very well be the case; I can't say for sure at this point.

        If you are in fact okay with reverting the patch and working on a revised version, please post here saying so, and then we can ask a commiter to back the change out.

        Thanks for your continued follow-up with this issue...

        Show
        A B added a comment - > AB, the patch substitutes ArrayLists (not synchronized) for Vectors (synchronized), so with > your MT-problems in mind I suggest we get a committer to revert the patch. Note that, as I mentioned in my previous comment, I am not able to say with certainty that the DERBY-815 patch is responsible for the intermittent hang--that has yet to be proven one way or the other. But that said, if you're okay with reverting the patch, I think I would prefer that since I have a lot of things on my plate right now and further investigation of this intermittent hang may not happen for another couple of days. > Maybe I can look at it again when you have committed your work. I'm not actively working on any changes related to this issue, so I don't think there's much point in waiting for me to "commit" something here I do have a lot of things in progress, but none address this part of the code: I simply noticed that the ODBC tests I have are acting up after DERBY-815 was committed, and I was just following up in an attempt to find out why... If you are willing, I think the best thing here is to revert the patch for now, and re-submit it with 1) the fix for the regression, 2) the test case for the regression included as part of derbyall, and 3) some additional explanatory comments (per my last post). If you have time to investigate the multithreaded angle a bit , that would be great ; if not, I think we could still commit an updated patch and I then can follow-up with more on the multithreaded test case when I have time. If it turns out that the DERBY-815 change are the cause (which we don't know yet), I can create another Jira issue with more details... Does that seem reasonable? > I must say that it seems very unlikey that DRDAStatements (which originally contained > both Vectors and ArrayLists) can be shared between threads. If it worked before it must be > by accident... That could very well be the case; I can't say for sure at this point. If you are in fact okay with reverting the patch and working on a revised version, please post here saying so, and then we can ask a commiter to back the change out. Thanks for your continued follow-up with this issue...
        Hide
        Dyre Tjeldvoll added a comment -

        Yes, we should go ahead and revert the issue.

        Show
        Dyre Tjeldvoll added a comment - Yes, we should go ahead and revert the issue.
        Hide
        Dyre Tjeldvoll added a comment -

        While converting AB's repro for the regression into a test (in the test harness) I discovered that the last execute triggers an unexpected exception in embedded mode (see the attached derby.log for details). I realize that to test the regression it is only necessary to run the test against the NetworkServer, but it shouldn't fail in embedded mode, should it?

        Show
        Dyre Tjeldvoll added a comment - While converting AB's repro for the regression into a test (in the test harness) I discovered that the last execute triggers an unexpected exception in embedded mode (see the attached derby.log for details). I realize that to test the regression it is only necessary to run the test against the NetworkServer, but it shouldn't fail in embedded mode, should it?
        Hide
        A B added a comment -

        Good catch. Actually, I think the error in embedded mode is probably correct, given the repro I posted. When we insert the 3rd row, we do two setBlobs and two setClobs; for the fourth row, we do a setNull for one of the blobs and for one of the clobs (params 7 and 9) but do not re-set the others (params 6 and 8), and hence this error occurs because we're trying to re-use the lob streams. If you add the following two lines to the block for the 4th row:

        pSt.setBlob(6, rs.getBlob(6));
        pSt.setClob(8, rs.getClob(8));

        then the test case will pass with embedded and still show the regression hang against the client. As for why the client didn't throw an error, I think it's because materialization of the lobs occurs on the client/server, so we're not reusing a "stream" per se, we're just reusing the materialized versions of the lobs. I believe this is related to DERBY-326 and DERBY-550... (thanks to Sunitha for the pointers!).

        Show
        A B added a comment - Good catch. Actually, I think the error in embedded mode is probably correct, given the repro I posted. When we insert the 3rd row, we do two setBlobs and two setClobs; for the fourth row, we do a setNull for one of the blobs and for one of the clobs (params 7 and 9) but do not re-set the others (params 6 and 8), and hence this error occurs because we're trying to re-use the lob streams. If you add the following two lines to the block for the 4th row: pSt.setBlob(6, rs.getBlob(6)); pSt.setClob(8, rs.getClob(8)); then the test case will pass with embedded and still show the regression hang against the client. As for why the client didn't throw an error, I think it's because materialization of the lobs occurs on the client/server, so we're not reusing a "stream" per se, we're just reusing the materialized versions of the lobs. I believe this is related to DERBY-326 and DERBY-550 ... (thanks to Sunitha for the pointers!).
        Hide
        Dyre Tjeldvoll added a comment -

        Ok, it is good to know that this is expected beahavior. Although, since this is a mistake that a user could easily make, it seems to me that a more user-friendly error message would have been preferable. Anyway, I have modified the test as suggested, and it works.

        Another thing: I have converted the repro into a separate test script. Based on the comments I got on DERBY-85, I'm thinking that I should try to integrate the repro into an existing test. That, however, gives me a problem. I wanted this repro to be run both with DerbyNet and DerbyNetClient, but based on the content of suites/derbynetmats.runnall and suites/derbynetclientmats.runall there doesn't appear to be any tests that are actually run in both frameworks. So which test should I extend? Any help on this would be much appreciated. Thanks.

        Show
        Dyre Tjeldvoll added a comment - Ok, it is good to know that this is expected beahavior. Although, since this is a mistake that a user could easily make, it seems to me that a more user-friendly error message would have been preferable. Anyway, I have modified the test as suggested, and it works. Another thing: I have converted the repro into a separate test script. Based on the comments I got on DERBY-85 , I'm thinking that I should try to integrate the repro into an existing test. That, however, gives me a problem. I wanted this repro to be run both with DerbyNet and DerbyNetClient, but based on the content of suites/derbynetmats.runnall and suites/derbynetclientmats.runall there doesn't appear to be any tests that are actually run in both frameworks. So which test should I extend? Any help on this would be much appreciated. Thanks.
        Hide
        A B added a comment -

        > I wanted this repro to be run both with DerbyNet and DerbyNetClient, but based on
        > the content of suites/derbynetmats.runnall and suites/derbynetclientmats.runall there
        > doesn't appear to be any tests that are actually run in both frameworks. So which test
        > should I extend? Any help on this would be much appreciated. Thanks.

        There are two places where the tests for a given suite are specified: the first is <suite>.runall, which specifies individual tests to run; the second is <suite>.properties, in which you can specify additional suites to run with the same configuration as the current suite.

        Thus, while derbynetmats.runall and derbynetclientmats.runall do not share any individual tests, a look at derbynetclientmats.properties shows the following:

        framework=DerbyNetClient
        suites= derbynetclientmats derbynetmats

        As I understand it, this means that all tests in derbynetclientmats.runall AND derbynetmats.runall will run as part of the "derbynetclientmats" suite using the DerbyNetClient framework. So as long as you add the new test case to a test that is in derbynetmats.runall, it will also run as part of derbynetclientmats (assuming the test is not listed in the DerbyNetClient.exclude file).

        That said, if you can find an appropriate test in either derbynetmats.runall or jdbcapi.runall, I think you could use that since both of those suites are run as part of derbynetmats and thus both are also run as part of derbynetclientmats. As I said, though, you'll just have to make sure whatever test you choose is not listed in either DerbyNet.exclude nor DerbyNetClient.exclude

        Does that help?

        Show
        A B added a comment - > I wanted this repro to be run both with DerbyNet and DerbyNetClient, but based on > the content of suites/derbynetmats.runnall and suites/derbynetclientmats.runall there > doesn't appear to be any tests that are actually run in both frameworks. So which test > should I extend? Any help on this would be much appreciated. Thanks. There are two places where the tests for a given suite are specified: the first is <suite>.runall, which specifies individual tests to run; the second is <suite>.properties, in which you can specify additional suites to run with the same configuration as the current suite. Thus, while derbynetmats.runall and derbynetclientmats.runall do not share any individual tests, a look at derbynetclientmats.properties shows the following: framework=DerbyNetClient suites= derbynetclientmats derbynetmats As I understand it, this means that all tests in derbynetclientmats.runall AND derbynetmats.runall will run as part of the "derbynetclientmats" suite using the DerbyNetClient framework. So as long as you add the new test case to a test that is in derbynetmats.runall, it will also run as part of derbynetclientmats (assuming the test is not listed in the DerbyNetClient.exclude file). That said, if you can find an appropriate test in either derbynetmats.runall or jdbcapi.runall, I think you could use that since both of those suites are run as part of derbynetmats and thus both are also run as part of derbynetclientmats. As I said, though, you'll just have to make sure whatever test you choose is not listed in either DerbyNet.exclude nor DerbyNetClient.exclude Does that help?
        Hide
        Dyre Tjeldvoll added a comment -

        Thanks! Yes, that explains a lot. I had a quick look at the tests in derbynetmats, and derbynet/prepStmt.java seems like a good candidate, since the problem appears when using prepared statements with the NetworkServer. I have checked that it isn't listed in either of the .exclude files. If nobody objects, I'll go ahead and add a new test case here.

        Show
        Dyre Tjeldvoll added a comment - Thanks! Yes, that explains a lot. I had a quick look at the tests in derbynetmats, and derbynet/prepStmt.java seems like a good candidate, since the problem appears when using prepared statements with the NetworkServer. I have checked that it isn't listed in either of the .exclude files. If nobody objects, I'll go ahead and add a new test case here.
        Hide
        Dyre Tjeldvoll added a comment -

        Patch description

        Show
        Dyre Tjeldvoll added a comment - Patch description
        Hide
        Dyre Tjeldvoll added a comment -

        New patch with testcase for the regression. Please review, but I think it would be wise to delay committing until AB has had a chance to run his ODBC tests with the patch. Thanks.

        Show
        Dyre Tjeldvoll added a comment - New patch with testcase for the regression. Please review, but I think it would be wise to delay committing until AB has had a chance to run his ODBC tests with the patch. Thanks.
        Hide
        A B added a comment -

        I applied the patch and ran my ODBC tests with no failures. I haven't had time to do a full review of the patch, but I did skim through and it looks like things are well commented now--so thanks for doing that.

        One note: the patch only updates the master files for jdk15; the "default" master file (master/prepStmt.out) wasn't updated, so if you run the new test with any JVM earlier than 1.5, there will be a diff. If you have time to try that out and include that master update in this patch, that'd be good.

        Thanks for taking the time to follow-up on my questions/concerns.

        Show
        A B added a comment - I applied the patch and ran my ODBC tests with no failures. I haven't had time to do a full review of the patch, but I did skim through and it looks like things are well commented now--so thanks for doing that. One note: the patch only updates the master files for jdk15; the "default" master file (master/prepStmt.out) wasn't updated, so if you run the new test with any JVM earlier than 1.5, there will be a diff. If you have time to try that out and include that master update in this patch, that'd be good. Thanks for taking the time to follow-up on my questions/concerns.
        Hide
        Dyre Tjeldvoll added a comment -

        The omission of the default master file was deliberate. It is explained in the updated patch description (derby-815.html) under the heading "Notes to reviewers".
        I chose not to touch the default since the unmodified test fails in embedded, both with java 1.4 and 1.5

        Show
        Dyre Tjeldvoll added a comment - The omission of the default master file was deliberate. It is explained in the updated patch description (derby-815.html) under the heading "Notes to reviewers". I chose not to touch the default since the unmodified test fails in embedded, both with java 1.4 and 1.5
        Hide
        A B added a comment -

        Okay, so the unmodified test fails in embedded. But the master file in master/prepStmt.out is not just for embedded; it also serves as the default master file for server mode if nothing else is found. In the case of server mode with a 1.4 jvm, the 1.5 masters don't apply, and since there's nothing else to use the default master/prepStmt.out will be used. Which is (I think) why that master file exists, even if the test doesn't run in embedded mode.

        The unmodified test passes in server mode with 1.4; the modified test fails in server mode with 1.4-and my guess is that that's not deliberate. So I think what's required here is that someone run the test in server mode against 1.4 and then copy that master file to master/prepStmt.out. Based on the "itch" philosophy, that someone doesn't have to be you-but in the meantime, any tests run with 1.4 will (so far as I understand it) fail with your changes.

        That's how I understand it, anyways. I'm not blocking the patch from being committed, I'm just pointing out a potential master-update problem that could lead to a new nightly test failure. shrug

        Show
        A B added a comment - Okay, so the unmodified test fails in embedded. But the master file in master/prepStmt.out is not just for embedded; it also serves as the default master file for server mode if nothing else is found. In the case of server mode with a 1.4 jvm, the 1.5 masters don't apply, and since there's nothing else to use the default master/prepStmt.out will be used. Which is (I think) why that master file exists, even if the test doesn't run in embedded mode. The unmodified test passes in server mode with 1.4; the modified test fails in server mode with 1.4- and my guess is that that's not deliberate. So I think what's required here is that someone run the test in server mode against 1.4 and then copy that master file to master/prepStmt.out. Based on the "itch" philosophy, that someone doesn't have to be you -but in the meantime, any tests run with 1.4 will (so far as I understand it) fail with your changes. That's how I understand it, anyways. I'm not blocking the patch from being committed, I'm just pointing out a potential master-update problem that could lead to a new nightly test failure. shrug
        Hide
        A B added a comment -

        > but in the meantime, any tests run with 1.4 will (so far as I understand it) fail with your changes.

        Check that, "any tests" isn' right; I meant "the prepStmt.java test" test. Sorry.

        Show
        A B added a comment - > but in the meantime, any tests run with 1.4 will (so far as I understand it) fail with your changes. Check that, "any tests" isn' right; I meant "the prepStmt.java test" test. Sorry.
        Hide
        Dyre Tjeldvoll added a comment -

        derby-815.v3.html is an updated description without the master comment. Thanks to AB for explaining this.

        Show
        Dyre Tjeldvoll added a comment - derby-815.v3.html is an updated description without the master comment. Thanks to AB for explaining this.
        Hide
        Dyre Tjeldvoll added a comment -

        New patch (derby-815.v3.*) which includes an updated default master that works with jdk1.4
        Ran derbyall with jdk1.4

        Show
        Dyre Tjeldvoll added a comment - New patch (derby-815.v3.*) which includes an updated default master that works with jdk1.4 Ran derbyall with jdk1.4
        Hide
        Andrew McIntyre added a comment -

        Committed to 10.1 branch with revision 398036.

        Show
        Andrew McIntyre added a comment - Committed to 10.1 branch with revision 398036.
        Hide
        Andrew McIntyre added a comment -

        Whoops. Previous comment meant for DERBY-85 not 815.

        Show
        Andrew McIntyre added a comment - Whoops. Previous comment meant for DERBY-85 not 815.
        Hide
        Kathey Marsden added a comment -

        I would like to look at committing this patch. Dyre I was wondering if you could sync it up. It seems to have become out of date since you posted it in February. Sorry for the long delay in looking at it.

        Show
        Kathey Marsden added a comment - I would like to look at committing this patch. Dyre I was wondering if you could sync it up. It seems to have become out of date since you posted it in February. Sorry for the long delay in looking at it.
        Hide
        Kathey Marsden added a comment -

        Should we uncheck patch available on this issue since it needs to be updated to apply?

        Show
        Kathey Marsden added a comment - Should we uncheck patch available on this issue since it needs to be updated to apply?
        Hide
        Dyre Tjeldvoll added a comment -

        Removing patch available flag, as requested.

        Show
        Dyre Tjeldvoll added a comment - Removing patch available flag, as requested.
        Hide
        Dyre Tjeldvoll added a comment -

        Attaching new patch (v4) based on revision 425020.

        Show
        Dyre Tjeldvoll added a comment - Attaching new patch (v4) based on revision 425020.
        Hide
        Dyre Tjeldvoll added a comment -

        Setting patch available for the v4 patch.

        Show
        Dyre Tjeldvoll added a comment - Setting patch available for the v4 patch.
        Hide
        Kathey Marsden added a comment -

        I am looking at this patch and have a couple of general questions

        1) I see that Bernt committed the original patch but I am having a hard time finding his original review comments in the mail archives. Are there comments from the initial review that I could refer to?

        2) I was wondering about the logic DRDAConnThread.java
        I was wonding if it might be possible to work this so that CliParam does not have to be exposed in DRDAConnThread? e.g just have a stmt.initCliParam(drdaType, drdaLength) that has all of this logic in it.

        final boolean haveParams = (stmt.getNumParams() > 0);
        [....]
        if (!haveParams)

        { // The statement has not been executed // before so a new CliParam object // must be added for each parameter stmt.addCliParam(drdaType, drdaLength); continue; }

        // This statement has been executed before
        // so we reuse the CliParam objects (to
        // save some cycles)
        final DRDAStatement.CliParam cp =
        stmt.getCliParam(j);
        // Clear out all info about the old
        // parameter and add info about the new one
        cp.init(drdaType, drdaLength);
        }

        Show
        Kathey Marsden added a comment - I am looking at this patch and have a couple of general questions 1) I see that Bernt committed the original patch but I am having a hard time finding his original review comments in the mail archives. Are there comments from the initial review that I could refer to? 2) I was wondering about the logic DRDAConnThread.java I was wonding if it might be possible to work this so that CliParam does not have to be exposed in DRDAConnThread? e.g just have a stmt.initCliParam(drdaType, drdaLength) that has all of this logic in it. final boolean haveParams = (stmt.getNumParams() > 0); [....] if (!haveParams) { // The statement has not been executed // before so a new CliParam object // must be added for each parameter stmt.addCliParam(drdaType, drdaLength); continue; } // This statement has been executed before // so we reuse the CliParam objects (to // save some cycles) final DRDAStatement.CliParam cp = stmt.getCliParam(j); // Clear out all info about the old // parameter and add info about the new one cp.init(drdaType, drdaLength); }
        Hide
        Kathey Marsden added a comment -

        My review comment about hiding CliParam and handling of clearing parameters etc from DRDAConnThread is my only comment for now on this patch. I do not really have my head around all the cases and handling for parameter setting and clearing etc, so look forward to hearing review comments from others on the patch to understand better. My feeble mind might also be able to sort it out better once this change is made. Can we uncheck patch available or would you like more comments on this version?

        Also I wanted to mention that if you have higher urgency items for 10.2, don't feel compelled to jump on changing this right away. It could easily go into the maintenance branch after releas. I just picked it up as my patch for the week.

        Thanks

        Kathey

        Show
        Kathey Marsden added a comment - My review comment about hiding CliParam and handling of clearing parameters etc from DRDAConnThread is my only comment for now on this patch. I do not really have my head around all the cases and handling for parameter setting and clearing etc, so look forward to hearing review comments from others on the patch to understand better. My feeble mind might also be able to sort it out better once this change is made. Can we uncheck patch available or would you like more comments on this version? Also I wanted to mention that if you have higher urgency items for 10.2, don't feel compelled to jump on changing this right away. It could easily go into the maintenance branch after releas. I just picked it up as my patch for the week. Thanks Kathey
        Hide
        Dyre Tjeldvoll added a comment -

        No I don't see this as urgent for 10.2. Wrt. to the visibility of CliParam in DRDAConnThread, I think you're making a valid point. However, CliParam was not added to improve encapsulation or code structure. It is just a utility to avoid having two containers of (pointers to) Integer/Long objects that were created and discarded again and again. I actually tried to preserve the existing program structure as much as possible, ("small incremental changes").

        Show
        Dyre Tjeldvoll added a comment - No I don't see this as urgent for 10.2. Wrt. to the visibility of CliParam in DRDAConnThread, I think you're making a valid point. However, CliParam was not added to improve encapsulation or code structure. It is just a utility to avoid having two containers of (pointers to) Integer/Long objects that were created and discarded again and again. I actually tried to preserve the existing program structure as much as possible, ("small incremental changes").
        Hide
        Mike Matrigali added a comment -

        From the comments I can't tell where this patch is. It looks like there was a review with some suggestions by Kathy who
        did not want to commit without changes. Dyre, do you still think this patch should be committed, or are you planning an
        update but it is just a low priority?

        Show
        Mike Matrigali added a comment - From the comments I can't tell where this patch is. It looks like there was a review with some suggestions by Kathy who did not want to commit without changes. Dyre, do you still think this patch should be committed, or are you planning an update but it is just a low priority?
        Hide
        Rick Hillegas added a comment -

        Moving to 10.2.2.0.

        Show
        Rick Hillegas added a comment - Moving to 10.2.2.0.
        Hide
        Dyre Tjeldvoll added a comment -

        Removing patch available, since the latest patch no longer can be applied to the trunk.

        Show
        Dyre Tjeldvoll added a comment - Removing patch available, since the latest patch no longer can be applied to the trunk.
        Hide
        Dyre Tjeldvoll added a comment -

        Here is a new patch that tries to address K. Marsden's concerns. The parameter book-keeping is now hidden inside DRDAStatement, and the interface has been extended accordingly.
        The testcase for the regression introduced by one of the earlier versions of the patch is still in the patch. Both derbyall and suites.All ran without failures. Please review. Thanks.

        Show
        Dyre Tjeldvoll added a comment - Here is a new patch that tries to address K. Marsden's concerns. The parameter book-keeping is now hidden inside DRDAStatement, and the interface has been extended accordingly. The testcase for the regression introduced by one of the earlier versions of the patch is still in the patch. Both derbyall and suites.All ran without failures. Please review. Thanks.
        Hide
        Knut Anders Hatlen added a comment -

        I think the patch looks good. A couple of minor issues that could be fixed for extra bonus:

        • addDrdaParamType() and addExtPos() contain similar code for growing an array. Could it be factored out?
        • jira815Test() lacks indentation (incorrect tab width?)
        • typos in comments in DRDAStatement: initailization, autmatically, paramters
        • typo in javadoc for jira815Test: intruduced
        Show
        Knut Anders Hatlen added a comment - I think the patch looks good. A couple of minor issues that could be fixed for extra bonus: addDrdaParamType() and addExtPos() contain similar code for growing an array. Could it be factored out? jira815Test() lacks indentation (incorrect tab width?) typos in comments in DRDAStatement: initailization, autmatically, paramters typo in javadoc for jira815Test: intruduced
        Hide
        Dyre Tjeldvoll added a comment -

        New patch which addresses KAH's comments. (No new stat file since no new files have been touched). derbyall and suites.All both passed.

        Show
        Dyre Tjeldvoll added a comment - New patch which addresses KAH's comments. (No new stat file since no new files have been touched). derbyall and suites.All both passed.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Dyre. Committed revision 467612.

        Show
        Knut Anders Hatlen added a comment - Thanks Dyre. Committed revision 467612.
        Hide
        Kathey Marsden added a comment -

        Reopen to backport to 10.2 so that DERBY-3085 can be more easily backported.

        Show
        Kathey Marsden added a comment - Reopen to backport to 10.2 so that DERBY-3085 can be more easily backported.

          People

          • Assignee:
            Dyre Tjeldvoll
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development