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

Only temporarily move item if using sequential updates #375

Closed

Conversation

tjwallace
Copy link

@tjwallace tjwallace commented Apr 13, 2020

Otherwise we can assume there is no unique index or the constraint is deferred, meaning the temporary placement has no use. This decreases the number of UPDATE statements required.

Otherwise we can assume there is no unique index or the constraint is
deferred.
temporary_position = bottom_position_in_list + 2
set_list_position(temporary_position, raise_exception_if_save_fails)

if sequential_updates?
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if I should check sequential_updates, or add a new option to bypass this logic. Let me know what you think @brendon.

@brendon
Copy link
Owner

brendon commented Apr 13, 2020

Hi @tjwallace, you raise a good point. I like the optimisation.

Perhaps just as a courtesy, contact the person who introduced this feature and see if they're using sequential_updates? That'll be a good test as to whether people rely on this particular feature without enabling sequential_updates.

It'll still be a breaking change I think.

@tjwallace
Copy link
Author

@zharikovpro would mind taking a look at this PR? You originally introduced sequential_updates in #246 to avoid problems with unique constraints.

@tjwallace tjwallace marked this pull request as ready for review April 14, 2020 18:47
@zharikovpro
Copy link
Contributor

@brendon @tjwallace thanks for asking guys!

Perhaps just as a courtesy, contact the person who introduced this feature and see if they're using sequential_updates?

I'm not using it right now. Still it was merged to the gem and looks like it'll be good to keep this logic to avoid breaking changes? Other devs may use it as well.

Otherwise we can assume there is no unique index or the constraint is deferred, meaning the temporary placement has no use.

I believe this is the correct use of sequential_updates?. If it's false, when we could avoid additional updates, that's for sure 👌

This decreases the number of UPDATE statements required.

With that kind of optimization IMO it would be great to add the test to check the number of updates.

@brendon
Copy link
Owner

brendon commented Apr 17, 2020

Thanks @zharikovpro. Indeed @tjwallace, we should probably have a test for this.

I think it's an ok change, but we'll have to bump a minor version to signal the change.

There is potentially some work around refactoring all of this logic and making the shuffling logic work with constraints at the database level: #372

@brendon
Copy link
Owner

brendon commented Jun 4, 2024

@tjwallace, I must apologise for this languishing. Is it still a concern for you? I'm still happy to pursue this but just require a test to ensure the case is covered. I'll close it for now since it's so old but we can reopen it at any point.

Check out my new gem that works around this issue in another way: https://github.com/brendon/positioning though we always assume a uniqueness constraint. We invert the position number to move them out of the way and then invert them again with the increment to make a gap. This requires we allow for 0 and negative positions.

@brendon brendon closed this Jun 4, 2024
@tjwallace
Copy link
Author

Hi @brendon thanks for following up. I've switched jobs and don't use this gem anymore, so it is no longer a concern.

@brendon
Copy link
Owner

brendon commented Jun 4, 2024

Thanks @tjwallace :D All the best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants