Bug 10478 - Do we need a sequential number generator?
Summary: Do we need a sequential number generator?
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-17 07:51 UTC by Marcel de Rooy
Modified: 2015-09-14 21:04 UTC (History)
3 users (show)

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


Attachments
Bug 10478: Sequential number generator (1.27 KB, patch)
2013-06-20 16:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-06-17 07:51:11 UTC
Copy and past from report 9921:

--- Comment #15 from Galen Charlton <gmcharlt@gmail.com> ---
(In reply to comment #14)
> 3) I feel that incrementalControlNumber is not (really) a pref. I can
> imagine more cases where we might need a sequential number. Would it be
> interesting to move it perhaps to Tools in a more general context such as a
> sequence number engine with its own table (id, keyname, seqnumber, date)? 
> There are more fields like callnumber (with plugins) that could use such an
> approach..
> This does again not block your patch, but we could trigger the discussion on
> a more general solution.

This idea (creating a sequence engine) is worth elevating to a new enhancement
bug.  There's been some chatter on #koha the past few days about this in the
context of improving how sequential patron cardnumbers are assigned.

[END-OF-QUOTE]

Note: Since this bug is not yet in assigned status, you are welcome to write some code :)
It would probably be interesting too to add another column subkey or something that allows you to have sequence number for the combination key, subkey.
You could e.g. have numbers then for 'shelf', '1 A' and 'shelf','2 C', etc.
But this idea could have many more applications.
Comment 1 Marcel de Rooy 2013-06-20 16:29:57 UTC
Created attachment 19175 [details] [review]
Bug 10478: Sequential number generator
Comment 2 Marcel de Rooy 2013-06-20 16:30:38 UTC
(In reply to comment #1)
> Created attachment 19175 [details] [review] [review]
> Bug 10478: Sequential number generator

Just as an example of what I had in mind..
Comment 3 Mark Tompsett 2013-06-22 04:55:16 UTC
M. de Rooy, you use MySQL's LAST_INSERT_ID, which is not forward compatible with people who may be attempting to use a different back end, or working towards that.

The next problem is code that calls something to get the next number without actually having put it into a table anywhere and then calling it again when someone refreshed. Do we really want such gaping holes in the sequences we store?

Is the session id number, as stored in the CGISESSID cookie, unique enough, such that one could look for the session id to grab the sequence value to see if it is in the destination table, and if it is generate a new sequence number, otherwise reuse the one that is already pre-calculated?

This is the situation I am pondering on the card number specific bug 10454.
Comment 4 Marcel de Rooy 2013-06-22 10:56:03 UTC
(In reply to comment #3)
> M. de Rooy, you use MySQL's LAST_INSERT_ID, which is not forward compatible
> with people who may be attempting to use a different back end, or working
> towards that.

See also more discussion about that on bug 9921. Note that it is still quite hypothetical when Koha contains tens of mysql_insert_ids and other constructs with specific date functions etc. But we should improve :)
The LAST_INSERT_ID construct prevents us from coding an exclusive row level lock. The area of locking is also quite driver-sensitive. If I am correct, e.g. PostgreSQL only blocks writing with such a lock and MySQL can block the read with serializable isolation level, etc.
Whatever we do here, it looks like that we need some lines per db implementation. If we add this for MySQL, everybody is welcome to add lines for Pg, etc. (Not even talking about DBIx::Class yet :)
 
> The next problem is code that calls something to get the next number without
> actually having put it into a table anywhere and then calling it again when
> someone refreshed. Do we really want such gaping holes in the sequences we
> store?

You could assign the number only when you save the record. But still records will be deleted. So there will always be gaps. The sequence number generator could either not care about that, or also retrieve numbers from some kind of buffer that is filled by a 'seqno garbage collector' (additional functionality). Could you mention a case where this is considered important?

> Is the session id number, as stored in the CGISESSID cookie, unique enough,
> such that one could look for the session id to grab the sequence value to
> see if it is in the destination table, and if it is generate a new sequence
> number, otherwise reuse the one that is already pre-calculated?

Sounds like you want a random number instead now, why not just use a cpan random number generator (several options) instead ?

Marcel
Comment 5 Mark Tompsett 2013-06-22 18:00:50 UTC
I was playing with DBI's last_insert_id and I'm generally impressed. I can't seem to trick it into giving me results I don't want. Even in threaded and forked code, because the database connection handle is different, last_insert_id returned the last_insert_id of the process or thread I was expecting. So, even a split command which uses DBI's last_insert_id is okay, in my assessment. You can see what I mean by a split command (INSERT and last_insert_id separated) in my patch on 10454.

While simplifying my patch (see bug 10454), I realized that the complexity of a Process_ID (to uniquely ID which process/thread is running this) I had aided me in preventing gaps created by some forms of mistakes (hitting refresh while adding patron a couple times). However, I have posted my simplified code, which is subject to huge fake gaps (keep hitting that refresh on a patron entry screen).

Overall, this isn't really just a sequential number problem (let's just +1), it's a sequential STRING problem. If you looked at my patch, I actually calculate my_UID (because I add a blank cardnumber to be filled in and grab last_insert_id), calculate all the cardnumbers which haven't been calculated yet (effectively $currentvalue = nextvalue($previousvalue) then updating the UID record being calculated), and once everything is calculated, I grab my value using my_UID.
Comment 6 Mark Tompsett 2013-06-27 06:21:42 UTC
I almost have a nice generic class. I just have to touch up a couple things. I hope to post something within the next day or two.

It will work for any signed integer sequence. I'll be basing my bug 10454 patch on it.
Comment 7 Marcel de Rooy 2013-06-27 06:57:03 UTC
(In reply to M. Tompsett from comment #6)
> I almost have a nice generic class. I just have to touch up a couple things.
> I hope to post something within the next day or two.
> 
> It will work for any signed integer sequence. I'll be basing my bug 10454
> patch on it.

Looking forward to your code. Could you perhaps extend the generation of this number for the combination range, subrange as describe earlier here?

Do you plan to add code under Tools or Administration to show this table? Viewing the values would at least be interesting. But perhaps initializing a new sequence from a certain number, or resetting a sequence is probably needed too in the long run.. But these functions might need some more thought, because they are pretty dangerous too.

Note also that there is a direct interest in your code for using it in bug 9921 that requires sequence numbers too.
Comment 8 Mark Tompsett 2013-06-27 07:37:34 UTC
(In reply to M. de Rooy from comment #7)
> Looking forward to your code. Could you perhaps extend the generation of
> this number for the combination range, subrange as describe earlier here?

The table is made up of sequence_name and value pairs.
If you want subranges, I suppose you could take the range and subrange together to form a sequence name.


> Do you plan to add code under Tools or Administration to show this table?

Card numbers don't need it.


> Viewing the values would at least be interesting.

I agree.


> But perhaps initializing a new sequence from a certain number,

Done.


> or resetting a sequence

I have a reset_everything method, which if modified for every sequence known to mankind as they are added, could potentially be broken out to reset_sequence.


> is probably
> needed too in the long run.. But these functions might need some more
> thought, because they are pretty dangerous too.

If there's a way to calculate the number to use, resetting to that number isn't so dangerous.


> Note also that there is a direct interest in your code for using it in bug
> 9921 that requires sequence numbers too.

Take a look at my sample test suite and output there.
Comment 9 Colin Campbell 2013-11-01 11:21:54 UTC
There are existing cpan modules to maintain sequences in a db agnostic way, e.g. DBIx::Sequence
Comment 10 Mark Tompsett 2013-11-01 14:03:37 UTC
Colin, for your perusal.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10454#c14