Skip to content
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

[DNM][WIP] client/leasemanager: create a package to build transaction… #45156

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ajwerner
Copy link
Contributor

… scoped leases

This is a very WIP implementation of transaction scoped locks.
The interface is garbage and not very considered.

Nevertheless it seems to mostly work. The biggest problem I see so far is that
at high concurrency there are cases where a lock can lay down all of the
relevant intents and then still fail to commit. This happens because the TS
cache information is lost due to a lease transfer before the txn record is layed
down. Such a possibility is a big problem for the lease use cases which assume
that an intent will be valid until the transaction expires. To some extent this
problem makes sense given the transaction expiration isn't well defined before
the txn record has been written.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)


pkg/kv/txn_coord_sender.go, line 882 at r1 (raw file):

	defer tc.mu.Unlock()
	if tc.mu.txn.WriteTimestamp.Less(ts) {
		tc.mu.txn.WriteTimestamp = ts

does this ensure commit at >= WriteTimestamp even if the txn does no more writes?

Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/kv/txn_coord_sender.go, line 882 at r1 (raw file):

Previously, sumeerbhola wrote…

does this ensure commit at >= WriteTimestamp even if the txn does no more writes?

Yes. The timestamp for the EndTxnRequest comes from the WriteTimestamp

Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/kv/txn_interceptor_heartbeater.go, line 238 at r2 (raw file):

func (h *txnHeartbeater) forceHeartbeat() error {
	h.forceHeartbeatCh <- struct{}{}

The synchronization here isn't clear. It seems like you can't call this more than once. Make that clear from the API?

I sort of also wish this call and its parents took a context and were cancelable.

I think we can come up with fancier dances with channels to make this reentrant and deal with context cancelation.

ajwerner and others added 4 commits February 19, 2020 17:09
… scoped leases

This is a very WIP implementation of transaction scoped locks.
The interface is garbage and not very considered.

Nevertheless it seems to mostly work. The biggest problem I see so far is that
at high concurrency there are cases where a lock can lay down all of the
relevant intents and then still fail to commit. This happens because the TS
cache information is lost due to a lease transfer before the txn record is layed
down. Such a possibility is a big problem for the lease use cases which assume
that an intent will be valid until the transaction expires. To some extent this
problem makes sense given the transaction expiration isn't well defined before
the txn record has been written.

Release note: None
- Txn.ForceHeartbeat() writes a transaction record.
- Txn.ExpiryTimestamp() returns the current expiry timestamp.
- TODOs for plumbing a special Intent that updates the ts cache when removed due to abort.

Release note: None
More plumbing for special intent that updates ts cache.
Remaining work is to somehow update the ts cache in cmd_resolve_intent.go
and cmd_resolve_intent_range.go,

Release note: None
Plumbing to update the ts cache when removing a "leasing intent".

Release note: None
Added proof sketch for lease/lock implementation.
Added description of alternative that does not have contention
between concurrent acquisitions of shared lease.

Release note: None
@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants