#891 confirmed
kristian

[patch] is list

Reported by kristian | June 7th, 2009 @ 05:27 AM

after I had quite some trouble with dm-is-list on production (table with > 10000 entries which had more then 100 double positions - no scope) I needed to change the implementation that I can set some unique constraints on position/scope which at least helps to keep my database consistence.

the patch allows these constraints and along I fixed the 'move :up/:down' to respect the scope. the behaviour of switching the scope without supplying a new position is now the same as inserting a new entry in that new scope, i.e. it will be put at the end of the list.

the tests got the unique constraints.

the problem with my big table was something with such update statements:

UPDATE "items" SET "position" = "position" + 1 WHERE "position" BETWEEN 4 AND 7

where it looks like that these are split up into a sequence of several statements which could interfere with each other. I never managed to reproduce the error in a test case, but it was reproducable on production and the new implementation works.

regards Kristian

Comments and changes to this ticket

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 7th, 2009 @ 03:07 PM

    • Milestone set to 0.10.0

    @kristian: Can you update this patch against the dm-more/next branch?

    Also, one reason for the problem you are seeing is that when you have a unique constraint on the position column, and you don't supply an ORDER BY statement to MySQL it can explode if the positions are changed in the wrong order internally by MySQL.

  • kristian

    kristian June 7th, 2009 @ 10:15 PM

    I tried the ORDER BY clause with sqlite3 and it did not work, see also the end of http://www.sqlite.org/lang_update.html

    so the ORDER BY solution will work with mysql and probably other DB but not with sqlite3.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 8th, 2009 @ 12:20 AM

    Perhaps the only case that'll work across all adapters is no to use Collection#update! (and mass-update like now) but to use Collection#update and perform the updates in the correct order (which can be ensured by making sure the Collection is ordered properly)

  • kristian

    kristian June 8th, 2009 @ 04:58 AM

    I hope this patch is now against the next branch.

    this patch works with three sql statements: first move the range into the negativ positions (no collisions possible) and then move them back into their new positions (again without collisions) and finally adjust the position of the raw which you wanted to move.

    so I see three possible implementations:
    * use 2 mass updates and one single row update (see attached patch) * use Collection.update which results into an update for each row (if understood it right) * make an options where you can choice either one of the above implementations

    I am not sure which one is better or faster (to keep your transaction short).

    I OK to change the patch for any of above implementations, just let me know what you think.

  • kristian

    kristian June 9th, 2009 @ 03:03 AM

    I tried the row by row approach yesterday and it performs really bad. factor ~500 slower with a list of 5000 entries on both sqlie3 and mysql. so I think the attached patch works for all adapters, though it could be optimized for non sqlite3 (but this needs a patch for dm-adjust as well to pass on 'order' parameters).

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 9th, 2009 @ 02:37 PM

    I recently talked to kematzy on IRC and iirc he said that he believes that most/all bugs with dm-is-list are fixed in his fork of dm-more available at

    http://github.com/kematzy/dm-more/tree/next_dm-is-list-fix

    I must admit I haven't tested it yet, but I'm all for a solid implementation of dm-is-list! Furthermore, commenting on that ticket now will bring it back to LH's ticket "frontpage" :-)

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 9th, 2009 @ 05:32 PM

    #739 also provides a patch for dm-is-list. I haven't actually looked at any of these patches, but it seems like dm-is-list should get fixed properly before 0.10 comes out.

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 9th, 2009 @ 06:48 PM

    #818 is another ticket with a failing spec for dm-is-list

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 9th, 2009 @ 07:37 PM

    • Assigned user changed from “Sindre Aarsaether” to “Martin Gamsjaeger (snusnu)”
    • State changed from “unconfirmed” to “accepted”

    #735 #737 #738 and #740 are all related to bugs in dm-is-list. I think those were the last ones. I'm collecting them here (and added a link to this ticket here to all those other ones) because they could serve as a checklist for kematzy's work (and possible extensions to that).

  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) July 9th, 2009 @ 07:54 PM

    I accepted this and all the other related tickets because i want to work through them with kematzy and be finally able to mark them all as resolved.

  • kristian

    kristian July 9th, 2009 @ 10:48 PM

    in the past I had a lot of trouble with this plugin, since with a list of ~4000 entries I found more than 100 double entries and fixing them manually is quite an unpleasant job. so when I wrote this patch I was sure that I want to have a unique constraint on the scope + position to make sure if things go wrong at least there are no double entries.

    if dm-adjust would obey the options :order => [:position] or so then you could implement it differently from what I did. sqlite3 is one exception due to the way it implements "order by" on updates statements.

  • kristian

    kristian July 9th, 2009 @ 11:00 PM

    sorry, another remark: once I ran a script to fix things and someone worked online on the same data and things got messed up because the changes is-list is making are done outside a transaction. so concurrent use of the is-list methods can currently mess up things as well. either make it clear in the documentation, or better make it on options to use transaction or just use transaction.

    I hope I do not put too much on the plate for you and I do appreciate your efforts and I hope I can use is-list again in production soon.

  • kematzy

    kematzy July 13th, 2009 @ 03:15 AM

    I believe the patch in this branch [ http://github.com/kematzy/dm-more/commits/clean-dm-is-list-fix ] fixes all the outstanding issues in dm-is-list, as well as:

    adds new functionality,

    transactions,

    extensive specs (with 98.8% coverage),

    better documentation

    and, most importantly, reduces the number of SQL queries required to order a list.

    There are however a few issues that may need to be addressed (they are clearly noted in the specs and declared as pending):

    1. [list_spec.rb:603] test #move_to_list method fails with a [ no such table: todos ] error when moving an item from one list to a FIXED position in another. This is due to nested transactions.

    2. [list_spec.rb:1023] test #move(:higher/lower) fails with a [ columns position, client_id are not unique ] error when declaring :unique_index => :position in the Model. This is due to issues with the order in which the DB updates the rows, and is therefore more difficult to fix.

    I think it should be clean enough to merge into the main next branch. IF not, please let me know and I'll fix it. I will also try to keep the patch up to date with the next branch.

    Kristian, please try this patch with your stuff, to see if there's anything my specs have missed on.
    Make sure you read the README first though, as the [ item.position = X ] behavior has been changed.

  • kematzy

    kematzy July 13th, 2009 @ 03:17 AM

    ooops!! That was NOT the formatting I wanted. I just tried to create a simple list, and forgot it's Markdown syntax here.

  • kristian

    kristian July 13th, 2009 @ 05:03 AM

    @kematzy I have two problems: I never was able to write up a spec which fails or see problems in development, but I fixed my two applications in production now a couple of times and secondly I am like a burnt child who avoids fire: I need the unique constraints without it I will not use this plugin in production.

    one thing to fix this is to have an "order by" in the dm-adjust plugin then you can shift around things in way where the unique index stays intact. maybe you need one more sql statement, but I have very clear preference in that matter: consistency before performance !

    to help you, I can offer to add the "order by" statement in the dm=adjust plugin. on the other hand I have an implementation which works smoothly so far (but does a bad job for ajax position lists). and sorry I can not prove my claims with specs.

    the nested transaction can be avoided if I understand your code right by having a _move methods without transaction boundaries which is used my "move_to_list" and "move".

    then I just saw that with "def move_to_list(scope, pos = nil)" the scope is an integer where as a scope is an array of fields - from an api point view of the is a bit inconsistent.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 4th, 2009 @ 09:33 PM

    • Milestone changed from 0.10.0 to 0.10.2

    [project:id#20609 not-tagged:"0.10.0" milestone:id#51895 bulk edit command]

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) February 1st, 2010 @ 04:33 PM

    • Milestone changed from 0.10.2 to 1.0.0
  • Martin Gamsjaeger (snusnu)

    Martin Gamsjaeger (snusnu) March 30th, 2010 @ 05:38 PM

    • State changed from “accepted” to “confirmed”
    • Assigned user cleared.

    Anyone up for solving this once and for all?

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) May 26th, 2010 @ 01:54 AM

    • Milestone cleared.

    It looks like the bulk of these issues are now resolved. I think to make any more progress on issues with dm-is-list we should summarize what the remaining issues are. Reading through this thread it's unclear to me what the actual problems are now, perhaps someone could comment with a point form list of outstanding things we could finally resolve and close this ticket.

    I also wanted to add a few comments:

    1. I removed the nested transaction from inside dm-is-list. DO has no support for nested transactions, so when caller code opened a transaction, the one inside dm-is-list would cause issues. At this time we're not recommending that any DM gem handle transactions themselves, and that caller code setup transactions where necessary (so basically anywhere you care about data integrity). Once DO supports nested transactions we can revisit adding this back.

    2. It seems to me that we would want the position column to be unique, or at least unique within a given scope. This presents a few problems. An UPDATE statement is free to modify matching records in any order it likes, and may even violate a uniqueness constraint in the process. Some databases provide a way to provide the ORDER BY statement, which specifies the order the matching records are updated in. I think this can be used to prevent some of the problems, but that still leaves SQLite3 which has no such capability.

    To address this problem I was wondering if we should have two strategies for updating records. One would perform the bulk updates and always use whatever ORDER BY is specified in the query. The other would perform each update individually, in the correct order, so that at no time would a conflict occur. In this latter case it would be more queries than the bulk update, but it would be correct, which IMHO is the most important thing to focus on first.

    For now I am going to remove this from the 1.0.0 milestone until the outstanding issues have been summarized by someone who understands what all those issues are.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Referenced by

Pages