Bug 5724 - Sometimes deletes aren't processed correctly by rebuild zebra
Summary: Sometimes deletes aren't processed correctly by rebuild zebra
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) major (vote)
Target Milestone: ---
Assignee: dev_patches
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on: 5725
Blocks: 5574
  Show dependency treegraph
 
Reported: 2011-02-08 22:03 UTC by Chris Cormack
Modified: 2012-10-26 00:40 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Proposed Patch (4.44 KB, patch)
2011-02-08 22:14 UTC, Chris Cormack
Details | Diff | Splinter Review
follow-up to frydolin comment (1.10 KB, patch)
2011-03-01 22:00 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 5724: Handling deleted records better (4.09 KB, patch)
2011-09-02 09:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5724: Handling deleted records better (4.19 KB, patch)
2011-09-02 09:32 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-02-08 22:03:47 UTC
Since the update of biblios would be done AFTER the deletion of biblios, and since biblios would be updated just before being deleted (in order to delete the items) update process would recreate the biblio.
Comment 1 Chris Cormack 2011-02-08 22:14:33 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-02-08 22:15:03 UTC
I'm unsure about changing the log level, is there a reason that was done?
Comment 3 Chris Cormack 2011-02-08 22:45:37 UTC
This depends on the changes to C4/Biblio.pm in bug 5725
Comment 4 Fridolin SOMERS 2011-02-16 13:28:46 UTC
(In reply to comment #1)
> Created attachment 3104 [details] [review]
> Proposed Patch

When adding UNION : 

    my $fetch_sth = $dbh->prepare_cached("SELECT marc FROM biblioitems WHERE biblionumber = ? UNION SELECT marc from deletedbiblioitems where biblionumber=?");
    $fetch_sth->execute($record_number);

Shouldn't you add the parameter in execution of the query :
    $fetch_sth->execute($record_number, $record_number);
Comment 5 Paul Poulain 2011-03-01 21:54:00 UTC
well spotted fridolyn !
Comment 6 Paul Poulain 2011-03-01 22:00:02 UTC Comment hidden (obsolete)
Comment 7 MJ Ray (software.coop) 2011-03-02 18:47:36 UTC
This bug is mentioned in:
Bug 5724: Handling deleted records better,	note this patch changes the reporting level of zebraidx too http://lists.koha-community.org/pipermail/koha-patches/2011-February/013790.html
Comment 8 Julian Maurice 2011-03-11 15:51:46 UTC
Patch does not apply because of some changes in rebuild_zebra.pl since writing of the patch. See bug 5831.
Comment 9 Paul Poulain 2011-04-08 13:03:49 UTC
I had no problem applying those patches with git am -iu3 :

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 5724: Handling deleted records better, note this patch changes the reporting level of zebraidx too
Using index info to reconstruct a base tree...
<stdin>:39: trailing whitespace.
        do_indexing($record_type, 'delete', "$directory/del_$record_type", $reset, $noshadow, $record_fmt, $zebraidx_log_opt) 
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging misc/migration_tools/rebuild_zebra.pl


back to Needs Signoff
Comment 10 Chris Cormack 2011-05-12 23:22:45 UTC
These patches still apply, and seem to work fine, however, 
-my $zebraidx_log_opt = " -v none,fatal,warn ";
+my $zebraidx_log_opt = " -v none,fatal ";

They contain this side effect, is there a reason we changed the behaviour of the -v switch?

Apart from that, I would be happy to sign off
Comment 11 Paul Poulain 2011-05-13 12:39:13 UTC
Chris,
The warn make zebra very verbose. If you have a small mistake in the indexes for example, you'll get an error.
As you put the cronjob every minut, it means you would get a mail every minut (except if you >/dev/null, but that would be a mistake. If a real problem happens, you would miss it !)

(I agree it could have been put in another patch though, it's not related to the delete not processed...)
HTH
Comment 12 Chris Cormack 2011-05-13 17:29:06 UTC
I might take it out, and put that in its own bug.
Also if we change -v to do that, i might add -vv to be more verbose (it is quite handy when setting up indexes to be able to see all the warnings too)

Would that be ok? Remove the -v change from here, and make a new bug that implements it + -vv

If so, I will do that, and ask you to sign off on them :)
Comment 13 Ian Walls 2011-05-13 17:35:29 UTC
What about making -v take a numeric value? 1 for -v and 2 instead of -vv?
Comment 14 Chris Cormack 2011-05-15 23:35:35 UTC
I have no strong opinions either way, but I will refactor these patches to take the -v change out if Paul is ok with that.

And make a new bug to deal with making verbose less verbose
Comment 15 Paul Poulain 2011-05-16 07:48:22 UTC
(In reply to comment #14)
> I have no strong opinions either way, but I will refactor these patches to take
> the -v change out if Paul is ok with that.
> 
> And make a new bug to deal with making verbose less verbose


I'm OK with that.
And maybe the best would be to be able to pass a parameter. But there are already so many parameters, i'm not sure it would be a so good idea !
Comment 16 dev_patches 2011-06-10 02:06:32 UTC
Should be noted though that the proposed solution  adds some overload in rebuild_zebra fetching the deletedbiblioitems table...
Maybe lesser load process could be envision...
GetMarcBiblio($biblioid,'deleted') could fetch only deleted biblios... And not on both tables...
And instead of using UNION, use either table when processing deleted biblios and normal entries.
That would certainly help a lot. But still that patch fixes a serious issue...
Patch welcome to improve them.
Comment 17 Katrin Fischer 2011-06-15 14:06:46 UTC
Can you provide a test plan for this?
Comment 18 Marcel de Rooy 2011-09-02 08:53:26 UTC
GBSD#4 Looking at this one..
Comment 19 Marcel de Rooy 2011-09-02 09:07:17 UTC
../5724a.patch:51: trailing whitespace.
        do_indexing($record_type, 'delete', "$directory/del_$record_type", $reset, $noshadow, $record_fmt, $zebraidx_log_opt)
error: patch failed: misc/migration_tools/rebuild_zebra.pl:34
error: misc/migration_tools/rebuild_zebra.pl: patch does not apply

Changing status.
From the above comments I understand that Chris will refactor this code too.
Comment 20 Chris Cormack 2011-09-02 09:29:44 UTC Comment hidden (obsolete)
Comment 21 Chris Cormack 2011-09-02 09:32:15 UTC
Created attachment 5279 [details] [review]
Bug 5724: Handling deleted records better

MT3915: rebuild_zebra changes because deleted biblios would stick in zebra indexes

    Since the update of biblios would be done AFTER the deletion of biblios,
    and since biblios would be updated just before being deleted (in order to delete the items)
    update process would recreate the biblio.

    This patch fixes the order in order to do the deletion AFTER the update.
Comment 22 Chris Cormack 2011-09-02 09:33:06 UTC
Rebased, merged the 2 patches and removed the change to the log level
Comment 23 Marcel de Rooy 2011-09-02 09:49:21 UTC
GBSD#4 Revisiting this one now.
Comment 24 Marcel de Rooy 2011-09-02 12:16:50 UTC
My comments FWIW:

I cannot reproduce this bug by deleting records and updating zebra. (Note that there is no test plan.) I can imagine that deleting a record while zebra is updating occasionally could result in a corrupted index. Harder to test.
But I still have my doubts if this code really works. Some comments about it:

1) [MAIN point] The order of updating and deleting is changed. But does it really make a difference? Routine export_marc_records_from_list already filters out deleted records with:
foreach my $record_number ( map { $_->{biblio_auth_number} }
                                grep { !$found{ $_->{biblio_auth_number} }++ }
                                @$entries ) {
[side note: the increment (++) after $found has no effect; the value is already 1 or undef]
So the update file does not contain deleted recs already (without this patch)! Changing the order is fine, but appears to have no effect. The two files (delete and update) are mutually exclusive on record numbers.

2) In routine get_raw_marc_record:
statement eval { $marc = GetMarcBiblio($record_number,"include_deleted_table"); } suggests something like including deleted records but the second parameter is actually asking for embedded items (see Biblio.pm)

3) generate_deleted_marc_records calls fix_biblio_ids with two record numbers, so it will never hit the deleted table
it does only hit the deletedbiblioitems from get_corrected_marc_record (called from export_marc_records_from_list)

4) Small whitespace error:
../5724.patch:42: trailing whitespace.
        do_indexing($record_type, 'delete', "$directory/del_$record_type", $reset, $noshadow, $record_fmt, $zebraidx_log_opt)
warning: 1 line adds whitespace errors.

Changing status.
Comment 25 dev_patches 2011-09-02 13:47:52 UTC
There is a problem when you edit a biblio just before deleting it.
When exporting biblios, koha will export and do the deletion first.
Then it will recreate the biblio with the upd exported iso2907.

Just stop your cronjob,
edit a record, then delete it.
Then try to do rebuild_zebra.pl -b -z 
And search the title (hoping that you have the list and not geting directly to the record... in case of which you are directed to 404)

The record is not deleted from zebra...


This patch was meant for that problem.
I think that simply changing delete/update order would be enough.
But it is an old patch, and was written before bug 5959.
Comment 26 Marcel de Rooy 2011-09-08 08:49:06 UTC
(In reply to comment #25)
> There is a problem when you edit a biblio just before deleting it.
> When exporting biblios, koha will export and do the deletion first.
> Then it will recreate the biblio with the upd exported iso2907.
> Just stop your cronjob,
> edit a record, then delete it.
> Then try to do rebuild_zebra.pl -b -z 
> And search the title (hoping that you have the list and not geting directly to
> the record... in case of which you are directed to 404)
> The record is not deleted from zebra...
> This patch was meant for that problem.
> I think that simply changing delete/update order would be enough.
> But it is an old patch, and was written before bug 5959.

Test results:

mysql> select * from zebraqueue where done=0;
+--------+--------------------+---------------+--------------+------+---------------------+
| id     | biblio_auth_number | operation     | server       | done | time                |
+--------+--------------------+---------------+--------------+------+---------------------+
| 123457 |             120900 | specialUpdate | biblioserver |    0 | 2011-09-08 10:26:23 |
| 123458 |             120900 | recordDelete  | biblioserver |    0 | 2011-09-08 10:26:39 |
+--------+--------------------+---------------+--------------+------+---------------------+
2 rows in set (0.32 sec)

====================
exporting biblio
====================
^M1.
Records exported: 1

10:28:18-08/09 zebraidx(30076) [log] dir /tmp/jcwrMc7Wq3/del_biblio
10:28:18-08/09 zebraidx(30076) [log] delete grs.marcxml.record /tmp/jcwrMc7Wq3/del_biblio/exported_records 0
10:28:20-08/09 zebraidx(30076) [log] Iterations: isam/dict 1269/458
10:28:20-08/09 zebraidx(30076) [log] Dict: inserts/updates/deletions: 1/384/73
10:28:20-08/09 zebraidx(30076) [log] Records: 1 i/u/d 0/0/1


So, the steps do not reproduce the problem on current master. The record is correctly deleted from zebra! Please note that I already explained why in comment 24. Changing order is not relevant. Probably, this could have worked in an older version of the script, but it is not actual anymore.

By the way: There is a (very) small timeframe between selecting records for update/delete and marking them as done in the zebraqueue table. If a record gets updated or deleted just in between, that zebraqueue record is not selected but it does get marked as done. This would result in not updating zebra or incorrectly keeping that record in zebra.
You could resolve it by e.g. changing the done column temporarily to a unique number indicating selection. And only updating these selected records to 1 later on. (Or do something with locking.) 
But it is a rather fictive situation.
Comment 27 Paul Poulain 2011-09-15 09:25:28 UTC
agreed with Marcel comments