From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 88C581F4C0 for ; Mon, 21 Oct 2019 11:02:21 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 0/3] fix reindex on multiple + overlapping Message-IDs Date: Mon, 21 Oct 2019 11:02:18 +0000 Message-Id: <20191021110221.23753-1-e@80x24.org> In-Reply-To: <20191016211415.GA6084@dcvr> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: This fixes my initial problems around --reindex not picking up previously-skipped messages, and without having to add extra command-line options. It took several iterations and I considered relying on "git log --reverse" since v2 epochs are limited in size compared to v1. But I'm glad we won't have to rely on the expensive --reverse option and only internally reverse a few rare messages which have multiple Message-IDs. I'm actually satisfied with this because it doesn't involve adding new switches to workaround buggy behavior, since I considered (and rejected) a "--renumber" option. original RFC: https://public-inbox.org/meta/20191017112215.GA13175@dcvr/ I'm still testing this with various repos, but the linux-renesas-soc mirror from lore seems good when doing initial indexing with an old version of this and using --reindex with this patch, as all previously-missed messages are now indexed. Eric Wong (3): v2writable: set unindexed article number v2writable: improve "num_for" API and disambiguate v2writable: reindex handles 3-headered monsters TODO | 3 + lib/PublicInbox/OverIdx.pm | 14 ++ lib/PublicInbox/V2Writable.pm | 257 +++++++++++++++++++++++----------- t/v2reindex.t | 66 +++++++++ 4 files changed, 256 insertions(+), 84 deletions(-) Interdiff: diff --git a/lib/PublicInbox/OverIdx.pm b/lib/PublicInbox/OverIdx.pm index 1ff435d7..64277342 100644 --- a/lib/PublicInbox/OverIdx.pm +++ b/lib/PublicInbox/OverIdx.pm @@ -343,18 +343,18 @@ sub remove_oid { $nr; } -sub num_for_oid { +sub num_mid0_for_oid { my ($self, $oid, $mid) = @_; - my $num; + my ($num, $mid0); $self->begin_lazy; each_by_mid($self, $mid, ['ddd'], sub { my ($smsg) = @_; my $blob = $smsg->{blob}; return 1 if (!defined($blob) || $blob ne $oid); # continue; - $num = $smsg->{num}; + ($num, $mid0) = ($smsg->{num}, $smsg->{mid}); 0; # done }); - $num; + ($num, $mid0); } sub create_tables { diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm index 6e1ac5e3..7ece6b01 100644 --- a/lib/PublicInbox/V2Writable.pm +++ b/lib/PublicInbox/V2Writable.pm @@ -155,8 +155,7 @@ sub _add { # leaking FDs to it... $self->idx_init; - my $mid0; - my $num = num_for($self, $mime, \$mid0); + my ($num, $mid0) = v2_num_for($self, $mime); defined $num or return; # duplicate defined $mid0 or die "BUG: $mid0 undefined\n"; my $im = $self->importer; @@ -172,16 +171,15 @@ sub _add { $cmt; } -sub num_for { - my ($self, $mime, $mid0) = @_; +sub v2_num_for { + my ($self, $mime) = @_; my $mids = mids($mime->header_obj); if (@$mids) { my $mid = $mids->[0]; my $num = $self->{mm}->mid_insert($mid); if (defined $num) { # common case - $$mid0 = $mid; - return $num; - }; + return ($num, $mid); + } # crap, Message-ID is already known, hope somebody just resent: foreach my $m (@$mids) { @@ -190,7 +188,7 @@ sub num_for { # easy, don't store duplicates # note: do not add more diagnostic info here since # it gets noisy on public-inbox-watch restarts - return if $existing; + return () if $existing; } # AltId may pre-populate article numbers (e.g. X-Mail-Count @@ -201,8 +199,7 @@ sub num_for { my $num = $self->{mm}->num_for($mid); if (defined $num && !$self->{over}->get_art($num)) { - $$mid0 = $mid; - return $num; + return ($num, $mid); } } @@ -215,39 +212,38 @@ sub num_for { $num = $self->{mm}->mid_insert($m); if (defined $num) { warn "alternative <$m> for <$mid> found\n"; - $$mid0 = $m; - return $num; + return ($num, $m); } } } # none of the existing Message-IDs are good, generate a new one: - num_for_harder($self, $mime, $mid0); + v2_num_for_harder($self, $mime); } -sub num_for_harder { - my ($self, $mime, $mid0) = @_; +sub v2_num_for_harder { + my ($self, $mime) = @_; my $hdr = $mime->header_obj; my $dig = content_digest($mime); - $$mid0 = PublicInbox::Import::digest2mid($dig, $hdr); - my $num = $self->{mm}->mid_insert($$mid0); + my $mid0 = PublicInbox::Import::digest2mid($dig, $hdr); + my $num = $self->{mm}->mid_insert($mid0); unless (defined $num) { # it's hard to spoof the last Received: header my @recvd = $hdr->header_raw('Received'); $dig->add("Received: $_") foreach (@recvd); - $$mid0 = PublicInbox::Import::digest2mid($dig, $hdr); - $num = $self->{mm}->mid_insert($$mid0); + $mid0 = PublicInbox::Import::digest2mid($dig, $hdr); + $num = $self->{mm}->mid_insert($mid0); # fall back to a random Message-ID and give up determinism: until (defined($num)) { $dig->add(rand); - $$mid0 = PublicInbox::Import::digest2mid($dig, $hdr); - warn "using random Message-ID <$$mid0> as fallback\n"; - $num = $self->{mm}->mid_insert($$mid0); + $mid0 = PublicInbox::Import::digest2mid($dig, $hdr); + warn "using random Message-ID <$mid0> as fallback\n"; + $num = $self->{mm}->mid_insert($mid0); } } - PublicInbox::Import::append_mid($hdr, $$mid0); - $num; + PublicInbox::Import::append_mid($hdr, $mid0); + ($num, $mid0); } sub idx_shard { @@ -845,142 +841,157 @@ sub mark_deleted ($$$$) { } } -sub regen_art_num ($$$$$) { - my ($self, $sync, $git, $oid, $mids) = @_; - my $num = $sync->{regen}--; - my $mm = $self->{mm}; - if ($num > 0) { - foreach my $mid (reverse @$mids) { - if ($mm->mid_set($num, $mid) == 1) { - return ($num, $mid); - } - } +sub reindex_checkpoint ($$$) { + my ($self, $sync, $git) = @_; - # nope, not using the regen number because either we - # have or overlap Message-IDs with another message. - $sync->{regen}++; - $num = undef; - } - - # --reindex was broken when Message-IDs were reused with a - # message that already had multiple Message-IDs. This causes - # failures when using --reindex again. The least bad option is - # to show messages twice to NNTP clients, rather than lose/drop - # messages entirely. - # - # create a new article number? - for my $mid (@$mids) { - $num = $self->{mm}->mid_insert($mid); - return ($num, $mid) if defined $num; - } - - # swap article numbers with a previously regenerated (newer message) - # which has an overlapping Message-ID. The current $oid is older - # than $smsg->{blob} in git history since we regen walks backwards. - my $reindex_q = $sync->{reindex_q} //= []; - for my $mid (@$mids) { - my $n = $self->{mm}->num_for($mid); - next unless defined $n; - if (my $smsg = $self->{over}->get_art($n)) { - push @$reindex_q, $smsg->{blob}; - return ($n, $mid); - } + $git->cleanup; + $sync->{mm_tmp}->atfork_prepare; + $self->done; # release lock + + if (my $pr = $sync->{-opt}->{-progress}) { + my ($bn) = (split('/', $git->{git_dir}))[-1]; + $pr->("$bn ".sprintf($sync->{-regen_fmt}, $sync->{nr})); } - warn "W: ran out of article numbers on $oid\n"; - (undef, undef); + + # allow -watch or -mda to write... + $self->idx_init; # reacquire lock + $sync->{mm_tmp}->atfork_parent; } -sub reindex_oid ($$$$) { +# only for a few odd messages with multiple Message-IDs +sub reindex_oid_m ($$$$) { my ($self, $sync, $git, $oid) = @_; - my $len; + my ($num, $mid0, $len); my $msgref = $git->cat_file($oid, \$len); my $mime = PublicInbox::MIME->new($$msgref); my $mids = mids($mime->header_obj); my $cid = content_id($mime); + die "BUG: reindex_oid_m called for <=1 mids" if scalar(@$mids) <= 1; - # get the NNTP article number we used before, which gets deleted - # from sync->{mm_tmp}; - my $mid0; - my $num = -1; - my $del = 0; - my $reindex = $sync->{reindex}; - if ($reindex) { - # see if it's already in the overview DB, but keep in mind - # --reindex may be used blindly w/o overview DB. - foreach my $mid (@$mids) { - $del += delete($sync->{D}->{"$mid\0$cid"}) ? 1 : 0; - my $n = $self->{over}->num_for_oid($oid, $mid); - if (defined $n) { - ($num, $mid0) = ($n, $mid); - $self->{mm}->mid_set($num, $mid0); - last; # yay! reused - } + for my $mid (reverse @$mids) { + delete($sync->{D}->{"$mid\0$cid"}) and + die "BUG: reindex_oid should handle <$mid> delete"; + } + for my $mid (reverse @$mids) { + ($num, $mid0) = $self->{over}->num_mid0_for_oid($oid, $mid); + last if defined $num; + } + unless (defined($num)) { + for my $mid (reverse @$mids) { + # is this a number we got before? + $num = $sync->{mm_tmp}->num_for($mid); + next unless defined $num; + $mid0 = $mid; + last; } } - - # not reindexing, or reindexing with broken/incomplete overview DB: - if (!defined($mid0)) { - # highest number wins for unseen messages - foreach my $mid (@$mids) { - $del += delete($sync->{D}->{"$mid\0$cid"}) ? 1 : 0; - my $n = $sync->{mm_tmp}->num_for($mid); - if (defined $n && $n > $num) { - ($num, $mid0) = ($n, $mid); - $self->{mm}->mid_set($num, $mid0); + if (defined($num)) { + $sync->{mm_tmp}->num_delete($num); + } else { + $num = $sync->{regen}--; + if ($num <= 0) { + # fixup bugs in old mirrors on reindex + for my $mid (reverse @$mids) { + $num = $self->{mm}->mid_insert($mid); + next unless defined $num; + $mid0 = $mid; + last; + } + if (defined $mid0) { + if ($sync->{reindex}) { + warn "reindex added #$num <$mid0>\n"; + } + } else { + warn "E: cannot find article #\n"; + return; + } + } else { # $num > 0, use the new article number + for my $mid (reverse @$mids) { + $self->{mm}->mid_set($num, $mid) == 1 or next; + $mid0 = $mid; + last; + } + unless (defined $mid0) { + warn "E: cannot regen #$num\n"; + return; } } } + $sync->{nr}++; + if (do_idx($self, $msgref, $mime, $len, $num, $oid, $mid0)) { + reindex_checkpoint($self, $sync, $git); + } +} - if (!defined($mid0) && !$del) { - ($num, $mid0) = regen_art_num($self, $sync, $git, $oid, $mids); - if (!defined($mid0)) { - my $mm = $self->{mm}; - my $id = '<' . join('> <', @$mids) . '>'; - warn "Message-ID $id unusable for $num\n"; - foreach my $mid (@$mids) { - defined(my $n = $mm->num_for($mid)) or next; - warn "#$n previously mapped for <$mid>\n"; - } - } +sub check_unindexed ($$$) { + my ($self, $num, $mid0) = @_; + my $unindexed = $self->{unindexed} // {}; + my $n = delete($unindexed->{$mid0}); + defined $n or return; + if ($n != $num) { + die "BUG: unindexed $n != $num <$mid0>\n"; + } else { + $self->{mm}->mid_set($num, $mid0); } +} - if (!defined($mid0) || $del) { - if (!defined($mid0) && $del) { # expected for deletes - $num = $sync->{regen}--; - if (!$reindex) { - ($num <= 0) and - die "BUG: ran out of article numbers\n"; +sub reindex_oid ($$$$) { + my ($self, $sync, $git, $oid) = @_; + my ($num, $mid0, $len); + my $msgref = $git->cat_file($oid, \$len); + return if $len == 0; # purged + my $mime = PublicInbox::MIME->new($$msgref); + my $mids = mids($mime->header_obj); + my $cid = content_id($mime); + + if (scalar(@$mids) == 0) { + warn "E: $oid has no Message-ID, skipping\n"; + return; + } elsif (scalar(@$mids) == 1) { + my $mid = $mids->[0]; + + # was the file previously marked as deleted?, skip if so + if (delete($sync->{D}->{"$mid\0$cid"})) { + if (!$sync->{reindex}) { + $num = $sync->{regen}--; $self->{mm}->num_highwater($num); } - return + return; } - my $id = '<' . join('> <', @$mids) . '>'; - defined($mid0) or - warn "Skipping $id, no article number found\n"; - if ($del && defined($mid0)) { - warn "$id was deleted $del " . - "time(s) but mapped to article #$num\n"; + # is this a number we got before? + $num = $sync->{mm_tmp}->num_for($mid); + if (defined $num) { + $mid0 = $mid; + check_unindexed($self, $num, $mid0); + } else { + $num = $sync->{regen}--; + die "BUG: ran out of article numbers\n" if $num <= 0; + if ($self->{mm}->mid_set($num, $mid) != 1) { + warn "E: unable to assign $num => <$mid>\n"; + return; + } + $mid0 = $mid; + } + } else { # multiple MIDs are a weird case: + my $del = 0; + for (@$mids) { + $del += delete($sync->{D}->{"$_\0$cid"}) // 0; + } + if ($del) { + unindex_oid_remote($self, $oid, $_) for @$mids; + # do not delete from {mm_tmp}, since another + # single-MID message may use it. + } else { # handle them at the end: + push @{$sync->{multi_mid} //= []}, $oid; } return; - } $sync->{mm_tmp}->mid_delete($mid0) or die "failed to delete <$mid0> for article #$num\n"; $sync->{nr}++; if (do_idx($self, $msgref, $mime, $len, $num, $oid, $mid0)) { - $git->cleanup; - $sync->{mm_tmp}->atfork_prepare; - $self->done; # release lock - - if (my $pr = $sync->{-opt}->{-progress}) { - my ($bn) = (split('/', $git->{git_dir}))[-1]; - $pr->("$bn ".sprintf($sync->{-regen_fmt}, $sync->{nr})); - } - - # allow -watch or -mda to write... - $self->idx_init; # reacquire lock - $sync->{mm_tmp}->atfork_parent; + reindex_checkpoint($self, $sync, $git); } } @@ -1112,8 +1123,9 @@ sub unindex_oid_remote ($$$) { $self->{over}->remove_oid($oid, $mid); } -sub unindex_oid ($$$) { - my ($self, $git, $oid) = @_; +sub unindex_oid ($$$;$) { + my ($self, $git, $oid, $unindexed) = @_; + my $mm = $self->{mm}; my $msgref = $git->cat_file($oid); my $mime = PublicInbox::MIME->new($msgref); my $mids = mids($mime->header_obj); @@ -1133,8 +1145,11 @@ sub unindex_oid ($$$) { join(',',sort keys %gone), "\n"; } foreach my $num (keys %gone) { - $self->{unindexed}->{$_}++; - $self->{mm}->num_delete($num); + if ($unindexed) { + my $mid0 = $mm->mid_for($num); + $unindexed->{$mid0} = $num; + } + $mm->num_delete($num); } unindex_oid_remote($self, $oid, $mid); } @@ -1143,20 +1158,21 @@ sub unindex_oid ($$$) { my $x40 = qr/[a-f0-9]{40}/; sub unindex ($$$$) { my ($self, $sync, $git, $unindex_range) = @_; - my $un = $self->{unindexed} ||= {}; # num => removal count - my $before = scalar keys %$un; + my $unindexed = $self->{unindexed} ||= {}; # $mid0 => $num + my $before = scalar keys %$unindexed; + # order does not matter, here: my @cmd = qw(log --raw -r --no-notes --no-color --no-abbrev --no-renames); my $fh = $self->{reindex_pipe} = $git->popen(@cmd, $unindex_range); while (<$fh>) { /\A:\d{6} 100644 $x40 ($x40) [AM]\tm$/o or next; - unindex_oid($self, $git, $1); + unindex_oid($self, $git, $1, $unindexed); } delete $self->{reindex_pipe}; $fh = undef; return unless $sync->{-opt}->{prune}; - my $after = scalar keys %$un; + my $after = scalar keys %$unindexed; return if $before == $after; # ensure any blob can not longer be accessed via dumb HTTP @@ -1208,16 +1224,6 @@ sub index_epoch ($$$) { } elsif (/\A:\d{6} 100644 $x40 ($x40) [AM]\td$/o) { mark_deleted($self, $sync, $git, $1); } - - while (my $reindex_q = delete $sync->{reindex_q}) { - my $all = $self->{-inbox}->git; - for my $oid (@$reindex_q) { - $self->{current_info} = "$i.git $oid"; - warn "reindexing to shuffle article numbers\n"; - reindex_oid($self, $sync, $all, $oid); - } - $all->cleanup; - } } $fh = undef; delete $self->{reindex_pipe}; @@ -1259,10 +1265,22 @@ sub index_sync { # unindex is required for leftovers if "deletes" affect messages # in a previous fetch+index window: + my $git; if (my @leftovers = values %{delete $sync->{D}}) { - my $git = $self->{-inbox}->git; - unindex_oid($self, $git, $_) for @leftovers; - $git->cleanup; + $git = $self->{-inbox}->git; + for my $oid (@leftovers) { + $self->{current_info} = "leftover $oid"; + unindex_oid($self, $git, $oid); + } + } + if (my $multi_mid = delete $sync->{multi_mid}) { + $git //= $self->{-inbox}->git; + + while (defined(my $oid = pop(@$multi_mid))) { + $self->{current_info} = "multi_mid $oid"; + reindex_oid_m($self, $sync, $git, $oid); + } + $git->cleanup if $git; } $self->done; diff --git a/t/v2reindex.t b/t/v2reindex.t index 5de067c6..52711f8f 100644 --- a/t/v2reindex.t +++ b/t/v2reindex.t @@ -491,9 +491,10 @@ EOF remove_tree($xap); eval { $im->index_sync() }; is($@, '', 'no error from initial indexing'); + is_deeply(\@warn, [], 'no warnings from initial index'); eval { $im->index_sync({reindex=>1}) }; is($@, '', 'no error from reindexing after reused Message-ID (x3)'); - is_deeply(\@warn, [], 'no warnings'); + is_deeply(\@warn, [], 'no warnings on reindex'); } done_testing();