fix #1313 Remove read only isolation level

[Nov11]

this is to #1313

Details: 1.remove IsolationLevelType::READ_ONLY 2.add a flag in transactioncontext 3.re-implement 'IsReadOnly' function. This is safe because the function has not been call anywhere before this modification. 4.substitute every usage of 'READ_ONLY' with IsReadOnly function call 5.add a new parameter to BeginTransaction and provide a short cut 'BeginTransactionReadOnly' 6.one invocation of BeginTransaction(READ_ONLY) is changed to 'BeginTransactionReadOnly(IsolationLevelType::READ_COMMITTED);'

[coveralls]

Coverage decreased (-0.04%) to 77.563% when pulling 83d08f5ac2893efb23f6fb69175a060a21ae7c83 on Nov11:remove-read-only-isolation-level into 6cdca1572c5a9ee3a81850cc142b481809ced895 on cmu-db:master.

[poojanilangekar]

@Nov11 Your change is breaking the serializable_transaction_test. Please fix it and let me know so I can review the change.

[Nov11]

It won't break tests now. jenkins failed because of network error : 'stderr: fatal: unable to access 'https://github.com/cmu-db/peloton.git/': Failed to connect to github.com port 443: No route to host '

[crd477]

On 2018-04-21 13:08, Lin Ma wrote:

Hi @crd477 https://github.com/crd477 @pervazea https://github.com/pervazea, it looks like some of our PRs including this one cannot download the repo with the Jenkins build again. For example: http://jenkins.db.cs.cmu.edu:8080/blue/organizations/jenkins/peloton/detail/PR-1322/1/pipeline/ http://jenkins.db.cs.cmu.edu:8080/blue/organizations/jenkins/peloton/detail/PR-1321/1/pipeline/ http://jenkins.db.cs.cmu.edu:8080/blue/organizations/jenkins/peloton/detail/PR-1133/86/pipeline/ http://jenkins.db.cs.cmu.edu:8080/blue/organizations/jenkins/peloton/detail/PR-1310/7/pipeline/

I'm wondering is this because we have another machine that has networking problem? Would really appreciate your help.

Ugh...

Yes, this was the default route problem again that originally cropped up in #1295. I thought I'd fixed it the other day but obviously it happened again. I've implemented a more permanent fix now and it should not happen again.

Sorry (again) for the inconvenience...

[Nov11]

As there's already a read only transaction test prior to this change(SerializableTransactionTests->ReadOnlyTransactionTest), I thought it's adequate and didn't add new ones. I broke that test case once. poojanilangekar reminded me of it. And it's been fixed. If that test is not suffice to peloton, I'd like to do the addition.

[apavlo]

@Nov11 What does the test do? We need to test the following scenario:

Txn #1 | Txn #2
----------------
BEGIN  |
W(X)   |
       | BEGIN R/O
       | R(X)
W(X)   |
COMMIT | 
       | R(X)
       | COMMIT

[Nov11]

1. About the existing test case. I didn't look into it closely. Assuming it verifies correctness. I find that test might be wrong however.

Here is the logic: The test case creates a table ,inserts 10 tuples and commits. This is the first txn. Then a read only sequential scan is executed. This is the second txn.

The comment says it should return 0 tuples but I think this is wrong. It should return 10 tuples instead. The scan starts after the first txn committed. It should be able to read those tuples. I think I should correct this test case.

Below is the first commit that added this test case. It was in 2016. No logic has changed since then.

// test predeclared read-only transaction
TEST_F(SerializableTransactionTests, ReadOnlyTransactionTest) {
  for (auto protocol_type : PROTOCOL_TYPES) {
    concurrency::TransactionManagerFactory::Configure(protocol_type, ISOLATION_LEVEL_TYPE);
    auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
    // Just scan the table
    {
      concurrency::EpochManagerFactory::GetInstance().Reset();
      storage::DataTable *table = TestingTransactionUtil::CreateTable();

      TransactionScheduler scheduler(1, table, &txn_manager, true);
      scheduler.Txn(0).Scan(0);
      scheduler.Txn(0).Commit();

      scheduler.Run();

      // Snapshot read cannot read the recent insert
      EXPECT_EQ(0, scheduler.schedules[0].results.size());
    }
  }
}

2. About the expected result This is my plan for a new test case.
   • ignore isolation level of txn#1
   • suppose x = 0 in the beginning
   • txns run in this order:

     Txn #1 | Txn #2
     ----------------
     BEGIN  |
     W(X,1) |
     | BEGIN R/O
     | R(X) 1st
     W(X,2) |
     COMMIT | 
     | R(X) 2nd
     | COMMIT

     the result should be :

isolation level of txn#2	1st read	2nd read	test case already added
read committed	0	2	N
repeatable read	0	0	N
serializable	0	0	Y

I did a project global search and find that 'BeginTransaction' runs with IsolationLevelType::SERIALIZABLE as default parameter. Test cases peloton has now are all running in this isolation level. It seems that read committed and repeatable read are not that important. Do you want me to add test cases of these two? This can be done by add a new member variable in TransactionSchedule and pass it to BeginTransaction. It may change certain portion of test codes.

The second read returns 2 when test case runs, which I think should be 0. I'm confused. Am I making wrong expectation? Or my code is wrong?

[apavlo]

@Nov11 Your comment about the broken test case looks to be correct. We want to test the scenario when there are two active txns running simultaneously. The code in ReadOnlyTransactionTest looks to have only one txn at a time.

Don't worry about any isolation level other than snapshot isolation.

[Nov11]

There's a concurrent test case now.

[poojanilangekar]

@apavlo @pervazea Can you please review this PR? @mengranwo has added support for change column name but we currently unable to test it because we need concurrent transactions to check the correctness.

[apavlo]

I will look at this today.

[apavlo]

I am going to merge this when the build passes. There are some stylistic things that I want to change but I am just going to fix that myself.

@Nov11 Thanks for your help!

[Nov11]

I'm glad to help, :-)