Fix bug in SP* transactions

[janardhan1993]

This change is 

[janardhan1993]

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]

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]

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