In C4::Members.pm there is a function AddMembers_Opac which calls fixup_cardnumber. The problem is there is no locking to ensure that someone else doesn't add a member at the same time and create a duplicate card number. This patch adds the missing locking in. I've left commented wait code in, so that the person testing this can switch tabs or browsers to test that the locking doesn't catastrophically break things.
A similar problem also exists on tools/import_borrowers.pl, which is scary, because they just put a #FIXME comment, despite knowing what to do. I'll make that a separate bug report, because it is more related to Tools than patrons.
gmcharlt pointed out that LOCK'ing is not really an option as it is not recommended for InnoDB under MySQL. And though transactions are handy in making things atomic, they do not create a "critical section" which is needed around fixup_cardnumber and AddMember calls.
Based on an idea from gmcharlt. Here's Pseudo-code of something I have in mind for fixup_cardnumber. I'm writing this so I don't forget it when I switch computers later to actually attempt implementing it. Already done elsewhere: CREATE TABLE cardnumber (UID autoincrement,PID string,Value string) my_cardnumber = select max(cardnumber) from borrowers check = select cardnumber from borrowers where cardnumber=my_cardnumber; while the check for the cardnumber exists { INSERT into cardnumber (PID) values (randomly generated PID string) my_uid = SELECT max(UID) from cardnumber where PID= the randomly generated PID string earliest uid = select min(uid) from cardnumber where value=''; while there is an earliest uid { previous_value = select value from cardnumber where uid=earliest uid-1; # the assumption is that f(previous value) will always return the # same new value regardless of how many times or when it is called. new value = f(previous value); update cardnumber set value=new value where uid=earliest uid; earliest uid = select min(uid) from cardnumber where value=''; } my_cardnumber = select Value from cardnumber where uid=my_uid check = select cardnumber from borrowers where cardnumber=my_cardnumber; } return my_cardnumber;
Others have suggested last_insert_id. Is last_insert_id a good option? The DBI page has scary things like: "Given all the caveats above, it's clear that this method must be used with care." And some of those are ugly caveats. I was thinking of generating an ID made of 4 parts which should be unique enough: 1) 16 character random string (A-Za-z0-9 character options). 2) process id padded to 6 digits 3) microsecond time padded to 20 digits 4) CGISESSID (or a 32 character random string if undefined). This, however, during testing created an every changing cardnumber when I just felt bored and clicked refresh on memberentry.pl So, I added a check for just the session id part to see if it is in borrowers. If not, then it would reuse it. Duplication should only happen on multi-tab stupidity, as far as I can tell. This got me thinking, do I need anything other that CGISESSID? Feedback appreciated.
(In reply to comment #4) > Others have suggested last_insert_id. Is last_insert_id a good option? The > DBI page has scary things like: "Given all the caveats above, it's clear > that this method must be used with care." And some of those are ugly caveats. See bug 10478 and bug 9921. > I was thinking of generating an ID made of 4 parts which should be unique > enough: > 1) 16 character random string (A-Za-z0-9 character options). > 2) process id padded to 6 digits > 3) microsecond time padded to 20 digits > 4) CGISESSID (or a 32 character random string if undefined). Use a random number generator from cpan?
Adding another dependency is undesirable. This is why my random string is based on String::Random, which is already a dependency. I use Time::HiRes for the microsecond time, which is already a dependency. I use CGI::Cookie to read the session number, which is already a dependency. And I use $$, which is already built into Perl. No dependencies added, and at 74 characters long, some of which are padding (1-5 for $$ and about 4 for the microsecond time), I'm pretty confident on it's random uniqueness. I have been playing with last_insert_id to try to get it to generate a bad result, and it would seem that DBI's last_insert_id is keyed to the database handle's connection, so it would be good enough to use. It would certainly simplify the code I'm working on. :) Any comments on what to put for the parameters to be as generic and forward compatible as possible? The DBI cpan pages gives the example: $rv = $dbh->last_insert_id($catalog, $schema, $table, $field); What should the $catalog and $schema be? I looked in information_schema for clues. I suspect 'def', 'kohadata' (or whatever you called your database). The schema then should be easily determined, but then I don't know of a good way to determine catalog.
Created attachment 19196 [details] [review] Bug 10454 - Duplicate card numbers may be generated Previously, there was a gap in time between the insertion of the card number into borrowers and the generation. This gap meant that the same card number could be calculated by two different processes resulting in two different members being added with the same card number. By serializing the cardnumber requests, at worst we may skip a cardnumber, rather than duplicate one. No locking required, because the INSERT into cardnumber_sequence is atomic! The reason no duplicates happen is because the sequence of cardnumbers is calculated from the previous one, and recalculating would generate exactly the same value. So even if two processes are calculating the same cardnumber for a given UID record, it will result in the same value. Skips can be generated. Clicking refresh while entering a patron, for example, will increment the auto generated number multiple times.
Comment on attachment 19196 [details] [review] Bug 10454 - Duplicate card numbers may be generated Review of attachment 19196 [details] [review]: ----------------------------------------------------------------- This is not a complete review of the patch, which will come later, just commenting on something that jumped out immediately. ::: kohaversion.pl @@ +16,4 @@ > use strict; > > sub kohaversion { > + our $VERSION = '3.13.00.XXX'; There is no need to kohaversion.pl -- the RM takes care of updating that any time a DBrev is pushed -- and leaving it in the patch just creates the potentially for merge conflict noise.
Test Plan ========= TEST updatedatabase.pl & kohaversion.pl ======================================= 1) Apply patch. 2) Back up koha database. -- Necessary to test empty borrowers case. 3) Upgrade the DB via staff client. 4) Confirm that there are 2 more records in cardnumber_sequence than borrowers. -- This means that the updatedatabase.pl part of the patch works correctly when there are borrowers. 5) Restore koha database. 6) delete all the records from the borrowers table. 7) Upgrade the DB via staff client. 8) Confirm there are only 2 records in cardnumber_sequence. -- This means that the updatadatabase.pl part of the patch works correctly where there are no borrowers. -- Given that you just triggered an upgrade successfully twice, the kohaversion.pl part of the patch works. NOTE: Currently it is using 3.13.00.XXX as the version number. It is my understanding that the release or maintenance manager is responsible for changing this number. TEST fixup_cardnumber CALLS =========================== This function is called by the member entry (home->patrons->new patron), import patrons (home->tools->import patrons), and opac self registration (home->administration->system preferences->opac opacuserlogin:allow, PatronSelfRegistration:Allow, PatronSelfRegistrationDefaultCategory: {something from the patron categories, like PT}, then click the register button on the login area of the OPAC client). Member Entry ============ 1) log into staff client 2) Home 3) Koha administration 4) Global system preferences 5) Patrons 6) autoMemberNum: Do, checkdigit: Don't (simple +1 sequence) 7) Save all Patrons preferences 8) Home 9) Patrons 10) New Patron (any category) 11) Note the cardnumber (you'll have to scroll down) 12) Refresh the page 13) Note the cardnumber (it should have incremented by one) -- Yes, the generation of a big gap in the card number sequence is a known flaw in this simplified version which has no unique process id. A gap is preferred over duplicates. 14) Home 15) Koha administration 16) Global system preferences 17) Patrons 18) autoMemberNum: Do, checkdigit: Do (adds checkdigit, starts with 'V') 19) Save all Patrons preferences 20) Home 21) Patrons 22) New Patron (any category) 23) Note the cardnumber (you'll have to scroll down) 24) Refresh the page 25) Note the cardnumber (V#######X, the #'s should have incremented by one) 26) Repeat steps 2 through 25. The first numbers (step 11 and 23) should be the next one of their respective types based on the last observed numbers (steps 13 and 25 from the previous iteration). OPAC Self Registration ====================== 1) log into staff client 2) Home 3) Koha administration 4) Global system preferences 5) OPAC 6) opacuserlogin:allow, PatronSelfRegistration:Allow, PatronSelfRegistrationDefaultCategory: {something from the patron categories, like PT} 7) Save all OPAC preferences 8) Patrons 9) autoMemberNum: Do, checkdigit: Don't (simple +1 sequence) 10) Save all Patrons preferences 12) open OPAC client 13) click the 'Register here' link. 14) Add a bogus patron 15) Check the cardnumber. It should be what is expected. 16) change the checkdigit value in Patrons to Do. 17) self register another patron in OPAC. 18) check the cardnumber. It should be what is expected for the other number generation method. Import Members ============== There is an export tool in misc called export_borrowers.pl I have not tested this, and have no idea if it would work, but using that with a -H option, to include headers. Delete the cardnumber column. And attempt to reimport it back into an empty borrowers tables.
From a glance through code, I feel that this routine fixup_cardnumber is still quite confusing and complicated. Should it really be done this way? With compliments for your hard work, but is it true that we spend so much effort on this only because we want to see a cardnumber already before hitting the Save button? Why not simplify it by disabling the cardnumber and probably also the login code fields for Insert Patron, generating them when saving the record and just displaying them after that? Note that e.g. we do not have a biblionumber too before saving a biblio record, etc. (Some other fields like 001, 005 are also finalized only when saving.)
(In reply to M. de Rooy from comment #10) Apologies for the reordering of your commentary. :) > With compliments for your hard work, but is it true that we spend so much > effort on this only because we want to see a cardnumber already before > hitting the Save button? No, the reason I did this is because of FIXME comments in places like C4::Input.pm and because the gap is larger in some places (like the above example) than others. Any chance of duplicates, no matter how small (in my opinion), is bad. > Why not simplify it by disabling the cardnumber and probably also the login > code fields for Insert Patron, generating them when saving the record and > just displaying them after that? I suppose we could disable or not display them if they are auto-generated, but if there is no auto-generation, we need to field visible and able to be entered. This would solve the skip generation problem, and make me much happier. But isn't whether to display or disable outside the scope of the initial problem? That is, limiting cardnumber generation to just before save is different than preventing duplicates, though it does overlap. > Note that e.g. we do not have a biblionumber too before saving a biblio > record, etc. (Some other fields like 001, 005 are also finalized only when > saving.) And I think that this idea of limiting in the case of auto-generation is a good one. It solves the fake skip problem (out of boredom hitting refresh). > From a glance through code, I feel that this routine fixup_cardnumber is > still quite confusing and complicated. Should it really be done this way? However, this patch (or something that at least serializes) is still necessary, because of the time gap between generation and insertion. Even if we shrink that gap, we don't eliminate the possibility of duplicates. This patch does. The reason this patch is complicated is to deal with the off-chance that someone attempts to generate a number, but the server crashes, before the calculated next value is inserted back into the sequence table. That would leave the previous value not calculated. This is why there is the loop, before returning the calculated value for the my_UID record. You will also note that this particular "number" (it's really a string for the 'katipo' value of checkdigit), can't be generalized as hoped for in bug 10478, because checkdigit is a cardnumber system preference. I really appreciate the feedback. I hope this reply makes sense.
(In reply to M. Tompsett from comment #11) > > From a glance through code, I feel that this routine fixup_cardnumber is > > still quite confusing and complicated. Should it really be done this way? > > However, this patch (or something that at least serializes) is still > necessary, because of the time gap between generation and insertion. Even if > we shrink that gap, we don't eliminate the possibility of duplicates. This > patch does. > > The reason this patch is complicated is to deal with the off-chance that > someone attempts to generate a number, but the server crashes, before the > calculated next value is inserted back into the sequence table. That would > leave the previous value not calculated. This is why there is the loop, > before returning the calculated value for the my_UID record. I too am suspicious of the complexity of this approach. I fully agree that improving concurrency is a worth goal, but I think the task of get a reliable sequentially-assigned integer can be *much* simpler. Here's a bit of code that provides an example. It runs with the MySQL's documented suggestion for emulating sequences [1]. If you create the table as per the script's prep() function, then run as many multiple copies of the script as you care to simultaneously, you won't get any repeats. If this is wrapped up in a module, and possibly tweaked a bit to allow more than one sequence to be defined, it could be nicely generalized. I don't feel that we should be concerned about false skips ... if a staff user gets a cardnumber value but doesn't use it... so what? [1] http://dev.mysql.com/doc/refman/5.5/en/information-functions.html#function_last-insert-id ___BEGIN___ #!/usr/bin/perl use Modern::Perl; use C4::Context; my $dbh = C4::Context->dbh(); ### only need to run prep() once ###prep(); $dbh->{AutoCommit} = 0; my $max = $ARGV[0] || 10; foreach my $i (0..$max) { my $val = fetch_next(); print "got $val\n"; } sub prep { $dbh->do('DROP TABLE IF EXISTS sequence'); $dbh->do('CREATE TABLE sequence (id INT NOT NULL) ENGINE myisam'); $dbh->do('INSERT INTO sequence VALUES (0)'); } sub fetch_next { $dbh->do('UPDATE sequence SET id=LAST_INSERT_ID(id+1)'); my $sth = $dbh->prepare('SELECT LAST_INSERT_ID()'); $sth->execute(); my @val = $sth->fetchrow_array(); return $val[0]; } ___END___
Here's a version that shows that one can store multiple sequences in a single table and add new ones at will: ___BEGIN____ #!/usr/bin/perl use Modern::Perl; use C4::Context; my $dbh = C4::Context->dbh(); ### only need to run prep() once ###prep(); $dbh->{AutoCommit} = 0; my $max = $ARGV[0] || 50; foreach my $i (0..$max) { my $val = fetch_next('id1'); print "got $val for sequence 1\n"; next unless 0 == ($i % 3); $val = fetch_next('id2'); print "got $val for sequence 2\n"; } sub prep { $dbh->do('DROP TABLE IF EXISTS sequence'); $dbh->do('CREATE TABLE sequence (id INT NOT NULL, seq varchar(10) NOT NULL) ENGINE myisam'); $dbh->do('INSERT INTO sequence VALUES (0,"id1")'); $dbh->do('INSERT INTO sequence VALUES (0,"id2")'); } sub fetch_next { my $seq = shift; $dbh->do('UPDATE sequence SET id=LAST_INSERT_ID(id+1) WHERE seq = ?', {}, $seq); my $sth = $dbh->prepare('SELECT LAST_INSERT_ID()'); $sth->execute(); my @val = $sth->fetchrow_array(); return $val[0]; } ___END___
As a point of information, I also tried using DBIx::Sequence. Pros of using DBIx::Sequence: - It works, in the sense that using a bunch of concurrent processes to fetch sequence values will get unique IDs. - It appears to be DBMS-agnostic Cons: - Under high simultaneous load, it is *significantly* slower to retrieve sequence values because of the way it implements spin-locking. - It doesn't expose a way to explicitly initialize a sequence to a given value. - The semantics of its Currval method are wrong -- ideally Currval() should stay stable within a session, but DBIx::Sequence->Currval() can change if another process fetches a value from the sequence. On balance, I prefer a roll-our-own because of the first two "cons".
(In reply to M. Tompsett from comment #11) > I suppose we could disable or not display them if they are auto-generated, > but if there is no auto-generation, we need to field visible and able to be > entered. This would solve the skip generation problem, and make me much > happier. +1 > But isn't whether to display or disable outside the scope of the initial > problem? That is, limiting cardnumber generation to just before save is > different than preventing duplicates, though it does overlap. I think we should be somewhat pragmatic about that. In some cases solving a problem is done better by extending the scope of the initial problem; otherwise we might just be treating symptoms..?
Created attachment 19264 [details] [review] Bug 10454 - Duplicate card numbers may be generated Previously, there was a gap in time between the insertion of the card number into borrowers and the generation. This gap meant that the same card number could be calculated by two different processes resulting in two different members being added with the same card number. By creating a Koha::Sequence class, the requests are serialized and prevent duplication. And this class allows for flexibility with other sequences that may wish to be maintained. The fixup_cardnumber now merely uses the Koha::Sequence to get the next numerical portion of the cardnumber. If checkdigit is 'katipo', the new cardnumber is calculated out of the numeric portion. Perfect serialization leads to another problem: gaps. Gaps which are generated by deletion of old records are to be expected, but gaps generated by continually clicking refresh are not. This is an outstanding problem to now solve on the patron entry screen. This is beyond the scope of this bug.
Created attachment 19265 [details] [review] Bug 10454 - Duplicate card numbers may be generated Previously, there was a gap in time between the insertion of the card number into borrowers and the generation. This gap meant that the same card number could be calculated by two different processes resulting in two different members being added with the same card number. By creating a Koha::Sequence class, the requests are serialized and prevent duplication. And this class allows for flexibility with other sequences that may wish to be maintained. The fixup_cardnumber now merely uses the Koha::Sequence to get the next numerical portion of the cardnumber. If checkdigit is 'katipo', the new cardnumber is calculated out of the numeric portion. Perfect serialization leads to another problem: gaps. Gaps which are generated by deletion of old records are to be expected, but gaps generated by continually clicking refresh are not. This is an outstanding problem to now solve on the patron entry screen. This is beyond the scope of this bug.
Though I would really like to fix up memberentry.pl and the javascript that is associated with cardnumber checking, I have decided against it. That way people can actually look at and potentially use Koha::Sequence.
Created attachment 20800 [details] [review] sign off of the bug
Failing QA for the MySQLisms in the perl module. Is it possible to rewrite this code without MySQLisms?
SUBSTR can be fixed. YAY! SUBSTRING is ANSI-1992. The UNSIGNED, from CAST( ... AS UNSIGNED), can be worked around using BIGINT and adding a >0 constraint on the value. Not sure I like that, but it will suffice. The only outstanding issue is LAST_INSERT_ID(). It can't really be ANSI SQL'd. :( However, as I mentioned a call to C4::Context->db_scheme2dbi in future code can provide optimized methods for other SQL back-ends. Given this last problem, I'm not sure there is a generic ANSI SQL way to do LAST_INSERT_ID. See Galen's comments in Comment 14. I think the use of a working C4::Context->db_scheme2dbi would be an acceptable work around, as we aim for a balance of speed and db-agnosticism. Granted, my position does not reflect the will of the masses. :) What other back-ends are people even trying right now other than MariaDB and Postgre? It would seem to me that the software aims to be ANSI SQL, but there are portions which are just plain better being SQL specific. Also note that http://wiki.koha-community.org/wiki/Coding_Guidelines does not mention avoiding mysqlisms, except for backticks.
Hi to all, I try to check the situation, and It appears that every SQL software uses a different solution for this problem. For example on Postegresql: http://stackoverflow.com/questions/2944297/postgresql-function-for-last-inserted-id So i think that mysqlism (and xxxism) are mandatory for this problem
I'm not sure there is a way to $dbh->last_insert_id() query a value, increment it, and update it in a way that is consistent in a multi-user scenario. $dbh doesn't let us "SQL;SQL;SQL" as far as I can tell. Unless we want to evil things like serialize a thread or process, which is just too evil and full of potential problems, I can't see a non-mysqlism way to handle the LAST_INSERT_ID code.
No activity for 2 years, lowering the severity.
I feel like this was mostly resolved by generating the card numbers on saving (not displaying them at the form while adding the user). Please reopen if you disagree.