New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in SP* transactions #2220

Merged
merged 2 commits into from Mar 20, 2018

Conversation

Projects
None yet
2 participants
@janardhan1993
Contributor

janardhan1993 commented Mar 13, 2018

This change is Reviewable

@janardhan1993

This comment has been minimized.

Show comment
Hide comment
@janardhan1993

janardhan1993 Mar 13, 2018

Contributor

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

package main

Tested manually with this program, implementation of jepsen delete test. Will refactor a bit and add to integration test later. Don't see this file.


posting/list.go, line 438 at r1 (raw file):

	}
	// Check if this commit is for sp*, markdeleteAll stores the startTs for sp*
	if l.markdeleteAll == startTs {

Say there are two parallel transactions one sp* and other which adds uid both did mutate, then if we were doing sp* irrespective of which transaction was committed.


posting/list.go, line 540 at r1 (raw file):

		// Fixing the pl is difficult with locks.
		// Ignore if SP* was committed with timestamp >= readTs
		if ts := Oracle().CommitTs(l.markdeleteAll); ts < readTs {

Need to ignore sp* if it was committed after the read transaction had started.


Comments from Reviewable

Contributor

janardhan1993 commented Mar 13, 2018

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

package main

Tested manually with this program, implementation of jepsen delete test. Will refactor a bit and add to integration test later. Don't see this file.


posting/list.go, line 438 at r1 (raw file):

	}
	// Check if this commit is for sp*, markdeleteAll stores the startTs for sp*
	if l.markdeleteAll == startTs {

Say there are two parallel transactions one sp* and other which adds uid both did mutate, then if we were doing sp* irrespective of which transaction was committed.


posting/list.go, line 540 at r1 (raw file):

		// Fixing the pl is difficult with locks.
		// Ignore if SP* was committed with timestamp >= readTs
		if ts := Oracle().CommitTs(l.markdeleteAll); ts < readTs {

Need to ignore sp* if it was committed after the read transaction had started.


Comments from Reviewable

@pawanrawal

This comment has been minimized.

Show comment
Hide comment
@pawanrawal

pawanrawal Mar 13, 2018

Contributor

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Tested manually with this program, implementation of jepsen delete test. Will refactor a bit and add to integration test later. Don't see this file.

Add a TODO to make this as a test and not a main program.


posting/list.go, line 438 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Say there are two parallel transactions one sp* and other which adds uid both did mutate, then if we were doing sp* irrespective of which transaction was committed.

Yeah, I saw this earlier and thought it to be a bug but didn't get around to talking to you about it.


posting/list.go, line 535 at r1 (raw file):

	} else if l.markdeleteAll == readTs {
		// Check if there is uncommitted sp* at current readTs.
		deleteTs = readTs

What is strange is that deleteTs is a readTs here and a commitTs below. It is used to compare against a commitTs in inSnapshot.


posting/list.go, line 538 at r1 (raw file):

	} else if l.markdeleteAll < readTs {
		// Ignore all reads before this.
		// Fixing the pl is difficult with locks.

Is this comment still valid? I don't understand what it means.


posting/list.go, line 539 at r1 (raw file):

		// Ignore all reads before this.
		// Fixing the pl is difficult with locks.
		// Ignore if SP* was committed with timestamp >= readTs

I don't think anything can get committed with timestamp = readTs.


Comments from Reviewable

Contributor

pawanrawal commented Mar 13, 2018

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Tested manually with this program, implementation of jepsen delete test. Will refactor a bit and add to integration test later. Don't see this file.

Add a TODO to make this as a test and not a main program.


posting/list.go, line 438 at r1 (raw file):

Previously, janardhan1993 (Janardhan Reddy) wrote…

Say there are two parallel transactions one sp* and other which adds uid both did mutate, then if we were doing sp* irrespective of which transaction was committed.

Yeah, I saw this earlier and thought it to be a bug but didn't get around to talking to you about it.


posting/list.go, line 535 at r1 (raw file):

	} else if l.markdeleteAll == readTs {
		// Check if there is uncommitted sp* at current readTs.
		deleteTs = readTs

What is strange is that deleteTs is a readTs here and a commitTs below. It is used to compare against a commitTs in inSnapshot.


posting/list.go, line 538 at r1 (raw file):

	} else if l.markdeleteAll < readTs {
		// Ignore all reads before this.
		// Fixing the pl is difficult with locks.

Is this comment still valid? I don't understand what it means.


posting/list.go, line 539 at r1 (raw file):

		// Ignore all reads before this.
		// Fixing the pl is difficult with locks.
		// Ignore if SP* was committed with timestamp >= readTs

I don't think anything can get committed with timestamp = readTs.


Comments from Reviewable

@janardhan1993 janardhan1993 changed the title from WIP: Fix bug in SP* transactions to Fix bug in SP* transactions Mar 20, 2018

@janardhan1993

This comment has been minimized.

Show comment
Hide comment
@janardhan1993

janardhan1993 Mar 20, 2018

Contributor

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.


posting/list.go, line 438 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yeah, I saw this earlier and thought it to be a bug but didn't get around to talking to you about it.

Done.


posting/list.go, line 535 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What is strange is that deleteTs is a readTs here and a commitTs below. It is used to compare against a commitTs in inSnapshot.

We should consider sp* if read is happening as part of same transaction which did sp*, in this case we would consider everything below that as deleted.
We should only consider sp* only if it was commited before readTs


posting/list.go, line 538 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this comment still valid? I don't understand what it means.

In other cases we updated the committs in posting using atomic variable, but we can't do that in case of sp* because sometimes we have read lock we can't fix the posting(delelte everything and make it empty)


posting/list.go, line 539 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't think anything can get committed with timestamp = readTs.

Yeah, will update comment


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a TODO to make this as a test and not a main program.

Converted to Test


Comments from Reviewable

Contributor

janardhan1993 commented Mar 20, 2018

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.


posting/list.go, line 438 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yeah, I saw this earlier and thought it to be a bug but didn't get around to talking to you about it.

Done.


posting/list.go, line 535 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What is strange is that deleteTs is a readTs here and a commitTs below. It is used to compare against a commitTs in inSnapshot.

We should consider sp* if read is happening as part of same transaction which did sp*, in this case we would consider everything below that as deleted.
We should only consider sp* only if it was commited before readTs


posting/list.go, line 538 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this comment still valid? I don't understand what it means.

In other cases we updated the committs in posting using atomic variable, but we can't do that in case of sp* because sometimes we have read lock we can't fix the posting(delelte everything and make it empty)


posting/list.go, line 539 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't think anything can get committed with timestamp = readTs.

Yeah, will update comment


contrib/integration/upsertDelete/main.go, line 1 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a TODO to make this as a test and not a main program.

Converted to Test


Comments from Reviewable

@janardhan1993 janardhan1993 merged commit 0c8f5fd into master Mar 20, 2018

3 of 4 checks passed

code-review/reviewable 2 files, 5 discussions left (pawanrawal)
Details
CI (dgraph) TeamCity build finished
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@janardhan1993 janardhan1993 deleted the fix/jepsen branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment