Implement lock feature

[shiba-hiro]

The PR for implementing the lock feature.

This change enables LiveTransaction to request SELECT ~ FOR UPDATE on MySQL and Postgres.

Now it returns the list of queryable by Crecto::Repo::Query like :all, but/so that the class which does not have the primary key can also uses this lock feature.

e.g.

# CREATE TABLE `samples` (`id` VARCHAR(36), `number` INT UNSIGNED, `updated_at` TIMESTAMP(3) DEFAULT CURRENT_TIMESTAMP(3), `created_at` TIMESTAMP(3) DEFAULT CURRENT_TIMESTAMP(3), PRIMARY KEY USING BTREE (`id`));

class Sample < Crecto::Model
  schema "samples" do
    field :id, String, primary_key: true
    field :number, Int32
  end
end

sample = Sample.new
sample.id = "7eb498c2-e63c-4be9-814f-b67d32a7ce38"
sample.number = 0
Repo.insert sample

Repo.transaction! do |tx|
  puts "start transaction"
  arr = tx.lock(Sample, Crecto::Repo::Query.where(id: "7eb498c2-e63c-4be9-814f-b67d32a7ce38"))
  sample = arr[0]
  sample.number = sample.number.as(Int32) + 1
  tx.update sample

  # you can test rollback and lock release with timeout, Exception...
  # sleep(2)
  # raise Exception.new
end

[shiba-hiro]

shiba-hiro#7

It's strange that one test in SQLite is failed even if it was succeeded in my push build.

It's same as below PR;
#198

[repomaa]

Yeah SQLite is being a pain right now... Has nothing to do with your change.

[repomaa]

I wonder if this should be an optional param for all existing query methods. something like Repo.get(User, id, locked: true)

[shiba-hiro]

@jreinert thank you for your comments!

I wonder if this should be an optional param for all existing query methods. something like Repo.get(User, id, locked: true)

I see.

The reason I used the word lock is because I saw this feature in Jennifer first.
https://imdrasil.github.io/jennifer.cr/docs/transaction_and_lock
I felt(just my feeling) it's easy for the Library users to understand what happens.

And about using optional param, because we need to pass the transaction object from the api caller, it should be like

Repo.get(User, id, transaction: tx, locked: true)

or

# Implicitly assume that it does SELECT FOR UPDATE if transaction object is passed
Repo.get(User, id, transaction: tx)

Now latter is OK but it can cause some problem if a transaction pool feature became available(I don't know it's planned or not though)(or achieve transaction pool by api callers).
On the other hand, former requires 2 param for one feature and it needs to write some validation logic("locked must be used with tx").

What do you think?

[repomaa]

Good points there. I have PR #203 on hold because there seems to be a problem using LAST_INSERT_ID() in transactions for MySQL but the PR would introduce all query methods from Repo to LiveTransaction. We could then use locked: true by default in all those methods. I could really use some help debugging that btw :)

[shiba-hiro]

@jreinert OK, I saw the PR and understood the situation.

Using the word lock or not contains a matter of taste for me.
I can change the way to use optional param locked: Bool? in existing Repo's methods and methods you are adding 👍

[repomaa]

I think the major benefit in having the optional param over using a single method called lock is that it will not only work for all but also for get and get_by (and their exclamation mark equivalents), which i think are more likely to be used in conjunction with locked: true than all. In your spec this also becomes apparent: You immediately assign the result[0] to your local variable.

[fridgerator]

I agree with @jreinert and creating named parameters for all Repo methods. Its not immediately obvious that .lock is only useful for all type queries.