Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need
Summary: UpdateFine() modyfies existing fine records even when there is no need
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15342
  Show dependency treegraph
 
Reported: 2016-08-17 09:26 UTC by Jacek Ablewicz
Modified: 2018-12-12 13:42 UTC (History)
6 users (show)

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


Attachments
Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need (2.41 KB, patch)
2016-08-17 10:04 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17138: Add and use comparison function and tests (3.50 KB, patch)
2017-01-03 03:05 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need (2.47 KB, patch)
2017-02-01 21:15 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17138: Add and use comparison function and tests (3.57 KB, patch)
2017-02-01 21:15 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-08-17 09:26:34 UTC
In C4/Overdues.pm:

  574:     # we're updating an existing fine.  Only modify if amount changed

  577:     if ( $data->{'amount'} != $amount ) {

for some numbers (e.g. 5.60) the comparison in line 577 fails ($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be a floating point number with no exact binary representation). E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3% of the amounts (on average).

Depending on the fine charging settings, this may trigger unnecessary database writes (and a lot of them). If 'Fine Charging Interval' is 1 day, this has very little practical consequences (misc/cronjobs/fines.pl script is usually run once per day, and all fines need an increase anyway). But if the charging interval is longer, e.g. 7 days, in a given day amount changes only for the 1/7 of the fines (again, on average), and over 60% of the database writes would be redundant.

To reproduce:
1) run misc/cronjobs/fines.pl
2) add

   warn "AMOUNT ".$data->{'amount'}." vs $amount";

below line 577 in C4/Overdues.pm
3) run misc/cronjobs/fines.pl again
4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6'

or: dump accountlines table after step 1), run the script 2nd time, dump the table again and compare the contents - some of the records would have a different timestamp.
Comment 1 Jacek Ablewicz 2016-08-17 10:04:55 UTC
Created attachment 54532 [details] [review]
Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need

In C4/Overdues.pm:

  574:     # we're updating an existing fine.  Only modify if amount changed

  577:     if ( $data->{'amount'} != $amount ) {

for some numbers (e.g. 5.60) the comparison in line 577 fails
($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be
a floating point number with no exact binary representation).
E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3%
of the amounts (on average).

Depending on the fine charging settings, this may trigger unnecessary
database writes (and a lot of them). If 'Fine Charging Interval' is
1 day, this has very little practical consequences (misc/cronjobs/fines.pl
script is usually run once per day, and all fines need an increase
anyway). But if the charging interval is longer, e.g. 7 days, in a given
day amount changes only for the 1/7 of the fines (again, on average),
and over 60% of the database writes would be redundant.

To reproduce:

1) run misc/cronjobs/fines.pl
2) add

   warn "AMOUNT ".$data->{'amount'}." vs $amount";

below line 577 in C4/Overdues.pm
3) run misc/cronjobs/fines.pl again
4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6'

or: dump accountlines table after step 1), run the script 2nd time, dump
the table again and compare the contents - some of the records would have
a different timestamp.

To test:

1) apply patch
2) redo steps 1) - 4) above
3) no more warnings in step 4
4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster
Comment 2 Jacek Ablewicz 2016-08-17 10:19:14 UTC
(In reply to Jacek Ablewicz from comment #1)

> 4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster

On my test setup (19200 overdues, ~6500 redundant updates, FinesLog disabled) fines.pl run time was 90 seconds (before) vs 50 seconds (after). Not very striking difference, but this is on server with SSD drive, and

   innodb_flush_log_at_trx_commit = 2

in mysql config; YMMV.
Comment 3 Jacek Ablewicz 2016-08-23 13:16:55 UTC
(In reply to Jacek Ablewicz from comment #1)

> Depending on the fine charging settings, this may trigger unnecessary
> database writes (and a lot of them). If 'Fine Charging Interval' is
> 1 day, this has very little practical consequences (misc/cronjobs/fines.pl
> script is usually run once per day, and all fines need an increase
> anyway).

After a closer look - this is not entirely true. If the library has fine caps enabled (for individual items, or a total limit for all overdues), unnecessary database writes may happen there as well, even if 'Fine Charging Interval' is
1 day.
Comment 4 Marcel de Rooy 2016-08-23 14:05:40 UTC
+ if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {

Why not force them both to sprintf and get rid of the hardcoded 6 ?
Or would rounding with Math::Round::nearest be better than sprintf ?

    my $info = $schema->source('Accountline')->column_info('amount');
    my $scale = $info->{size}->[1];
Comment 5 Jacek Ablewicz 2016-08-25 09:38:48 UTC
(In reply to Marcel de Rooy from comment #4)
> + if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {

> Or would rounding with Math::Round::nearest be better than sprintf ?

Quite possibly; sprintf('%.6f', ...) is not ideal, theoretically this comparison may still fail (but less often then currently) for some borderline cases, because mysql is supposedly using slightly different kind of rounding for internal decimal(x,y) arithmetic then sprintf (sprintf: half to even aka bankers rounding, mysql - half away from zero, commercial rounding).

If someone is having numbers like 1.1212125 (more then 6 fractional digits, and the 7th digit is 5) in circulation & fine rules, calculated fine amount may get rounded differently. In such case though, fines.pl being not always as fast as humanly possible would be the least of his/her problems ;)

> Why not force them both to sprintf and get rid of the hardcoded 6 ?

I guess this can be done in other ways like

   abs($amount1 - $amount2) < 0.000001

but IMO that specific .6 precision is kind of important?

>     my $info = $schema->source('Accountline')->column_info('amount');
>     my $scale = $info->{size}->[1];

This (and adding a new module for more predictable rounding etc.) would be a good start for solving all prices / fines rounding problems in Koha, once and for all.. I'm just not entirely happy with starting such monumental task right in this report, looks like a bit of an overkill to me.
Comment 6 Marcel de Rooy 2016-08-25 09:43:03 UTC
(In reply to Jacek Ablewicz from comment #5)
> This (and adding a new module for more predictable rounding etc.) would be a
> good start for solving all prices / fines rounding problems in Koha, once
> and for all.. I'm just not entirely happy with starting such monumental task
> right in this report, looks like a bit of an overkill to me.

Agreed. But replacing a hardcoded 6 by one or two lines might not be that monumental ?
Comment 7 Jacek Ablewicz 2016-08-25 10:39:46 UTC
(In reply to Marcel de Rooy from comment #6)
> (In reply to Jacek Ablewicz from comment #5)
> > This (and adding a new module for more predictable rounding etc.) would be a
> > good start for solving all prices / fines rounding problems in Koha, once
> > and for all.. I'm just not entirely happy with starting such monumental task
> > right in this report, looks like a bit of an overkill to me.
> 
> Agreed. But replacing a hardcoded 6 by one or two lines might not be that
> monumental ?

Ok, I'm happy with that; I don't have any strong opinion either way. My gut feeling though is that it would be better to have this in some kind of module, so it will be reusable. Otherwise, I'm worried that something like that would need to be included in each and every places where calculations in Koha are being tinkered with.

But I'll better check first if those DBIx calls are not too costly. One never knows - this DBIx stuff is full of surprises, and not always the pleasant ones..
Comment 8 M. Tompsett 2017-01-02 20:15:11 UTC
Comment on attachment 54532 [details] [review]
Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need

Review of attachment 54532 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +574,4 @@
>          # we're updating an existing fine.  Only modify if amount changed
>          # Note that in the current implementation, you cannot pay against an accruing fine
>          # (i.e. , of accounttype 'FU').  Doing so will break accrual.
> +        if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {

While this does work, would not a compare_numbers($n1,$n2,$accuracy) function work better? Let's not rely on implicit type conversions for $data->{'amount'}.
Comment 9 Jacek Ablewicz 2017-01-02 21:57:22 UTC
(In reply to M. Tompsett from comment #8)
> Comment on attachment 54532 [details] [review] [review]
> Bug 17138 - UpdateFine() modyfies existing fine records even when there is
> no need
> 
> Review of attachment 54532 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Overdues.pm
> @@ +574,4 @@
> >          # we're updating an existing fine.  Only modify if amount changed
> >          # Note that in the current implementation, you cannot pay against an accruing fine
> >          # (i.e. , of accounttype 'FU').  Doing so will break accrual.
> > +        if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {
> 
> While this does work, would not a compare_numbers($n1,$n2,$accuracy)
> function work better?

I'm not familiar with this function (compare_numbers), and google doesn't yield any (useful) results; from which module is it?
Comment 10 M. Tompsett 2017-01-03 03:03:43 UTC
(In reply to Jacek Ablewicz from comment #9)
> (In reply to M. Tompsett from comment #8)
> > Comment on attachment 54532 [details] [review] [review] [review]
> > Bug 17138 - UpdateFine() modyfies existing fine records even when there is
> > no need
> > 
> > Review of attachment 54532 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: C4/Overdues.pm
> > @@ +574,4 @@
> > >          # we're updating an existing fine.  Only modify if amount changed
> > >          # Note that in the current implementation, you cannot pay against an accruing fine
> > >          # (i.e. , of accounttype 'FU').  Doing so will break accrual.
> > > +        if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {
> > 
> > While this does work, would not a compare_numbers($n1,$n2,$accuracy)
> > function work better?
> 
> I'm not familiar with this function (compare_numbers), and google doesn't
> yield any (useful) results; from which module is it?

I was implying writing it... like... wait for it...
Comment 11 M. Tompsett 2017-01-03 03:05:01 UTC
Created attachment 58559 [details] [review]
Bug 17138: Add and use comparison function and tests

Added C4::Koha::compareNumbers($n1,$n2,$accuracy).
Added corresponding tests to t/Koha.t
Tweaked C4/Overdues to use the new function.
Comment 12 Josef Moravec 2017-02-01 21:15:12 UTC
Created attachment 59755 [details] [review]
[SIGNED-OFF] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need

In C4/Overdues.pm:

  574:     # we're updating an existing fine.  Only modify if amount changed

  577:     if ( $data->{'amount'} != $amount ) {

for some numbers (e.g. 5.60) the comparison in line 577 fails
($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be
a floating point number with no exact binary representation).
E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3%
of the amounts (on average).

Depending on the fine charging settings, this may trigger unnecessary
database writes (and a lot of them). If 'Fine Charging Interval' is
1 day, this has very little practical consequences (misc/cronjobs/fines.pl
script is usually run once per day, and all fines need an increase
anyway). But if the charging interval is longer, e.g. 7 days, in a given
day amount changes only for the 1/7 of the fines (again, on average),
and over 60% of the database writes would be redundant.

To reproduce:

1) run misc/cronjobs/fines.pl
2) add

   warn "AMOUNT ".$data->{'amount'}." vs $amount";

below line 577 in C4/Overdues.pm
3) run misc/cronjobs/fines.pl again
4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6'

or: dump accountlines table after step 1), run the script 2nd time, dump
the table again and compare the contents - some of the records would have
a different timestamp.

To test:

1) apply patch
2) redo steps 1) - 4) above
3) no more warnings in step 4
4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2017-02-01 21:15:24 UTC
Created attachment 59756 [details] [review]
[SIGNED-OFF] Bug 17138: Add and use comparison function and tests

Added C4::Koha::compareNumbers($n1,$n2,$accuracy).
Added corresponding tests to t/Koha.t
Tweaked C4/Overdues to use the new function.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Jonathan Druart 2017-02-02 08:19:29 UTC
(In reply to Josef Moravec from comment #13)
> Created attachment 59756 [details] [review] [review]
> [SIGNED-OFF] Bug 17138: Add and use comparison function and tests

I think you should use Number::Format instead of creating a new subroutine.
In any cases I am pretty sure that C4::Koha is the wrong place to put it in :)