Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

*: implement LOCK and UNLOCK of tables #448

Merged
merged 3 commits into from
Oct 18, 2018

Conversation

erizocosmico
Copy link
Contributor

Closes #410

@erizocosmico erizocosmico requested a review from a team October 11, 2018 15:02
@@ -61,6 +61,10 @@ func (h *Handler) ConnectionClosed(c *mysql.Conn) {
delete(h.c, c.ConnectionID)
h.mu.Unlock()

if err := h.e.Catalog.UnlockTables(nil, c.ConnectionID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the tables would keep locked forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it fails, you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you can retry UNLOCK TABLES. If it fails it's because the tables Unlock failed, so nothing we can do there, as the implementation of that method depends on the tables.

@ajnavarro
Copy link
Contributor

@erizocosmico can you rebase please?

c.locks[id][db] = make(tableLocks)
}

c.locks[id][db][table] = struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we plan (in the future) lock all DB or Session (not only tables, rows), but if yes then maybe flat structure, e.g. c.locks["id.db.table"] may work better, because you don't need to create nested structs. Moreover when you unlock you have to propagate this info to the higher levels, instead of delete one key.
But frankly it's up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, we use the elements of each separate step. c.locks[id] to get all the locks for a session and then c.locks[id][db] to get the tables. We can't do that with a single key


func (l *lockableTable) Lock(ctx *sql.Context, write bool) error {
if write {
l.writeLocks++
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we count locks?
Isn't it 1/0 (SET/UNSET) for the same session?

Copy link
Contributor

Choose a reason for hiding this comment

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

...or is it just counter for number of read/writes for unlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is just a counter for the test

Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

shall we add this statement to readonly.go rule?

@@ -788,6 +789,34 @@ var fixtures = map[string]sql.Node{
},
plan.NewUnresolvedTable("bar", "foo"),
),
`UNLOCK TABLES`: plan.NewUnlockTables(),
Copy link
Contributor

Choose a reason for hiding this comment

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

could you test table names with numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@erizocosmico
Copy link
Contributor Author

Rebased @ajnavarro

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
@erizocosmico
Copy link
Contributor Author

@ajnavarro added to readonly

@ajnavarro ajnavarro merged commit 6a6452a into src-d:master Oct 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants