Bug #24295 for DBIx-FullTextSearch: Missing "unlock tables" in case of exceptions

At several places in DBIx::FullTextSearch "lock tables" / "unlock tables" pairs enclose critical code blocks. When exceptions occur within these code blocks, the database is left in the "lock tables" state, because no "unlock tables" is issued. This may lead to an "table xyz was not locked..." MySQL error message later. The attached patch fixes this issue. The critical code blocks are now enclosed in an eval{}. In case of an exception locks will be released first, then the exception will be thrown to the next level. The patch was created by Gert Brinkmann <gert AT dimedis.de>, a colleague of mine. I file this bugreport, because he has no CPAN account.

diff -u DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Blob.pm DBIx_patched/FullTextSearch/Blob.pm --- DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Blob.pm 2007-01-09 18:59:05.691265842 +0100 +++ DBIx_patched/FullTextSearch/Blob.pm 2007-01-09 17:52:50.812997385 +0100 @@ -84,53 +84,61 @@ my @insert_values; + my $num_words = 0; $dbh->do("lock tables $data_table write"); - my $select_sth = $dbh->prepare("select word from $data_table"); - $select_sth->execute; - - my $packstring = $DBIx::FullTextSearch::BITS_TO_PACK{$fts->{'doc_id_bits'}} - . $DBIx::FullTextSearch::BITS_TO_PACK{$fts->{'count_bits'}}; - my ($packnulls) = pack $packstring, 0, 0; - my $packlength = length $packnulls; - my $num_words = 0; - while (my ($word) = $select_sth->fetchrow_array) { - my $value = (defined $words->{$word} ? - pack($packstring, $id, $words->{$word}) : ''); - - # the method find_position finds the position of the - # "record" for document $id with word $word; returned is - # the position in bytes and yes/no values specifying if - # the record is already present in the blob; if it is, - # we need to replace it, otherwise just insert. - - my ($pos, $shift) = $self->find_position($word, $id); - if (not defined $pos) { - push @insert_values, $word, $value; - } - else { - my $spos = $pos + 1; # I'm not sure why this - $spos += $packlength if $shift; - $update_sth->execute($pos, $value, $spos, $word); - } - delete $words->{$word}; - $num_words++ if defined $value; - } - - for my $word ( keys %$words ) { - my $value = pack $packstring, $id, $words->{$word}; - push @insert_values, $word, $value; -# $insert_sth->execute($word, $value); - $num_words++; - } - - if(@insert_values){ - my $sql_str = "insert into $data_table values ". join(',', ('(?, ?)') x (@insert_values/2)); - $dbh->do($sql_str,{},@insert_values); - } - - $dbh->do("unlock tables"); + #---- In case of an exception the table should be unlocked. + eval { + my $select_sth = $dbh->prepare("select word from $data_table"); + $select_sth->execute; + + my $packstring = $DBIx::FullTextSearch::BITS_TO_PACK{$fts->{'doc_id_bits'}} + . $DBIx::FullTextSearch::BITS_TO_PACK{$fts->{'count_bits'}}; + my ($packnulls) = pack $packstring, 0, 0; + my $packlength = length $packnulls; + while (my ($word) = $select_sth->fetchrow_array) { + my $value = (defined $words->{$word} ? + pack($packstring, $id, $words->{$word}) : ''); + + # the method find_position finds the position of the + # "record" for document $id with word $word; returned is + # the position in bytes and yes/no values specifying if + # the record is already present in the blob; if it is, + # we need to replace it, otherwise just insert. + + my ($pos, $shift) = $self->find_position($word, $id); + if (not defined $pos) { + push @insert_values, $word, $value; + } + else { + my $spos = $pos + 1; # I'm not sure why this + $spos += $packlength if $shift; + $update_sth->execute($pos, $value, $spos, $word); + } + delete $words->{$word}; + $num_words++ if defined $value; + } + + for my $word ( keys %$words ) { + my $value = pack $packstring, $id, $words->{$word}; + push @insert_values, $word, $value; + # $insert_sth->execute($word, $value); + $num_words++; + } + + if(@insert_values){ + my $sql_str = "insert into $data_table values ". join(',', ('(?, ?)') x (@insert_values/2)); + $dbh->do($sql_str,{},@insert_values); + } + + $dbh->do("unlock tables"); + }; + + if ( $@ ) { + $dbh->do("unlock tables"); + die $@; + } return $num_words; } diff -u DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Column.pm DBIx_patched/FullTextSearch/Column.pm --- DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Column.pm 2007-01-09 18:59:05.728264600 +0100 +++ DBIx_patched/FullTextSearch/Column.pm 2007-01-09 17:55:28.666130517 +0100 @@ -69,34 +69,45 @@ my $num_words = 0; my (@wids,@data,@widshandler,@datahandler); my $wordid; + $dbh->do("lock tables $word_id_table write"); - my ($maxid) = $dbh->selectrow_array("select max(id) - from $word_id_table"); - foreach my $word (keys %$words) { - if(!defined $self->{'wordids'}->{$word}) { - $self->{'select_wordid_sth'}->execute($word); - ($wordid) = $self->{'select_wordid_sth'}->fetchrow_array(); - unless ($wordid) { - $maxid++; - push @widshandler, "(?,$maxid)"; - push @wids, $word; - $wordid = $maxid; + #---- In case of an exception the table should be unlocked. + eval { + my ($maxid) = $dbh->selectrow_array("select max(id) + from $word_id_table"); + foreach my $word (keys %$words) { + if(!defined $self->{'wordids'}->{$word}) { + $self->{'select_wordid_sth'}->execute($word); + ($wordid) = $self->{'select_wordid_sth'}->fetchrow_array(); + unless ($wordid) { + $maxid++; + push @widshandler, "(?,$maxid)"; + push @wids, $word; + $wordid = $maxid; + } + $self->{'wordids'}->{$word} = $wordid; + } else { + $wordid=$self->{'wordids'}->{$word}; } - $self->{'wordids'}->{$word} = $wordid; - } else { - $wordid=$self->{'wordids'}->{$word}; - } - if ($count_bits) { - push @datahandler, "($wordid,$id,?)"; - push @data, $words->{$word}; - } else { - push @datahandler, "($wordid,$id)"; - } - $num_words++; + if ($count_bits) { + push @datahandler, "($wordid,$id,?)"; + push @data, $words->{$word}; + } else { + push @datahandler, "($wordid,$id)"; + } + $num_words++; + }; + $dbh->do("insert into $word_id_table values " . + join (',',@widshandler),undef,@wids) if @wids; + $dbh->do("unlock tables"); }; - $dbh->do("insert into $word_id_table values " . - join (',',@widshandler),undef,@wids) if @wids; - $dbh->do("unlock tables"); + + if ( $@ ) { + $dbh->do("unlock tables"); + die $@; + } + + if ($count_bits) { $dbh->do("insert into $data_table values " . join (',',@datahandler),undef,@data) if @data; } else { diff -u DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Phrase.pm DBIx_patched/FullTextSearch/Phrase.pm --- DBIx-FullTextSearch-0.73/lib/DBIx/FullTextSearch/Phrase.pm 2007-01-09 18:59:05.648267285 +0100 +++ DBIx_patched/FullTextSearch/Phrase.pm 2007-01-09 18:02:23.669796748 +0100 @@ -72,30 +72,41 @@ my $num_words = 0; my (@wids,@data,@widshandler,@datahandler); my $wordid; + $dbh->do("lock tables $word_id_table write"); - my ($maxid) = $dbh->selectrow_array("select max(id) - from $word_id_table"); - foreach my $word (keys %$words) { - if(!defined $self->{'wordids'}->{$word}) { - $self->{'select_wordid_sth'}->execute($word); - ($wordid) = $self->{'select_wordid_sth'}->fetchrow_array(); - unless ($wordid) { - $maxid++; - push @widshandler, "(?,$maxid)"; - push @wids, $word; - $wordid = $maxid; + #---- In case of an exception the table should be unlocked. + eval { + my ($maxid) = $dbh->selectrow_array("select max(id) + from $word_id_table"); + foreach my $word (keys %$words) { + if(!defined $self->{'wordids'}->{$word}) { + $self->{'select_wordid_sth'}->execute($word); + ($wordid) = $self->{'select_wordid_sth'}->fetchrow_array(); + unless ($wordid) { + $maxid++; + push @widshandler, "(?,$maxid)"; + push @wids, $word; + $wordid = $maxid; + } + $self->{'wordids'}->{$word} = $wordid; + } else { + $wordid=$self->{'wordids'}->{$word}; } - $self->{'wordids'}->{$word} = $wordid; - } else { - $wordid=$self->{'wordids'}->{$word}; - } - push @datahandler, "($wordid,$id,?)"; - push @data, pack $packstring.'*', @{$words->{$word}}; - $num_words++; + push @datahandler, "($wordid,$id,?)"; + push @data, pack $packstring.'*', @{$words->{$word}}; + $num_words++; + }; + $dbh->do("insert into $word_id_table values " . + join (',',@widshandler),undef,@wids) if @wids; + + $dbh->do("unlock tables"); }; - $dbh->do("insert into $word_id_table values " . - join (',',@widshandler),undef,@wids) if @wids; - $dbh->do("unlock tables"); + + if ( $@ ) { + $dbh->do("unlock tables"); + die $@; + } + $dbh->do("insert into $data_table values " . join (',',@datahandler),undef,@data) if @data; return $num_words;