Skip to content
This repository has been archived by the owner on Sep 27, 2019. It is now read-only.

Read-Only TxnContext Interface; Read-Only (single-statement select, txn) optimizations #1402

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

lmwnshn
Copy link
Contributor

@lmwnshn lmwnshn commented Jun 11, 2018

#1396

  1. Removed default read_only=false from TransactionManager::BeginTransaction
  2. Removed TransactionContext::SetReadOnly
  3. Added read-only param to TransactionContext constructors
  4. Transactions with no modifying queries treated as read-only
    4.1 assumption: if transaction is marked read-only, then READ transactions are not added to ReadWriteSet

#1395
Single-statement selects are marked as read-only

Unable to exercise this code path, possibly due to #1441

@lmwnshn
Copy link
Contributor Author

lmwnshn commented Jun 11, 2018

Currently, I am only failing copy_test (1/181).

If I add traffic_cop.SetStatement(statement) to copy_test here, I will pass. The question is, should I do that?

I'm looking into what else is using SetStatement right now.

apavlo
apavlo previously approved these changes Jun 12, 2018
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// if no modifying queries, treat as read-only
if (rw_set.empty()) {
EndTransaction(current_txn);
Copy link
Member

@apavlo apavlo Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just add a LOG_TRACE statement here to record what is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+0.04%) to 77.125% when pulling ec4c796 on lmwnshn:txnctx/readonly into a045cfc on cmu-db:master.

auto &rw_set = current_txn->GetReadWriteSet();

// if no modifying queries, treat as read-only
if (rw_set.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the implementation of the is_written_ flag in #1401 could we also check the is_written_ flag here? That would tell us if the RWSet is just reads.

Copy link
Contributor Author

@lmwnshn lmwnshn Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbutrovich I've tried changing it to use is_written_ instead of empty(), but I keep breaking on multi_granularity_access_test. Did you and/or @pervazea say something about SCAN being broken earlier?

In particular, I added an IsWritten() getter and the check became if (!current_txn->IsWritten()). Haven't been able to find the bug, for now, I'm leaving it as empty().

@mbutrovich
Copy link
Contributor

mbutrovich commented Jun 18, 2018

The more I look at this, the more it seems like we're building a hack on top of an unstable foundation -- at least part 4 of @lmwnshn's description. We're trying to find a shortcut if the RWSet only contains Reads/is empty. However, we shouldn't even be putting Reads in the RWSet in the first place. @yingjunwu says as much. The only reason we even add READ_OWNs is for releasing the write lock at Commit/Abort.

Commit and Abort do nothing if an element is READ when they iterate through the RWSet, so they shouldn't really be there in the first place.

This PR looks fine on its own, but it seems like a workaround for what a larger design issue that should be addressed.

@yingjunwu
Copy link
Contributor

I am not sure what you guys are currently working on, but I wanna mention that the original rwset was design to be general enough -- We didn't assume the underlying CC protocol, and the rwset was supposed to be compatible with any protocol, like OCC and 2PL. While our experiments show that timestamp ordering performs best, it seems that some people (e.g., Phil Bernstein) have a strong faith in 2PL :-)

@mbutrovich
Copy link
Contributor

So that’s sort of emblematic of the issues I feel like I’m finding in the current system. Things like maitaining a doubly-linked version chain, reads getting added to the RWSet, etc. seem to be preventing us from making the best MVTO system possible.

I definitely see why it was necessary in the context of comparing CC systems, but if MVTO is what we’re sticking with I think we can improve it.

@apavlo
Copy link
Member

apavlo commented Jun 21, 2018

@mbutrovich Do we want to merge this PR?

@mbutrovich
Copy link
Contributor

@apavlo Yes. @lmwnshn might need to do a rebase first, but I still think it should be merged.

@lmwnshn lmwnshn force-pushed the txnctx/readonly branch 5 times, most recently from 2a965b2 to e24ddaf Compare June 26, 2018 17:30
@lmwnshn
Copy link
Contributor Author

lmwnshn commented Jun 26, 2018

@mbutrovich Undid the formatting changes

@mbutrovich mbutrovich assigned lmwnshn and unassigned mbutrovich Jun 26, 2018
Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss the object stuff. It might be as simple as moving the check later in CommitTransaction, or maybe relying on a different flag that nothing was dropped.

LOG_TRACE("Transaction not yet written, ending transaction.");
EndTransaction(current_txn);
return ResultType::SUCCESS;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the transaction just drops an object, like an index? IsWritten will still return false since that doesn't touch the RWSet yet we'll directly commit the transaction without adding the objects to the gc_object_set. I'm not sure we support drop index yet, but I foresee problems with this.

@mbutrovich
Copy link
Contributor

mbutrovich commented Jun 28, 2018

I can't get your change for #1395 to work. I put a breakpoint here and then ran the following in psql:

default_database=# CREATE TABLE foo(a int, b int);
default_database=# SELECT * FROM foo;
 a | b 
---+---
(0 rows)

default_database=# insert into foo values(1,2),(3,4);
default_database=# SELECT * FROM foo;
 a | b 
---+---
 1 | 2
 3 | 4
(2 rows)

The breakpoint doesn't trigger with the second SELECT. Am I misunderstanding the query I should be using?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants