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 4FE151F9F4 for ; Thu, 23 Sep 2021 05:53:04 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 3/3] xcpdb: avoid race when shards are added Date: Thu, 23 Sep 2021 05:53:03 +0000 Message-Id: <20210923055303.6409-4-e@80x24.org> In-Reply-To: <20210923055303.6409-1-e@80x24.org> References: <20210923055303.6409-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: It's possible for the rename() sequence to cause read-only daemons using ->xdb_shards_flat to load an incomplete set of contiguous shards and get invalid docids for search results. With this change, we favor the case where search is momentarily unavailable rather than giving wrong results during the small window where Xapcmd->commit_changes runs. --- lib/PublicInbox/Xapcmd.pm | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/PublicInbox/Xapcmd.pm b/lib/PublicInbox/Xapcmd.pm index daef896c..44e0f8e5 100644 --- a/lib/PublicInbox/Xapcmd.pm +++ b/lib/PublicInbox/Xapcmd.pm @@ -20,16 +20,23 @@ sub commit_changes ($$$$) { my $reshard = $opt->{reshard}; $SIG{INT} or die 'BUG: $SIG{INT} not handled'; - my @old_shard; - my $over_chg; - my $mode; - - while (my ($old, $newdir) = each %$tmp) { + my (@old_shard, $over_chg); + + # Sort shards highest-to-lowest, since ->xdb_shards_flat + # determines the number of shards to load based on the max; + # and we'd rather xdb_shards_flat to momentarily fail rather + # than load out-of-date shards + my @order = sort { + my ($x) = ($a =~ m!/([0-9]+)/*\z!); + my ($y) = ($b =~ m!/([0-9]+)/*\z!); + ($y // -1) <=> ($x // -1) # we may have non-shards + } keys %$tmp; + + my ($dname) = ($order[0] =~ m!(.*/)[^/]+/*\z!); + my $mode = (stat($dname))[2]; + for my $old (@order) { next if $old eq ''; # no invalid paths - $mode //= do { - my ($dname) = ($old =~ m!(.*/)[^/]+/*\z!); - (stat($dname))[2]; - }; + my $newdir = $tmp->{$old}; my $have_old = -e $old; if (!$have_old && !defined($opt->{reshard})) { die "failed to stat($old): $!"; @@ -57,11 +64,7 @@ sub commit_changes ($$$$) { die "rename $old => $new/old: $!\n"; } rename($new, $old) or die "rename $new => $old: $!\n"; - if ($have_old) { - my $prev = "$old/old"; - remove_tree($prev) or - die "failed to remove $prev: $!\n"; - } + push @old_shard, "$old/old" if $have_old; } # trigger ->check_inodes in read-only daemons