consistent lookup vindex #4861

sougou · 2019-05-09T06:41:07Z

This implements #4855.

Design decisions

Beyond the high level approach described in the RFC. There are a few additional design decisions I had to make to get this feature working:

planbuilder and engine should be mostly unaffected: Since the planbuilder and engine are going to be growing in complexity, I didn't want them to take the burden of coordinating commit order and row checks. This required me to implement the entire logic within the consistent lookup vindex itself. In order to achieve this, I had to extend the VCursor API to allow the Vindex to perform things that were previously not possible. Specifically: Issuing Pre and Post transaction statements. And the ability to target a shard by its keyspace id (ExecuteKeyspaceID).
A Vindex can optionally implement the WantOwnerInfo interface. If it does the vschema builder will give it the info needed for it to verify if the main row is present.
Parallel commits: With the new lookup, there is actually no need to follow a commit order, which we previously did, in order to prefer committing the lookup rows first. However, some people still rely on this order, at least while they're using the older lookups. So, commits have been parallelized for pre and post transactions, but the main transaction retains its old behavior of committing in DML order. Once we get everyone migrated to the consistent lookup, we can change this behavior to be all parallel.
Removed the deprecated legacy mode support for transactions. No one uses this any more, and it was complicating the number of states to consider for this feature. Also simplified other flows within session to allow it to accommodate the commit order feature.
Fixed some bugs in partial DML detection. Previously, any statement within a transaction was marked as modifying data. This made us forcibly rollback transactions as partially executed even if it only performed reads. This was not interacting well with this feature. So, I fixed the behavior to fail only if actual partial DMLs were executed.

Implementation

Based on the above decisions, there are changes in the following areas:

Session: Clean up and extend to support commit ordering. Change the transaction commit function to honor this order.
VSchema: Define the WantOwnerInfo interface and have the builder set the owner info if a vindex implements it.
VCursor: Implement Pre, Post and ExecuteKeyspaceID functions. Also accurately detect partial DML execution.
Consistent Lookup Vindexes: Implementation of the actual vindex.
New endtoend test for VTGate using VTCombo: This will also act as framework for eventually porting the vtgatev3 python tests into this new framework.

SafeSession and underlying session always non-nil. Append makes session not autocommitable. Remove legacy mode. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Added two new shard sessions for commit ordering: pre and post. Added API to set the commit order and changed tx conn to honor it. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Add the ability to request the owner column names, which will be used to check for presence of owner row by the vindex. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Some people may still rely on the sequential commit order of the normal transactions. So, it's better not to parallelize that part. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

demmer · 2019-05-09T20:31:05Z

go/vt/vtgate/safe_session.go

 	newSession.Autocommit = true
+	newSession.Warnings = nil


Why are the warnings cleared here?

This is used as a private session within the context of an outer session, specifically for vindex autocommit. The warnings of the outer one are untouched. After this execution finishes, we could consider appending any new warnings generated back to the outer session. But that may or may not be right. We should look at some real use cases before making this decision.

Maybe we should rename this to NewInnerSession?

demmer

Overall this looks sound to me.

A couple high level comments:

It would have been good to have separated out the legacyAutocommit deprecation and scattered bugfixes into a separate PR to ease the review and improve clarity.

Overall I am a bit concerned about over-promising magic by using the name "consistent_lookup". At the end of the day there's a tradeoff between potentially leaving dangling rows in the lookup table (like this one does) or potentially missing rows (like the other one does), but there's no perfect way to ensure consistency when we have best effort distributed transactions as the underlying primitive (ignoring 2PC of course).

So... maybe we should name this variant something like covering_lookup or super_lookup or something like that? Something to be explicit about how it might fail.

demmer · 2019-05-09T22:08:32Z

go/vt/vtgate/scatter_conn.go

