http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |mtompset@hotmail.com |ity.org | Summary|Failure to lock could |Lack of critical section |generate duplicate card |could generate duplicate |numbers |card numbers --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- 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; -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #5 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 19196 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19196&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #13 from Galen Charlton <gmcharlt@gmail.com> --- 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___ -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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..?

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19264|0 |1 is obsolete| | --- Comment #17 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 19265 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19265&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9921 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 Paola Rossi <paola.rossi@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |paola.rossi@cineca.it -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com QA Contact| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #21 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 --- Comment #23 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.