Bug 10454 - Duplicate card numbers may be generated
Summary: Duplicate card numbers may be generated
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-12 23:54 UTC by Mark Tompsett
Modified: 2023-07-08 16:06 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10454 - Duplicate card numbers may be generated (11.93 KB, patch)
2013-06-22 17:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10454 - Duplicate card numbers may be generated (20.28 KB, patch)
2013-06-27 15:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10454 - Duplicate card numbers may be generated (19.84 KB, patch)
2013-06-27 15:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
sign off of the bug (19.89 KB, patch)
2013-09-05 13:35 UTC, Paola Rossi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2013-06-12 23:54:30 UTC
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.
Comment 1 Mark Tompsett 2013-06-13 03:06:03 UTC
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.
Comment 2 Mark Tompsett 2013-06-13 03:08:28 UTC
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.
Comment 3 Mark Tompsett 2013-06-13 18:36:33 UTC
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;
Comment 4 Mark Tompsett 2013-06-22 04:31:06 UTC
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.
Comment 5 Marcel de Rooy 2013-06-22 10:58:05 UTC
(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?
Comment 6 Mark Tompsett 2013-06-22 16:20:23 UTC
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.
Comment 7 Mark Tompsett 2013-06-22 17:30:53 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2013-06-22 18:14:07 UTC
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.
Comment 9 Mark Tompsett 2013-06-22 21:15:05 UTC
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.
Comment 10 Marcel de Rooy 2013-06-26 14:10:27 UTC
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.)
Comment 11 Mark Tompsett 2013-06-26 15:26:58 UTC
(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.
Comment 12 Galen Charlton 2013-06-26 16:57:06 UTC
(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___
Comment 13 Galen Charlton 2013-06-26 17:31:19 UTC
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___
Comment 14 Galen Charlton 2013-06-26 18:35:28 UTC
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".
Comment 15 Marcel de Rooy 2013-06-27 06:50:10 UTC
(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..?
Comment 16 Mark Tompsett 2013-06-27 15:50:05 UTC Comment hidden (obsolete)
Comment 17 Mark Tompsett 2013-06-27 15:53:42 UTC Comment hidden (obsolete)
Comment 18 Mark Tompsett 2013-06-27 15:56:42 UTC
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.
Comment 19 Paola Rossi 2013-09-05 13:35:08 UTC
Created attachment 20800 [details] [review]
sign off of the bug
Comment 20 Kyle M Hall 2013-09-12 17:10:30 UTC
Failing QA for the MySQLisms in the perl module. Is it possible to rewrite this code without MySQLisms?
Comment 21 Mark Tompsett 2013-09-12 19:00:19 UTC
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.
Comment 22 Zeno Tajoli 2013-09-29 08:05:05 UTC
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
Comment 23 Mark Tompsett 2013-09-30 00:14:07 UTC
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.
Comment 24 Jonathan Druart 2015-08-19 15:32:34 UTC
No activity for 2 years, lowering the severity.
Comment 25 Katrin Fischer 2023-07-08 16:06:21 UTC
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.