@@ -740,7 +740,9 @@ func (stc *ScatterConn) multiGo(
 	oneShard := func(rs *srvtopo.ResolvedShard, i int) {
 		var err error
 		startTime, statsKey := stc.startAction(name, rs.Target)
-		defer stc.endAction(startTime, allErrors, statsKey, &err, nil)
+		// Send a dummy session.
+		// TODO(sougou): plumb a real session through this call.


What are the implications here? I'm a bit confused about where the multiGo parts actually come into play?

I previously made a dumb decision that allowed a SafeSession to be nil, which required me to check for it on every method. It turns out that this is the only use case where the nil value is used. So, I changed it to send an empty session which means the same thing.

The comment is a reminder to look up the stack to see if the real session could to be plumbed through. However, I suspect that we'll just remove this code when we get rid of V2.

demmer · 2019-05-09T22:12:20Z

go/vt/vtgate/tx_conn.go

+		return err
+	}
+
+	// Retain backward compatibility on commit order for the normal session.


It seems to me we should be able to mark the session as to whether or not we can commit in parallel -- if it's only the legacy vindexes that care about the ordering, then maybe we should flag the session when adding a transaction id whether or not the commit order matters?

Yeah. I want to do this as a separate feature.

demmer · 2019-05-09T22:36:04Z

go/vt/vtgate/vcursor_impl.go

+	return vc.executeByOrder(method, query, bindVars, isDML, commitOrderPost)
+}
+
+func (vc *vcursorImpl) executeByOrder(method string, query string, bindVars map[string]*querypb.BindVariable, isDML bool, co commitOrder) (*sqltypes.Result, error) {


Instead of widening the VCursor interface into Execute, ExecutePre, and ExecutePost, it seems cleaner to make this the signature for a combined VindexExecute function.

This solves a couple problems IMO -- one is that callers already need to know Execute vs ExecutePre vs ExecutePost, and so exposing them all in one function instead of three seems reasonable.

The second is that I have always found it confusing that the VCursor interface defines a recursive Execute method that is called via the Executor only through the vindex functions. Calling it VindexExecute or ExecuteForVindex or some such would be clearer.

Moreover we could consider folding ExecuteAutocommit in here as well.

I went this route because we otherwise introduce a circular dependency on commitOrder: vtgate calls engine, which calls into vindexes, and the vindex calls back into VCursor (as interface).

We could move a commitOrder to vindexes and export it. Do you think that will still remain readable?

Or, I just thought of this: define it in the proto. I think proto looks like the best place.

demmer · 2019-05-09T22:47:15Z

go/vt/vtgate/vindexes/lookup_internal.go

@@ -91,9 +93,9 @@ func (lkp *lookupInternal) Verify(vcursor VCursor, ids, values []sqltypes.Value)
 		var err error
 		var result *sqltypes.Result
 		if lkp.Autocommit {
-			result, err = vcursor.ExecuteAutocommit("VindexVerify", lkp.ver, bindVars, true /* isDML */)
+			result, err = vcursor.ExecuteAutocommit("VindexVerify", lkp.ver, bindVars, false /* isDML */)


This looks like it was just a bug found during the course of doing this cleanup?

Yeah. But it became significant for the feature: these failures would mark the transaction as partially complete and abort the transaction.

go/vt/vtgate/vindexes/vindex.go

demmer · 2019-05-09T22:57:04Z

go/vt/vtgate/vcursor_impl.go

+
+func (vc *vcursorImpl) executeByOrder(method string, query string, bindVars map[string]*querypb.BindVariable, isDML bool, co commitOrder) (*sqltypes.Result, error) {
+	vc.safeSession.SetCommitOrder(co)
+	defer vc.safeSession.SetCommitOrder(commitOrderNormal)


Although I'm not too worried about this, the way this is implemented seems like a potentially dangerous pattern since we need to match these two. It's not that different from Lock/Unlock, but an alternative might be to use a facade:

The underlying implementation would still have the three arrays of ShardSession of course, but instead of a state variable indicating which one should be used, the facade would override and put txns in the right list based on the order.

Then the Executor would take a Session interface instead of directly taking a safeSession object, and we'd call into it with something like

qr, err := vc.executor.Execute(vc.ctx, method, NewSessionForCommitOrder(vc.safeSession, co), vc.marginComments.Leading+query+vc.marginComments.Trailing, bindVars)

On balance, it's probably not worth it but figured I'd just share.

This approach would have totally made sense if ShardSessions was a struct. Being a slice requires wrapping it, which makes the whole thing awkward.

deepthi · 2019-05-10T02:14:45Z

go/vt/vtgate/safe_session.go

+	case commitOrderPre:
+		session.PreSessions = append(session.PreSessions, shardSession)
+	case commitOrderPost:
+		session.PostSessions = append(session.PostSessions, shardSession)


why isn't this:
session.PostSessions = append(shardSession, session.PostSessions)

The three session variables are completely independent of each other. You could potentially have two transactions going to the same shard, one from each pool. If so, the ones in the Pre session are committed first, and the ones from Post are committed last.

deepthi · 2019-05-10T02:18:09Z

go/vt/vtgate/safe_session.go

 	}

-	if session.autocommitState == autocommittable && len(session.ShardSessions) == 0 {
+	if session.autocommitState == autocommittable {


autocommittable is sometimes spelt with one t like in the func SetAutoCommitable. Do we want to change all instances to be consistent?

I'll rename the function.

sougou

I'll soon push fixes to address review comments.

As for naming, I would argue that it's consistent for the use case it's meant to fulfill. Our guidelines have always recommended that you treat it as a hidden index. If so, it fulfills all the criteria.

Ideally, calling it consistent is bad naming. We should eventually deprecate the old lookups and call this one lookup, because the consistency should be taken for granted.

sougou · 2019-05-10T02:18:02Z

go/vt/vtgate/scatter_conn.go

@@ -740,7 +740,9 @@ func (stc *ScatterConn) multiGo(
 	oneShard := func(rs *srvtopo.ResolvedShard, i int) {
 		var err error
 		startTime, statsKey := stc.startAction(name, rs.Target)
-		defer stc.endAction(startTime, allErrors, statsKey, &err, nil)
+		// Send a dummy session.
+		// TODO(sougou): plumb a real session through this call.


I previously made a dumb decision that allowed a SafeSession to be nil, which required me to check for it on every method. It turns out that this is the only use case where the nil value is used. So, I changed it to send an empty session which means the same thing.

The comment is a reminder to look up the stack to see if the real session could to be plumbed through. However, I suspect that we'll just remove this code when we get rid of V2.

sougou · 2019-05-10T02:22:41Z

go/vt/vtgate/tx_conn.go

+		return err
+	}
+
+	// Retain backward compatibility on commit order for the normal session.


Yeah. I want to do this as a separate feature.

sougou · 2019-05-10T02:27:56Z

go/vt/vtgate/vcursor_impl.go

+	return vc.executeByOrder(method, query, bindVars, isDML, commitOrderPost)
+}
+
+func (vc *vcursorImpl) executeByOrder(method string, query string, bindVars map[string]*querypb.BindVariable, isDML bool, co commitOrder) (*sqltypes.Result, error) {


I went this route because we otherwise introduce a circular dependency on commitOrder: vtgate calls engine, which calls into vindexes, and the vindex calls back into VCursor (as interface).

We could move a commitOrder to vindexes and export it. Do you think that will still remain readable?

Or, I just thought of this: define it in the proto. I think proto looks like the best place.

sougou · 2019-05-10T02:32:30Z

go/vt/vtgate/vcursor_impl.go

+
+func (vc *vcursorImpl) executeByOrder(method string, query string, bindVars map[string]*querypb.BindVariable, isDML bool, co commitOrder) (*sqltypes.Result, error) {
+	vc.safeSession.SetCommitOrder(co)
+	defer vc.safeSession.SetCommitOrder(commitOrderNormal)


This approach would have totally made sense if ShardSessions was a struct. Being a slice requires wrapping it, which makes the whole thing awkward.

sougou · 2019-05-10T02:34:41Z

go/vt/vtgate/vindexes/lookup_internal.go

@@ -91,9 +93,9 @@ func (lkp *lookupInternal) Verify(vcursor VCursor, ids, values []sqltypes.Value)
 		var err error
 		var result *sqltypes.Result
 		if lkp.Autocommit {
-			result, err = vcursor.ExecuteAutocommit("VindexVerify", lkp.ver, bindVars, true /* isDML */)
+			result, err = vcursor.ExecuteAutocommit("VindexVerify", lkp.ver, bindVars, false /* isDML */)


Yeah. But it became significant for the feature: these failures would mark the transaction as partially complete and abort the transaction.

The VCursor API is impressively simpler no. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou · 2019-05-10T04:00:43Z

The VCursor API is impressively simpler now.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou added 10 commits May 5, 2019 12:15

consistent lookup: cleanup Session

f8115aa

SafeSession and underlying session always non-nil. Append makes session not autocommitable. Remove legacy mode. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: commit order in Session

91ea12e

Added two new shard sessions for commit ordering: pre and post. Added API to set the commit order and changed tx conn to honor it. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: extend vindex API

2431ba1

Add the ability to request the owner column names, which will be used to check for presence of owner row by the vindex. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: Pre and Post in vcursor

6ea59b5

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: skeletal implementation

9199612

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: testing phase 1

db5cd42

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: vindexes unit test

1af4c40

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: endtoend tests

d2799b6

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: fix stray tests

f04d405

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: revert to normal commit order

d775954

Some people may still rely on the sequential commit order of the normal transactions. So, it's better not to parallelize that part. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou requested review from demmer, deepthi and rafael May 9, 2019 06:41

demmer reviewed May 9, 2019

View reviewed changes

demmer approved these changes May 9, 2019

View reviewed changes

deepthi reviewed May 10, 2019

View reviewed changes

sougou commented May 10, 2019

View reviewed changes

consistent lookup: address review comments

b4379fa

The VCursor API is impressively simpler no. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

consistent lookup: address more review comments

21b6ea0

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou merged commit 948c251 into vitessio:master May 10, 2019

sougou deleted the ss-consistent-lookup branch May 11, 2019 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consistent lookup vindex #4861

consistent lookup vindex #4861

sougou commented May 9, 2019 •

edited by deepthi

Loading

demmer May 9, 2019

sougou May 9, 2019

demmer left a comment

demmer May 9, 2019

sougou May 10, 2019

demmer May 9, 2019

sougou May 10, 2019

demmer May 9, 2019

demmer May 9, 2019

sougou May 10, 2019

demmer May 9, 2019

sougou May 10, 2019

demmer May 9, 2019

sougou May 10, 2019

deepthi May 10, 2019

sougou May 10, 2019

deepthi May 10, 2019

sougou May 10, 2019

sougou left a comment

sougou May 10, 2019

sougou May 10, 2019

sougou May 10, 2019

sougou May 10, 2019

sougou May 10, 2019

sougou commented May 10, 2019

consistent lookup vindex #4861

consistent lookup vindex #4861

Conversation

sougou commented May 9, 2019 • edited by deepthi Loading

Design decisions

Implementation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou commented May 10, 2019

sougou commented May 9, 2019 •

edited by deepthi

Loading