Derby
  1. Derby
  2. DERBY-3839

Convert "org.apache.derbyTesting.functionTests.tests.store.holdCursorJDBC30.sql" to junit.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: Test
    • Labels:
      None
    • Issue & fix info:
      Patch Available
    1. derby-3839.patch
      143 kB
      Tiago R. Espinha
    2. derby-3839.patch
      140 kB
      Tiago R. Espinha
    3. derby-3839.patch
      135 kB
      Tiago R. Espinha
    4. derby-3839.stat
      0.7 kB
      Tiago R. Espinha
    5. derby-3839-1.patch
      223 kB
      Junjie Peng
    6. derby-3839-1.stat
      0.8 kB
      Junjie Peng
    7. ReproHoldCursorBug.java
      3 kB
      Tiago R. Espinha
    8. ReproHoldCursorBug.java
      3 kB
      Tiago R. Espinha
    9. ReproHoldCursorBug.java
      3 kB
      Tiago R. Espinha

      Issue Links

        Activity

        Hide
        Junjie Peng added a comment -

        Please check the patch, thanks!

        Show
        Junjie Peng added a comment - Please check the patch, thanks!
        Hide
        Mamta A. Satoor added a comment -

        Junjie, thanks for picking this really long test for conversion. The conversion job has been done pretty well. I just have following comments. (the comments might look little scattered because I wrote them down as I went through each of the subtest conversion.)

        1)The first thing the test does is set autocommit to false. Shouldn't each of the junit test fixture do the same?

        2)In several places in the original test, it does close on resultset first and then commit but the converted junit test appears to do it in reverse order.

        3)The converted junit test has fixture testBasicBtreeScanForZeroRowsUpdateNonkeyfield. This test uses for update of clause here but the some of the other tests didn't.

        4)The converted junit test has fixture testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField. It needs to define index like the earlier test stUtil.executeUpdate("create index foox on foo (a)"); This will match what the original non-junit test intended.

        5)The following set of tests from original test are missing in the junit test. I think it could go in test fixutre testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField
        ij> – test negative case of trying non next operations after commit
        get with hold cursor test1 as
        'select * from foo for update of data';
        ij> next test1;
        A |DATA
        -----------------------
        1 |10
        ij> commit;
        ij> delete from foo where current of test1;
        ERROR 24000: Invalid cursor state - no current row.
        ij> next test1;
        A |DATA
        -----------------------
        1 |20
        ij> commit;
        ij> update foo set data=-3000 where current of test1;
        ERROR 24000: Invalid cursor state - no current row.
        ij> next test1;
        A |DATA
        -----------------------
        1 |30
        ij> next test1;
        A |DATA
        -----------------------
        1 |40
        ij> next test1;
        A |DATA
        -----------------------
        1 |50
        ij> next test1;
        No current row
        ij> close test1;
        ij> commit;
        ij> – should fail
        next test1;
        IJ ERROR: Unable to establish cursor

        6)There is a test as follows in the original test
        ij> – make sure above deletes/updates worked.
        get with hold cursor test1 as
        'select * from foo for update of data';
        ij> next test1;
        A |DATA
        -----------------------
        1 |10
        ij> commit;
        ij> next test1;
        A |DATA
        -----------------------
        1 |30
        ij> commit;
        ij> next test1;
        A |DATA
        -----------------------
        1 |-3000
        The test above checks that update made earlier shows up. The converted test does not look for new value -3000. The converted test for above test is as below (it is in testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField)
        + //make sure above deletes/updates worked.
        + test1 = st.executeQuery("select * from foo for update of data");
        + assertTrue(test1.next());
        + assertEquals(1, test1.getRow());
        + commit();
        + assertTrue(test1.next());
        + assertEquals(2, test1.getRow());
        + commit();
        + assertTrue(test1.next());
        + assertEquals(3, test1.getRow());

        7)The converted junit test has fixture testBasicBtreeScanTestsForMultipleRowsOrReadOnly. It should create an index on foo to match the original test.

        8)There are several places in the test where it matters what was the exact row data retruned on a "next.." but the converted test always just checks for test1.getRow() rather than the actual row data. One of those examples would be Test 7, Test 8, Test 9. There might be other places in the test where we should look for the row data rather than row number.

        Show
        Mamta A. Satoor added a comment - Junjie, thanks for picking this really long test for conversion. The conversion job has been done pretty well. I just have following comments. (the comments might look little scattered because I wrote them down as I went through each of the subtest conversion.) 1)The first thing the test does is set autocommit to false. Shouldn't each of the junit test fixture do the same? 2)In several places in the original test, it does close on resultset first and then commit but the converted junit test appears to do it in reverse order. 3)The converted junit test has fixture testBasicBtreeScanForZeroRowsUpdateNonkeyfield. This test uses for update of clause here but the some of the other tests didn't. 4)The converted junit test has fixture testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField. It needs to define index like the earlier test stUtil.executeUpdate("create index foox on foo (a)"); This will match what the original non-junit test intended. 5)The following set of tests from original test are missing in the junit test. I think it could go in test fixutre testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField ij> – test negative case of trying non next operations after commit get with hold cursor test1 as 'select * from foo for update of data'; ij> next test1; A |DATA ----------------------- 1 |10 ij> commit; ij> delete from foo where current of test1; ERROR 24000: Invalid cursor state - no current row. ij> next test1; A |DATA ----------------------- 1 |20 ij> commit; ij> update foo set data=-3000 where current of test1; ERROR 24000: Invalid cursor state - no current row. ij> next test1; A |DATA ----------------------- 1 |30 ij> next test1; A |DATA ----------------------- 1 |40 ij> next test1; A |DATA ----------------------- 1 |50 ij> next test1; No current row ij> close test1; ij> commit; ij> – should fail next test1; IJ ERROR: Unable to establish cursor 6)There is a test as follows in the original test ij> – make sure above deletes/updates worked. get with hold cursor test1 as 'select * from foo for update of data'; ij> next test1; A |DATA ----------------------- 1 |10 ij> commit; ij> next test1; A |DATA ----------------------- 1 |30 ij> commit; ij> next test1; A |DATA ----------------------- 1 |-3000 The test above checks that update made earlier shows up. The converted test does not look for new value -3000. The converted test for above test is as below (it is in testBasicBtreeScanTestsForMultipleRowsOrUpdateNonkeyField) + //make sure above deletes/updates worked. + test1 = st.executeQuery("select * from foo for update of data"); + assertTrue(test1.next()); + assertEquals(1, test1.getRow()); + commit(); + assertTrue(test1.next()); + assertEquals(2, test1.getRow()); + commit(); + assertTrue(test1.next()); + assertEquals(3, test1.getRow()); 7)The converted junit test has fixture testBasicBtreeScanTestsForMultipleRowsOrReadOnly. It should create an index on foo to match the original test. 8)There are several places in the test where it matters what was the exact row data retruned on a "next.." but the converted test always just checks for test1.getRow() rather than the actual row data. One of those examples would be Test 7, Test 8, Test 9. There might be other places in the test where we should look for the row data rather than row number.
        Hide
        Junjie Peng added a comment -

        Hi, Mamta. Thanks for attention. I'm sorry to return lately as not free these days. I will provide a new patch some time later.

        Show
        Junjie Peng added a comment - Hi, Mamta. Thanks for attention. I'm sorry to return lately as not free these days. I will provide a new patch some time later.
        Hide
        Kristian Waagan added a comment -

        Clearing patch available flag, a new patch is expected later.

        Show
        Kristian Waagan added a comment - Clearing patch available flag, a new patch is expected later.
        Hide
        Tiago R. Espinha added a comment -

        Assigning this issue to me as it is related to DERBY-4090 and I'm continuing its resolution.

        Show
        Tiago R. Espinha added a comment - Assigning this issue to me as it is related to DERBY-4090 and I'm continuing its resolution.
        Hide
        Tiago R. Espinha added a comment -

        Mamta said:
        > 3)The converted junit test has fixture testBasicBtreeScanForZeroRowsUpdateNonkeyfield. This test uses for update of clause here but the some of the other tests didn't.

        Mamta, you say that some of the other tests did not use the 'for update of' clause, but looking at holdCursorJDBC30.out it seems to me that all the tests on the Btree Scan For Zero Rows Update Nonkey (refer to line 328 of holdCursorJDBC30.out) do use 'for update of data'.

        Am I missing something or were you talking about Junjie's patch not containing the said clause?

        Show
        Tiago R. Espinha added a comment - Mamta said: > 3)The converted junit test has fixture testBasicBtreeScanForZeroRowsUpdateNonkeyfield. This test uses for update of clause here but the some of the other tests didn't. Mamta, you say that some of the other tests did not use the 'for update of' clause, but looking at holdCursorJDBC30.out it seems to me that all the tests on the Btree Scan For Zero Rows Update Nonkey (refer to line 328 of holdCursorJDBC30.out) do use 'for update of data'. Am I missing something or were you talking about Junjie's patch not containing the said clause?
        Hide
        Mamta A. Satoor added a comment -

        Tiago, I think you can disregard that comment. I looked through the test and the patch and seems like the conversion is fine. From what I recall, I may have thought that all the tests in the junit need to use for update clause but I don't see the original test using for update clause for all the test cases either.

        Show
        Mamta A. Satoor added a comment - Tiago, I think you can disregard that comment. I looked through the test and the patch and seems like the conversion is fine. From what I recall, I may have thought that all the tests in the junit need to use for update clause but I don't see the original test using for update clause for all the test cases either.
        Hide
        Tiago R. Espinha added a comment -

        I went through Junjie's patch line-by-line and here's what I changed:

        • Added the 'for update' clause where it was missing, to mimic the original test.
        • Added the setAutoCommit(false) on the setUp() method.
        • Set the bulkFetch to 1 where it was missing, as per the original test.
        • Removed assertTrue(true) from the exceptions as I believe this is a waste of space and resources.
        • Checked the order of all the close() and commit() calls, so that it also mimics the original test (there were a few wrong ones as Mamta said).
        • Made the asserts so that we check the actual data in the tables rather than just the row number.

        Now, the problem is that when I run the test, I systematically get errors like "Table/View 'FOO' already exists in Schema 'APP'.".

        Ideally we'd create the tables on the decorator so that they'd be self-managed, but that isn't optimal as each test has its own requirement as far as tables go. Some require the tables to be empty, some require some records in it, and it is often the case that the tables are simply different.

        With this said, I kept Junjie's approach of creating and dropping the tables on each fixture. I must be doing something wrong though, as I keep hitting that error.

        Just to cover all possibilities, I added a commit() call after the dropTable is called but still no luck. I still get the same errors and I must be missing something.

        If anyone has any ideas, I'd appreciate it.

        Show
        Tiago R. Espinha added a comment - I went through Junjie's patch line-by-line and here's what I changed: Added the 'for update' clause where it was missing, to mimic the original test. Added the setAutoCommit(false) on the setUp() method. Set the bulkFetch to 1 where it was missing, as per the original test. Removed assertTrue(true) from the exceptions as I believe this is a waste of space and resources. Checked the order of all the close() and commit() calls, so that it also mimics the original test (there were a few wrong ones as Mamta said). Made the asserts so that we check the actual data in the tables rather than just the row number. Now, the problem is that when I run the test, I systematically get errors like "Table/View 'FOO' already exists in Schema 'APP'.". Ideally we'd create the tables on the decorator so that they'd be self-managed, but that isn't optimal as each test has its own requirement as far as tables go. Some require the tables to be empty, some require some records in it, and it is often the case that the tables are simply different. With this said, I kept Junjie's approach of creating and dropping the tables on each fixture. I must be doing something wrong though, as I keep hitting that error. Just to cover all possibilities, I added a commit() call after the dropTable is called but still no luck. I still get the same errors and I must be missing something. If anyone has any ideas, I'd appreciate it.
        Hide
        Tiago R. Espinha added a comment -

        This file is a repro test class that I'm uploading to sort out a behaviour that I'm witnessing. This is either a bug or then I am most certainly missing something.

        This test should mimic the following behaviour:
        ---------------------8<-----------------------
        call SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY('derby.language.bulkFetchDefault', '1');

        create table foo4 (a int, data int);

        insert into foo4 values (1,10);
        insert into foo4 values (1,20);
        insert into foo4 values (1,30);
        insert into foo4 values (1,40);
        insert into foo4 values (1,50);

        get with hold cursor test1 as 'select * from foo4 for update of data';

        next test1;
        commit;

        delete from foo4 where current of test1;
        – should fail

        next test1;
        commit;

        update foo4 set data=-3000 where current of test1;
        – should fail
        ---------------------8<-----------------------
        Both the update and the delete should fail because of the commit, and they do if I follow those exact steps on ij. However, the JDBC test doesn't throw an exception on the update and it reaches the fail() call.

        The error that ij rightfully throws is ERROR 24000: Invalid cursor state - no current row.

        Any ideas?

        Show
        Tiago R. Espinha added a comment - This file is a repro test class that I'm uploading to sort out a behaviour that I'm witnessing. This is either a bug or then I am most certainly missing something. This test should mimic the following behaviour: --------------------- 8< ----------------------- call SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY('derby.language.bulkFetchDefault', '1'); create table foo4 (a int, data int); insert into foo4 values (1,10); insert into foo4 values (1,20); insert into foo4 values (1,30); insert into foo4 values (1,40); insert into foo4 values (1,50); get with hold cursor test1 as 'select * from foo4 for update of data'; next test1; commit; delete from foo4 where current of test1; – should fail next test1; commit; update foo4 set data=-3000 where current of test1; – should fail --------------------- 8< ----------------------- Both the update and the delete should fail because of the commit, and they do if I follow those exact steps on ij. However, the JDBC test doesn't throw an exception on the update and it reaches the fail() call. The error that ij rightfully throws is ERROR 24000: Invalid cursor state - no current row. Any ideas?
        Hide
        Tiago R. Espinha added a comment -

        After talking this over on IRC, Dag suggested something that fixed this issue.

        This test is using the following parameters on the createStatement:
        ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDATABLE and ResultSet.HOLD_CURSORS_OVER_COMMIT

        After going through the ResultSet code, I found out that the checkForUpdatableResultSet() method (called upon the deleteRow() ) overrides any of the checks that it would do, unless the ResultSet is set as CONCUR_READ_ONLY. So, basically put, switching this parameter will make the repro pass.

        I am still working on the actual test and at this point I decided that it would be a better option to create individual tables for each test.
        (As suggested by Kathey here: http://tinyurl.com/dxozhb )

        Finally, I am also opening a bug for the described behavior above.

        Show
        Tiago R. Espinha added a comment - After talking this over on IRC, Dag suggested something that fixed this issue. This test is using the following parameters on the createStatement: ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDATABLE and ResultSet.HOLD_CURSORS_OVER_COMMIT After going through the ResultSet code, I found out that the checkForUpdatableResultSet() method (called upon the deleteRow() ) overrides any of the checks that it would do, unless the ResultSet is set as CONCUR_READ_ONLY. So, basically put, switching this parameter will make the repro pass. I am still working on the actual test and at this point I decided that it would be a better option to create individual tables for each test. (As suggested by Kathey here: http://tinyurl.com/dxozhb ) Finally, I am also opening a bug for the described behavior above.
        Hide
        Tiago R. Espinha added a comment -

        I am uploading a new version of my reproduction test. This time I think I might have found a different bug.

        The test as it is, is failing but it shouldn't. The problem seems to be that the updateInt() isn't persisting the changes into the database. The fail I get is:
        1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)junit.framework.AssertionFailedError: expected:<-3000> but was:<10>

        suggesting that the update isn't indeed being successful. No exceptions are thrown and no update is made, so I believe that either way there is something wrong going on here.

        Show
        Tiago R. Espinha added a comment - I am uploading a new version of my reproduction test. This time I think I might have found a different bug. The test as it is, is failing but it shouldn't. The problem seems to be that the updateInt() isn't persisting the changes into the database. The fail I get is: 1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)junit.framework.AssertionFailedError: expected:<-3000> but was:<10> suggesting that the update isn't indeed being successful. No exceptions are thrown and no update is made, so I believe that either way there is something wrong going on here.
        Hide
        Knut Anders Hatlen added a comment -

        Shouldn't there have been a call to ResultSet.updateRow()? I don't think the database will be updated if you only call updateInt() + commit().

        Show
        Knut Anders Hatlen added a comment - Shouldn't there have been a call to ResultSet.updateRow()? I don't think the database will be updated if you only call updateInt() + commit().
        Hide
        Tiago R. Espinha added a comment -

        Oh you're right Knut, my bad. I am not much of a JDBC user so I end up missing on these little details...

        Anyway, I did add the updateRow() call after the updateInt() and now I'm getting an even weirder error. It's even weirder that it is happening on such a small fixture...

        I'm getting this on one of the drivers:
        1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)org.apache.derby.shared.common.sanity.AssertFailure: ASSERT FAILED statementContext is not expected to equal statementContexts[0]

        (this one also blows the JVM away)

        and this on the other:
        2)testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)java.sql.SQLException: DERBY SQL error: SQLCODE: -1, SQLSTATE: XJ001, SQLERRMC: java.lang.NullPointerExceptionXJ001.U

        Any ideas? This should be so simple...

        Show
        Tiago R. Espinha added a comment - Oh you're right Knut, my bad. I am not much of a JDBC user so I end up missing on these little details... Anyway, I did add the updateRow() call after the updateInt() and now I'm getting an even weirder error. It's even weirder that it is happening on such a small fixture... I'm getting this on one of the drivers: 1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)org.apache.derby.shared.common.sanity.AssertFailure: ASSERT FAILED statementContext is not expected to equal statementContexts [0] (this one also blows the JVM away) and this on the other: 2)testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)java.sql.SQLException: DERBY SQL error: SQLCODE: -1, SQLSTATE: XJ001, SQLERRMC: java.lang.NullPointerExceptionXJ001.U Any ideas? This should be so simple...
        Hide
        Knut Anders Hatlen added a comment -

        The assert failure must be a bug, so it would be good to file a separate JIRA issue for it.

        I was able to get the test to run without failures by replacing "for update of data" with "for update" (or by removing the entire FOR UPDATE clause). No idea why that should be necessary, though.

        Show
        Knut Anders Hatlen added a comment - The assert failure must be a bug, so it would be good to file a separate JIRA issue for it. I was able to get the test to run without failures by replacing "for update of data" with "for update" (or by removing the entire FOR UPDATE clause). No idea why that should be necessary, though.
        Hide
        Tiago R. Espinha added a comment -

        This issue is currently blocked by DERBY-4198.

        I was reading through the DERBY-3801 and Myrna says:
        "I think you believe a ScriptTest is a 'tool' to convert the test to junit, and it may have started as a quick way to run certain .sql ways in junit, but it is currently the best way to execute the language constructs that are specific to ij.
        Yes, we have in the charter to use only standard SQL, but there are some extensions that are specific to Derby and they have to be tested also.

        Language clauses as "get cursor with hold", "next", "update where current of".
        So, it's important to keep a test that verifies appropriate behavior when such things are used in ij. Thus, we do need a HoldCursorIJTest, but it should be extending ScriptTestCase."

        Considering that this test does rely a lot on the GET CURSOR WITH HOLD and it has its share of UPDATE WHERE CURRENT OF, should this test be extending ScriptTest as well rather than a BaseJDBCTestCase?

        Show
        Tiago R. Espinha added a comment - This issue is currently blocked by DERBY-4198 . I was reading through the DERBY-3801 and Myrna says: "I think you believe a ScriptTest is a 'tool' to convert the test to junit, and it may have started as a quick way to run certain .sql ways in junit, but it is currently the best way to execute the language constructs that are specific to ij. Yes, we have in the charter to use only standard SQL, but there are some extensions that are specific to Derby and they have to be tested also. Language clauses as "get cursor with hold", "next", "update where current of". So, it's important to keep a test that verifies appropriate behavior when such things are used in ij. Thus, we do need a HoldCursorIJTest, but it should be extending ScriptTestCase." Considering that this test does rely a lot on the GET CURSOR WITH HOLD and it has its share of UPDATE WHERE CURRENT OF, should this test be extending ScriptTest as well rather than a BaseJDBCTestCase?
        Hide
        Myrna van Lunteren added a comment -

        As long as the constructs are tested in one place, it should suffice. Lang is probably more suitable for that then store, I don't think we'd need to test ij specifics in any test in the tests/store directory.

        Show
        Myrna van Lunteren added a comment - As long as the constructs are tested in one place, it should suffice. Lang is probably more suitable for that then store, I don't think we'd need to test ij specifics in any test in the tests/store directory.
        Hide
        Tiago R. Espinha added a comment -

        So can I simply switch this over to a ScriptTest under Lang?

        If I get a green light, I'll get on it first thing tomorrow, since this issue will no longer be blocked.

        Show
        Tiago R. Espinha added a comment - So can I simply switch this over to a ScriptTest under Lang? If I get a green light, I'll get on it first thing tomorrow, since this issue will no longer be blocked.
        Hide
        Kathey Marsden added a comment -

        I think having a JDBC test for this is good as it has already brought to light a serious bug. I think you should continue with the java test and work around the bug for now and put a comment referring that it should be changed once DERBY-4198 is fixed.

        Then make a separate small ScriptTest that just tests the ij specific syntax without all the rigorous testing of hold behavior, but just basic testing to make the ij syntax works.

        Show
        Kathey Marsden added a comment - I think having a JDBC test for this is good as it has already brought to light a serious bug. I think you should continue with the java test and work around the bug for now and put a comment referring that it should be changed once DERBY-4198 is fixed. Then make a separate small ScriptTest that just tests the ij specific syntax without all the rigorous testing of hold behavior, but just basic testing to make the ij syntax works.
        Hide
        Tiago R. Espinha added a comment -

        I have run into what seems to be another bug.

        Initially I thought this could simply be a normal different behaviour between embedded and client drivers. Kathey has however suggested that it could be a bug so I'm posting it here to get some opinions.

        The issue is on the embedded driver. The problem happens when we create a cursor and while having it on a certain record, another statement is executed that deletes both the record that the cursor is on and the next one. If afterwards we advance the cursor, the embedded driver will act as if the rows haven't been deleted at all, whereas the client driver seems to behave correctly.

        It needs to be said though that the client driver will only pass as long as the bulkFetchDefault is set to 1. Any other values on this property seem to make the client error out as well.

        Show
        Tiago R. Espinha added a comment - I have run into what seems to be another bug. Initially I thought this could simply be a normal different behaviour between embedded and client drivers. Kathey has however suggested that it could be a bug so I'm posting it here to get some opinions. The issue is on the embedded driver. The problem happens when we create a cursor and while having it on a certain record, another statement is executed that deletes both the record that the cursor is on and the next one. If afterwards we advance the cursor, the embedded driver will act as if the rows haven't been deleted at all, whereas the client driver seems to behave correctly. It needs to be said though that the client driver will only pass as long as the bulkFetchDefault is set to 1. Any other values on this property seem to make the client error out as well.
        Hide
        Tiago R. Espinha added a comment - - edited

        This is a working patch of this test. I haven't yet ran suites.All nor derbyall but will do so if this patch is considered for committal.

        Keep in mind that this test has two workarounds:

        • It drops the 'FOR UPDATE' clause of some of the fixtures as they were causing crashes (on both embedded and client/server)
        • It has two separate behaviours (for embedded and client/server) on the tests testPositionPurgedRow and testPositionPurgedPage.

        Also, my last comment was wrong. It is the client driver's results that aren't faithful to those of the original test. As can be seen on the repro class I have attached, the underlying changes to a table aren't replicated to an already open cursor of that table. Is this by design? Considering it only happens on the client driver?

        Show
        Tiago R. Espinha added a comment - - edited This is a working patch of this test. I haven't yet ran suites.All nor derbyall but will do so if this patch is considered for committal. Keep in mind that this test has two workarounds: It drops the 'FOR UPDATE' clause of some of the fixtures as they were causing crashes (on both embedded and client/server) It has two separate behaviours (for embedded and client/server) on the tests testPositionPurgedRow and testPositionPurgedPage. Also, my last comment was wrong. It is the client driver's results that aren't faithful to those of the original test. As can be seen on the repro class I have attached, the underlying changes to a table aren't replicated to an already open cursor of that table. Is this by design? Considering it only happens on the client driver?
        Hide
        Dag H. Wanvik added a comment -

        It seems in the client/Server case, a prefetch of rows is performed independently of setting derby.language.bulkFetchDefault, so the change is not seen. I think it acceptable
        behavior that the client driver does some prefetching of its own. I tried calling st.setFetchSize(1) but it made no difference. That quantity is only considered a hint, though. If the app really cares about changes, it should make a new query. Btw, Derby does not yet implement scrollable sensitive result sets.

        Show
        Dag H. Wanvik added a comment - It seems in the client/Server case, a prefetch of rows is performed independently of setting derby.language.bulkFetchDefault, so the change is not seen. I think it acceptable behavior that the client driver does some prefetching of its own. I tried calling st.setFetchSize(1) but it made no difference. That quantity is only considered a hint, though. If the app really cares about changes, it should make a new query. Btw, Derby does not yet implement scrollable sensitive result sets.
        Hide
        Dag H. Wanvik added a comment -

        Btw, making the result set updatable, makes the repro work correctly since this forces prefetching to be disabled.

        Show
        Dag H. Wanvik added a comment - Btw, making the result set updatable, makes the repro work correctly since this forces prefetching to be disabled.
        Hide
        Kathey Marsden added a comment -

        Thanks Tiago for the patch. The test looks good, but we need to add the new test to store._Suite and remove the old test from derbynetmats.runall, storemore.runall and j9derbynetmats.runall.

        It might be cleanest to change the result set to be updatable as Dag suggests to get consistent client/embedded behavior and add a comment as to why this was done.

        Show
        Kathey Marsden added a comment - Thanks Tiago for the patch. The test looks good, but we need to add the new test to store._Suite and remove the old test from derbynetmats.runall, storemore.runall and j9derbynetmats.runall. It might be cleanest to change the result set to be updatable as Dag suggests to get consistent client/embedded behavior and add a comment as to why this was done.
        Hide
        Tiago R. Espinha added a comment -

        I tried Dag's suggestion of making the result set updatable but it didn't help. Both the repro and the actual test fail, even with the result set set as updatable.

        Show
        Tiago R. Espinha added a comment - I tried Dag's suggestion of making the result set updatable but it didn't help. Both the repro and the actual test fail, even with the result set set as updatable.
        Hide
        Tiago R. Espinha added a comment -

        I'm attaching another patch. This one removes the test from the harness, adds it to the Suite and effectively removes the .sql and .out files from the system.

        It seems that Junjie's patch wasn't removing those files but just emptying them instead. I'm not sure we want to keep the empty files around so I just deleted them. If this isn't the desired behavior please let me know.

        Show
        Tiago R. Espinha added a comment - I'm attaching another patch. This one removes the test from the harness, adds it to the Suite and effectively removes the .sql and .out files from the system. It seems that Junjie's patch wasn't removing those files but just emptying them instead. I'm not sure we want to keep the empty files around so I just deleted them. If this isn't the desired behavior please let me know.
        Hide
        Kathey Marsden added a comment -

        Committed revision 772354.

        Show
        Kathey Marsden added a comment - Committed revision 772354.
        Hide
        Tiago R. Espinha added a comment -

        Thank you Kathey for committing the patch. The regressions ran fine, so I'm closing the issue.

        Show
        Tiago R. Espinha added a comment - Thank you Kathey for committing the patch. The regressions ran fine, so I'm closing the issue.

          People

          • Assignee:
            Tiago R. Espinha
            Reporter:
            Junjie Peng
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development