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-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 DA0C91F55B; Thu, 11 Jun 2020 00:57:53 +0000 (UTC) Date: Thu, 11 Jun 2020 00:57:53 +0000 From: Eric Wong To: Kyle Meyer Cc: meta@public-inbox.org Subject: [PATCH] nntpd+imapd: detect replaced over.sqlite3 Message-ID: <20200611005753.GA30101@dcvr> References: <87r1unyjj0.fsf@kyleam.com> <20200609232407.GA27163@dcvr> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200609232407.GA27163@dcvr> List-Id: Eric Wong wrote: > Will have to think about how -compact needs to interact with > read-only daemons w.r.t. over.sqlite3, and whether moving > over.sqlite3 can be avoided. It probably can't be avoided for v1 or I would've done it. And I've been thinking about making public-inbox-compact support VACUUM and possibly other expensive optimizations, too. Anyways, the following patch (which goes on top of the imapd stuff I posted yesterday) should fix it. -------8<------- Subject: [PATCH] nntpd+imapd: detect replaced over.sqlite3 For v1 inboxes (and possibly v2 in the future, for VACUUM), public-inbox-compact replaces over.sqlite3 with a new file. This currently doesn't need an extra inotify watch descriptor (or FD for kevent) at the moment, so it can coexist nicely for systems w/o IO::KQueue or Linux::Inotify2. --- lib/PublicInbox/Inbox.pm | 14 +++++++++++--- lib/PublicInbox/NNTP.pm | 4 +++- lib/PublicInbox/NNTPD.pm | 12 ++++++++++-- lib/PublicInbox/Over.pm | 36 +++++++++++++++++++++++++++++------- t/nntpd.t | 35 +++++++++++++++++++++++++++++++---- 5 files changed, 84 insertions(+), 17 deletions(-) diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm index b2b0b56fdd3..7d5e048363f 100644 --- a/lib/PublicInbox/Inbox.pm +++ b/lib/PublicInbox/Inbox.pm @@ -31,7 +31,7 @@ sub cleanup_task () { for my $ibx (values %$CLEANUP) { my $again; if ($have_devel_peek) { - foreach my $f (qw(mm search over)) { + foreach my $f (qw(mm search)) { # we bump refcnt by assigning tmp, here: my $tmp = $ibx->{$f} or next; next if Devel::Peek::SvREFCNT($tmp) > 2; @@ -45,9 +45,9 @@ sub cleanup_task () { $again = 1 if $git->cleanup; } } + check_inodes($ibx); if ($have_devel_peek) { - $again ||= !!($ibx->{over} || $ibx->{mm} || - $ibx->{search}); + $again ||= !!($ibx->{mm} || $ibx->{search}); } $next->{"$ibx"} = $ibx if $again; } @@ -407,9 +407,17 @@ sub unsubscribe_unlock { delete $self->{unlock_subs}->{$ident}; } +sub check_inodes ($) { + my ($self) = @_; + for (qw(over)) { # TODO: search, mm + $self->{$_}->check_inodes if $self->{$_}; + } +} + # called by inotify sub on_unlock { my ($self) = @_; + check_inodes($self); my $subs = $self->{unlock_subs} or return; for (values %$subs) { eval { $_->on_inbox_unlock($self) }; diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index ac13c7df8ce..bffd773cf9e 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -334,7 +334,9 @@ sub cmd_newnews ($$$$;$$) { sub cmd_group ($$) { my ($self, $group) = @_; my $no_such = '411 no such news group'; - my $ng = $self->{nntpd}->{groups}->{$group} or return $no_such; + my $nntpd = $self->{nntpd}; + my $ng = $nntpd->{groups}->{$group} or return $no_such; + $nntpd->idler_start; $self->{ng} = $ng; my ($min, $max) = $ng->mm->minmax; diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm index ed5cf7cc8c0..6b762d8923b 100644 --- a/lib/PublicInbox/NNTPD.pm +++ b/lib/PublicInbox/NNTPD.pm @@ -8,6 +8,7 @@ use strict; use warnings; use Sys::Hostname; use PublicInbox::Config; +use PublicInbox::InboxIdle; sub new { my ($class) = @_; @@ -24,15 +25,17 @@ sub new { err => \*STDERR, out => \*STDOUT, grouplist => [], + pi_config => $pi_config, servername => $name, greet => \"201 $name ready - post via email\r\n", # accept_tls => { SSL_server => 1, ..., SSL_reuse_ctx => ... } + # idler => PublicInbox::InboxIdle }, $class; } sub refresh_groups { - my ($self, $pi_config) = @_; - $pi_config //= PublicInbox::Config->new; + my ($self, $sig) = @_; + my $pi_config = $sig ? PublicInbox::Config->new : $self->{pi_config}; my $new = {}; my @list; $pi_config->each_inbox(sub { @@ -59,8 +62,13 @@ sub refresh_groups { }); @list = sort { $a->{newsgroup} cmp $b->{newsgroup} } @list; $self->{grouplist} = \@list; + $self->{pi_config} = $pi_config; # this will destroy old groups that got deleted %{$self->{groups}} = %$new; } +sub idler_start { + $_[0]->{idler} //= PublicInbox::InboxIdle->new($_[0]->{pi_config}); +} + 1; diff --git a/lib/PublicInbox/Over.pm b/lib/PublicInbox/Over.pm index e32104f0cf8..74c8fb86270 100644 --- a/lib/PublicInbox/Over.pm +++ b/lib/PublicInbox/Over.pm @@ -19,13 +19,23 @@ sub dbh_new { if ($rw && !-f $f) { # SQLite defaults mode to 0644, we want 0666 open my $fh, '+>>', $f or die "failed to open $f: $!"; } - my $dbh = DBI->connect("dbi:SQLite:dbname=$f",'','', { - AutoCommit => 1, - RaiseError => 1, - PrintError => 0, - ReadOnly => !$rw, - sqlite_use_immediate_transaction => 1, - }); + my (@st, $st, $dbh); + my $tries = 0; + do { + @st = stat($f) or die "failed to stat $f: $!"; + $st = pack('dd', $st[0], $st[1]); # 0: dev, 1: inode + $dbh = DBI->connect("dbi:SQLite:dbname=$f",'','', { + AutoCommit => 1, + RaiseError => 1, + PrintError => 0, + ReadOnly => !$rw, + sqlite_use_immediate_transaction => 1, + }); + $self->{st} = $st; + @st = stat($f) or die "failed to stat $f: $!"; + $st = pack('dd', $st[0], $st[1]); + } while ($st ne $self->{st} && $tries++ < 3); + warn "W: $f: .st_dev, .st_ino unstable\n" if $st ne $self->{st}; $dbh->{sqlite_unicode} = 1; $dbh; } @@ -259,4 +269,16 @@ SELECT MAX(num) FROM over WHERE num > 0 ($exists, $uidnext, $sth->fetchrow_array // 0); } +sub check_inodes { + my ($self) = @_; + if (my @st = stat($self->{filename})) { # did st_dev, st_ino change? + my $st = pack('dd', $st[0], $st[1]); + + # don't actually reopen, just let {dbh} be recreated later + delete($self->{dbh}) if ($st ne ($self->{st} // $st)); + } else { + warn "W: stat $self->{filename}: $!\n"; + } +} + 1; diff --git a/t/nntpd.t b/t/nntpd.t index d2f31323115..d6042d18758 100644 --- a/t/nntpd.t +++ b/t/nntpd.t @@ -14,8 +14,11 @@ use Net::NNTP; use Sys::Hostname; # FIXME: make easier to test both versions -my $version = $ENV{PI_TEST_VERSION} || 2; +my $version = $ENV{PI_TEST_VERSION} || 1; require_git('2.6') if $version == 2; +my $lsof = which('lsof'); +my $fast_idle = eval { require Linux::Inotify2; 1 } // + eval { require IO::KQueue; 1 }; my ($tmpdir, $for_destroy) = tmpdir(); my $home = "$tmpdir/pi-home"; @@ -302,13 +305,13 @@ Date: Fri, 02 Oct 1993 00:00:00 +0000 is($rdr, waitpid($rdr, 0), 'reader done'); is($? >> 8, 0, 'no errors'); } + my $noerr = { 2 => \(my $null) }; SKIP: { if ($INC{'Search/Xapian.pm'} && ($ENV{TEST_RUN_MODE}//2)) { skip 'Search/Xapian.pm pre-loaded (by t/run.perl?)', 1; } - my $lsof = which('lsof') or skip 'lsof missing', 1; - my $rdr = { 2 => \(my $null) }; - my @of = xqx([$lsof, '-p', $td->{pid}], undef, $rdr); + $lsof or skip 'lsof missing', 1; + my @of = xqx([$lsof, '-p', $td->{pid}], undef, $noerr); skip('lsof broken', 1) if (!scalar(@of) || $?); my @xap = grep m!Search/Xapian!, @of; is_deeply(\@xap, [], 'Xapian not loaded in nntpd'); @@ -330,6 +333,30 @@ Date: Fri, 02 Oct 1993 00:00:00 +0000 is(scalar @r, 1, 'only one response line'); } + # -compact requires Xapian + SKIP: { + require_mods('Search::Xapian', 2); + which('xapian-compact') or skip 'xapian-compact missing', 2; + is(xsys(qw(git config), "--file=$home/.public-inbox/config", + "publicinbox.$group.indexlevel", 'medium'), + 0, 'upgraded indexlevel'); + my $ex = eml_load('t/data/0001.patch'); + is($n->article($ex->header('Message-ID')), undef, + 'article did not exist'); + $im->add($ex); + $im->done; + ok(run_script([qw(-index --reindex -c), $ibx->{inboxdir}], + undef, $noerr), '-compacted'); + select(undef, undef, undef, $fast_idle ? 0.1 : 2.1); + $art = $n->article($ex->header('Message-ID')); + ok($art, 'new article retrieved after compact'); + $lsof or skip 'lsof missing', 1; + ($^O =~ /\A(?:linux)\z/) or + skip "lsof /(deleted)/ check untested on $^O", 1; + my @of = xqx([$lsof, '-p', $td->{pid}], undef, $noerr); + is(scalar(grep(/\(deleted\)/, @of)), 0, 'no deleted files'); + }; + $n = $s = undef; $td->join; is($?, 0, 'no error in exited process');