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

Transaction improvements #203

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

repomaa
Copy link

@repomaa repomaa commented Aug 27, 2018

  • actually test transaction operations (was using Repo instead of tx inside transaction! block
  • streamline CUD methods
  • add all read methods to LiveTransaction

I noticed the using Repo.all inside of a transaction! block won't return any of the records created inside of the transaction which is why i included all read methods for LiveTransaction

@fridgerator
Copy link
Member

Nice!

@fridgerator
Copy link
Member

On this line: https://github.com/jreinert/crecto/blob/transaction_improvements/src/crecto/adapters/mysql_adapter.cr#L58

Does this work?

if conn.is_a?(DB::TopLevelTransaction)
  return execute(conn, "SELECT * FROM #{changeset.instance.class.table_name} WHERE (#{changeset.instance.class.primary_key_field} = #{last_insert_id})")
end

@repomaa
Copy link
Author

repomaa commented Aug 30, 2018

well, last_insert_id is undefined up there but basically it would be the same as leaving the guard clause out. :/ In this case the method just freezes the program.

@repomaa repomaa mentioned this pull request Aug 30, 2018
@repomaa
Copy link
Author

repomaa commented Aug 30, 2018

To recap: This PR is on hold, because the specs for the query methods introduced to LiveTransaction rely on having the the inserted record (with it's id set) returned. This is currently not working for MySQL. A guard clause in the insert method of the mysql_adapter prevents the fetching of the inserted record. When it's removed the method will block indefinetely.

@repomaa repomaa added the on hold Dont merge this label Sep 1, 2018
@repomaa repomaa force-pushed the transaction_improvements branch from 51b9dc2 to c82b3c0 Compare September 1, 2018 16:50
@repomaa repomaa force-pushed the transaction_improvements branch from c82b3c0 to b1512e8 Compare October 24, 2019 10:17
@repomaa repomaa force-pushed the transaction_improvements branch 2 times, most recently from 29eaaaf to ad5a6ff Compare November 6, 2019 14:14
@repomaa
Copy link
Author

repomaa commented Nov 6, 2019

I now use compiler flags to determine which db the specs are run against and depending on those enable/disable some specs. Nested transactions aren't supported by sqlite so i exclude the spec for it when run against sqlite. The tx read spec (reading records created within a transaction from within the same transaction) should work for all databases but there are underlying issues in the mysql and sqlite drivers that prevent this. This is why i mark the tx read spec as pending for mysql and sqlite.

@repomaa
Copy link
Author

repomaa commented Nov 6, 2019

What's a bit of a mystery to me is the currently failing spec for delete_all in a transaction for sqlite. I can't reproduce this on my local machine (arch linux with sqlite 3.30.1). Any help in figuring out what goes wrong there would be much appreciated!

@repomaa repomaa force-pushed the transaction_improvements branch from 7f47b9c to 3508411 Compare March 6, 2020 10:46
@repomaa repomaa force-pushed the transaction_improvements branch from 3508411 to 7fd4945 Compare September 28, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Dont merge this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants