Attachment 117876 Details for Bug 26457 – Bug 26457: [20.05.x] Throw exception if update of issues table fails

[patch] Bug 26457: [20.05.x] Throw exception if update of issues table fails

Bug-26457-2005x-Throw-exception-if-update-of-issue.patch (text/plain), 5.61 KB, created by Nick Clemens (kidclamp) on 2021-03-05 20:19:38 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2021-03-05 20:19:38 UTC

Size: 5.61 KB

patch

obsolete

>From 73038e797f3c1d0c86071a2d4cb2c625f1347515 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Fri, 4 Dec 2020 12:03:52 +0000
>Subject: [PATCH] Bug 26457: [20.05.x] Throw exception if update of issues
> table fails
>
>While this won't prevent the deadlock, it should catch the case where
>a deadlock causes the DB update to fail and provide feedback to the user
>and rollback the transaction
>
>I don't know how to trigger the deadlock, I can only confirm that we see it, and
>that this should catch it.
>
>To test:
>1 - Apply patches
>2 - Checkout several items to a patron
>3 - Confirm that 'Renew all' feature continues to work as expected and all items are renewed
>
>Signed-off-by: David Nind <david@davidnind.com>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 26457: Unit test
>
>Signed-off-by: David Nind <david@davidnind.com>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 26457: (QA follow-up) Switch to PK index in UPDATE on issues
>
>The deadlock reports tell us that multiple transactions are
>waiting for a X lock on a record but using a secondary index
>on borrowernumber and itemnumber. Since we have the issue_id
>at hand already, we should use that and benefit from the clustered
>index (on PK) instead of using a secondary index.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> C4/Circulation.pm            | 15 ++++++++++-----
> Koha/Exceptions/Checkout.pm  | 15 +++++++++++++++
> svc/renew                    | 13 +++++++++++--
> t/db_dependent/Circulation.t |  7 ++++++-
> 4 files changed, 42 insertions(+), 8 deletions(-)
> create mode 100644 Koha/Exceptions/Checkout.pm
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index e6ca5038d4..43475b2703 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -60,6 +60,7 @@ use Koha::Charges::Fees;
> use Koha::Config::SysPref;
> use Koha::Checkouts::ReturnClaims;
> use Koha::SearchEngine::Indexer;
>+use Koha::Exceptions::Checkout;
> use Carp;
> use List::MoreUtils qw( uniq any );
> use Scalar::Util qw( looks_like_number );
>@@ -2970,12 +2971,16 @@ sub AddRenewal {
>         # Update the issues record to have the new due date, and a new count
>         # of how many times it has been renewed.
>         my $renews = ( $issue->renewals || 0 ) + 1;
>-        my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ?
>-                                WHERE borrowernumber=?
>-                                AND itemnumber=?"
>-        );
>+        my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? WHERE issue_id = ?");
> 
>-        $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber );
>+        eval{
>+            $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $issue->issue_id );
>+        };
>+        if( $sth->err ){
>+            Koha::Exceptions::Checkout::FailedRenewal->throw(
>+                error => 'Update of issue# ' . $issue->issue_id . ' failed with error: ' . $sth->errstr
>+            );
>+        }
> 
>         # Update the renewal count on the item, and tell zebra to reindex
>         $renews = ( $item_object->renewals || 0 ) + 1;
>diff --git a/Koha/Exceptions/Checkout.pm b/Koha/Exceptions/Checkout.pm
>new file mode 100644
>index 0000000000..ed4b0a5a27
>--- /dev/null
>+++ b/Koha/Exceptions/Checkout.pm
>@@ -0,0 +1,15 @@
>+package Koha::Exceptions::Checkout;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+    'Koha::Exceptions::Checkout' => {
>+        description => "Something went wrong!"
>+    },
>+    'Koha::Exceptions::Checkout::FailedRenewal' => {
>+        isa         => 'Koha::Exceptions::Checkout',
>+        description => "Renewing checkout failed"
>+    },
>+);
>+
>+1;
>diff --git a/svc/renew b/svc/renew
>index 51b915b004..03b1b63559 100755
>--- a/svc/renew
>+++ b/svc/renew
>@@ -21,6 +21,7 @@ use Modern::Perl;
> 
> use CGI;
> use JSON qw(to_json);
>+use Try::Tiny;
> 
> use C4::Circulation;
> use C4::Context;
>@@ -66,8 +67,16 @@ if ( $data->{error} && $data->{error} eq 'on_reserve' && C4::Context->preference
> }
> 
> if ( $data->{renew_okay} ) {
>-    $date_due = AddRenewal( $borrowernumber, $itemnumber, $branchcode, $date_due );
>-    $data->{date_due} = output_pref( { dt => $date_due, as_due_date => 1 } );
>+    try{
>+        $date_due = AddRenewal( $borrowernumber, $itemnumber, $branchcode, $date_due );
>+        $data->{date_due} = output_pref( { dt => $date_due, as_due_date => 1 } );
>+    } catch {
>+        if ( ref($_) eq 'Koha::Exceptions::Checkout::FailedRenewal' ) {
>+            $data->{error} = 'renewal_failed';
>+        } else {
>+            $_->rethrow;
>+        }
>+    };
> }
> 
> print to_json($data);
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index 20aa33bf8c..30f0a9ca08 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -3689,7 +3689,7 @@ subtest 'AddReturn should clear items.onloan for unissued items' => sub {
> 
> subtest 'AddRenewal and AddIssuingCharge tests' => sub {
> 
>-    plan tests => 12;
>+    plan tests => 13;
> 
> 
>     t::lib::Mocks::mock_preference('item-level_itypes', 1);
>@@ -3732,6 +3732,11 @@ subtest 'AddRenewal and AddIssuingCharge tests' => sub {
> 
>     # Check the item out
>     AddIssue( $patron->unblessed, $item->barcode );
>+
>+    throws_ok {
>+        AddRenewal( $patron->borrowernumber, $item->itemnumber, $library->id, undef, {break=>"the_renewal"} );
>+    } 'Koha::Exceptions::Checkout::FailedRenewal', 'Exception is thrown when renewal update to issues fails';
>+
>     t::lib::Mocks::mock_preference( 'RenewalLog', 0 );
>     my $date = output_pref( { dt => dt_from_string(), dateonly => 1, dateformat => 'iso' } );
>     my %params_renewal = (
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 26457: 114184 | 114185 | 114186 | 114187 | 114190 | 114669 | 114697 | 117227 | 117228 | 117371 | 117372 | 117373 | 117876 | 118647