[PATCH 0/9] convert: more fixes and tests

[PATCH 0/9] convert: more fixes and tests

[Eric Wong]

Nothing major, but SMP systems should be better utilized
now that -convert can take advantage of parallel shard
indexing in Xapian.  The switches for convert are also
now documented in the manpage.

Eric Wong (9):
  t/multi-mid.t: extra test for -convert highwater mark
  v2writable: nproc_shards: subtract 1 from given value
  v2writable: do not clobber {shards} or {parallel} if unset
  convert: remove unused variables capturing :from
  searchidxshard: rely on autoflush instead of ->flush
  convert: shift @ARGV explicitly
  convert: fix --no-index switch
  doc: -convert: document switches
  v2writable: more ways to detect online CPU count

 Documentation/public-inbox-convert.pod | 41 ++++++++++++++++-
 MANIFEST                               |  1 +
 lib/PublicInbox/SearchIdxShard.pm      |  4 +-
 lib/PublicInbox/V2Writable.pm          | 38 +++++++++++-----
 script/public-inbox-convert            | 16 ++-----
 t/convert-compact.t                    |  3 ++
 t/multi-mid.t                          | 61 ++++++++++++++++++++++++++
 t/xcpdb-reshard.t                      |  2 +-
 8 files changed, 137 insertions(+), 29 deletions(-)
 create mode 100644 t/multi-mid.t

[PATCH 1/9] t/multi-mid.t: extra test for -convert highwater mark

[Eric Wong]

This is derived from a real-world test case where I encounterd
multiple Message-IDs in a v1 inbox causing regen problems.

Fixes: eea47b676127bcdb ("convert: preserve highwater mark from v1 msgmap")
---
 MANIFEST      |  1 +
 t/multi-mid.t | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 t/multi-mid.t

diff --git a/MANIFEST b/MANIFEST
index 3736c777..5eb5d53a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -250,6 +250,7 @@ t/mime.t
 t/msg_iter.t
 t/msgmap.t
 t/msgtime.t
+t/multi-mid.t
 t/nntp.t
 t/nntpd-tls.t
 t/nntpd.t
diff --git a/t/multi-mid.t b/t/multi-mid.t
new file mode 100644
index 00000000..0d283a0d
--- /dev/null
+++ b/t/multi-mid.t
@@ -0,0 +1,61 @@
+# Copyright (C) 2020 all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use Test::More;
+use PublicInbox::MIME;
+use PublicInbox::TestCommon;
+use PublicInbox::InboxWritable;
+use PublicInbox::SearchIdx;
+require_git(2.6);
+require_mods(qw(DBD::SQLite));
+
+my $addr = 'test@example.com';
+my $bad = PublicInbox::MIME->new(<<EOF);
+Message-ID: <a\@example.com>
+Message-ID: <b\@example.com>
+From: a\@example.com
+To: $addr
+Date: Fri, 02 Oct 1993 00:00:00 +0000
+Subject: bad
+
+EOF
+
+my $good = PublicInbox::MIME->new(<<EOF);
+Message-ID: <b\@example.com>
+Date: Fri, 02 Oct 1993 00:00:00 +0000
+From: b\@example.com
+To: $addr
+Subject: good
+
+EOF
+
+for my $order ([$bad, $good], [$good, $bad]) {
+	my $before;
+	my ($tmpdir, $for_destroy) = tmpdir();
+	my $ibx = PublicInbox::InboxWritable->new({
+		inboxdir => "$tmpdir/v1",
+		name => 'test-v1',
+		indexlevel => 'basic',
+		-primary_address => $addr,
+	}, my $creat_opt = {});
+	if ('setup v1 inbox') {
+		my $im = $ibx->importer(0);
+		ok($im->add($_), 'added '.$_->header('Subject')) for @$order;
+		$im->done;
+		my $s = PublicInbox::SearchIdx->new($ibx, 1);
+		$s->index_sync;
+		$before = [ $ibx->mm->minmax ];
+		$ibx->cleanup;
+	}
+	my $rdr = { 1 => \(my $out = ''), 2 => \(my $err = '') };
+	my $cmd = [ '-convert', $ibx->{inboxdir}, "$tmpdir/v2" ];
+	ok(run_script($cmd, undef, $rdr),
+		'convert to v2');
+	$err =~ s!\AW: $tmpdir/v1 not configured[^\n]+\n!!s;
+	is($err, '', 'no errors or warnings from -convert');
+	$ibx->{version} = 2;
+	$ibx->{inboxdir} = "$tmpdir/v2";
+	is_deeply([$ibx->mm->minmax], $before,
+		'min, max article numbers unchanged');
+}
+
+done_testing();

[PATCH 2/9] v2writable: nproc_shards: subtract 1 from given value

[Eric Wong]

This is to be consistent with the `nproc(1)' code path.  It also
quiets down a warning from Admin when "-j $JOBS" is specified,
since the master process (which distributes work to shards and
handles OverIdx and Msgmap) is considered a job on its own.
---
 lib/PublicInbox/V2Writable.pm | 9 ++-------
 t/xcpdb-reshard.t             | 2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 8ad71b54..37d27302 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -34,13 +34,8 @@ our $NPROC_MAX_DEFAULT = 4;
 
 sub nproc_shards ($) {
 	my ($creat_opt) = @_;
-	if (ref($creat_opt) eq 'HASH') {
-		if (defined(my $n = $creat_opt->{nproc})) {
-			return $n
-		}
-	}
-
-	my $n = $ENV{NPROC};
+	my $n = $creat_opt->{nproc} if ref($creat_opt) eq 'HASH';
+	$n //= $ENV{NPROC};
 	if (!$n) {
 		chomp($n = `nproc 2>/dev/null`);
 		# assume 2 cores if GNU nproc(1) is not available
diff --git a/t/xcpdb-reshard.t b/t/xcpdb-reshard.t
index 9f0034f1..5fce5ead 100644
--- a/t/xcpdb-reshard.t
+++ b/t/xcpdb-reshard.t
@@ -40,7 +40,7 @@ for my $i (1..$ndoc) {
 }
 $im->done;
 my @shards = grep(m!/\d+\z!, glob("$ibx->{inboxdir}/xap*/*"));
-is(scalar(@shards), $nproc, 'got expected shards');
+is(scalar(@shards), $nproc - 1, 'got expected shards');
 my $orig = $ibx->over->query_xover(1, $ndoc);
 my %nums = map {; "$_->{num}" => 1 } @$orig;
 

[PATCH 3/9] v2writable: do not clobber {shards} or {parallel} if unset

[Eric Wong]

The $jobs parameter in `public-inbox-convert' is passed to
V2Writable->init_inbox as `undef' by default, causing
parallelization to be disabled.

Instead, leave the underlying {parallel} flag untouched if
$shards is undef and do not clobber the default shard count.

This allows us to take advantage of multicore systems when
running public-inbox-convert with no command-line switches.
---
 lib/PublicInbox/V2Writable.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 37d27302..72d8d5af 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -107,8 +107,11 @@ sub new {
 
 # public (for now?)
 sub init_inbox {
-	my ($self, $parallel, $skip_epoch) = @_;
-	$self->{parallel} = $parallel;
+	my ($self, $shards, $skip_epoch) = @_;
+	if (defined $shards) {
+		$self->{parallel} = 0 if $shards == 0;
+		$self->{shards} = $shards if $shards > 0;
+	}
 	$self->idx_init;
 	my $epoch_max = -1;
 	git_dir_latest($self, \$epoch_max);

[PATCH 4/9] convert: remove unused variables capturing :from

[Eric Wong]

Looking at git history, they were never used.
---
 script/public-inbox-convert | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index 8ac111a2..acecf3d5 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -87,7 +87,6 @@ $clone may not be valid after migrating to v2, not copying
 	}
 });
 my $state = '';
-my ($prev, $from);
 my $head = $old->{ref_head} || 'HEAD';
 my ($rd, $pid) = $old->git->popen(qw(fast-export --use-done-feature), $head);
 $v2w->idx_init;
@@ -132,11 +131,6 @@ while (<$rd>) {
 			$last = 'd';
 			next;
 		}
-		if (m{^from (:[0-9]+)}) {
-			$prev = $from;
-			$from = $1;
-			# no next
-		}
 	}
 	last if $_ eq "done\n";
 	$w->print($_) or $im->wfail;

[PATCH 5/9] searchidxshard: rely on autoflush instead of ->flush

[Eric Wong]

It reduces the number of ops and simplifies the code, slightly.
Add a missing IO::Handle import while we're at it, to be
explicit about which methods we use.
---
 lib/PublicInbox/SearchIdxShard.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/SearchIdxShard.pm b/lib/PublicInbox/SearchIdxShard.pm
index b22e51dc..0522ecea 100644
--- a/lib/PublicInbox/SearchIdxShard.pm
+++ b/lib/PublicInbox/SearchIdxShard.pm
@@ -7,6 +7,7 @@ package PublicInbox::SearchIdxShard;
 use strict;
 use warnings;
 use base qw(PublicInbox::SearchIdx);
+use IO::Handle (); # autoflush
 
 sub new {
 	my ($class, $v2writable, $shard) = @_;
@@ -24,6 +25,7 @@ sub spawn_worker {
 	pipe($r, $w) or die "pipe failed: $!\n";
 	binmode $r, ':raw';
 	binmode $w, ':raw';
+	$w->autoflush(1);
 	my $pid = fork;
 	defined $pid or die "fork failed: $!\n";
 	if ($pid == 0) {
@@ -83,7 +85,6 @@ sub index_raw {
 	if (my $w = $self->{w}) {
 		print $w "$bytes $artnum $oid $mid0\n", $$msgref or die
 			"failed to write shard $!\n";
-		$w->flush or die "failed to flush: $!\n";
 	} else {
 		$$msgref = undef;
 		$self->begin_txn_lazy;
@@ -100,7 +101,6 @@ sub remote_barrier {
 	my ($self) = @_;
 	if (my $w = $self->{w}) {
 		print $w "barrier\n" or die "failed to print: $!";
-		$w->flush or die "failed to flush: $!";
 	} else {
 		$self->commit_txn_lazy;
 	}

[PATCH 6/9] convert: shift @ARGV explicitly

[Eric Wong]

Relying on implicit "@_" for shift fails with
TestCommon::_run_sub iff GetOptions modifies @ARGV.
---
 script/public-inbox-convert | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index acecf3d5..281313e0 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -19,8 +19,8 @@ my %opts = (
 	'--index!' => \$index,
 );
 GetOptions(%opts) or die "bad command-line args\n$usage";
-my $old_dir = shift or die $usage;
-my $new_dir = shift or die $usage;
+my $old_dir = shift(@ARGV) or die $usage;
+my $new_dir = shift(@ARGV) or die $usage;
 die "$new_dir exists\n" if -d $new_dir;
 die "$old_dir not a directory\n" unless -d $old_dir;
 my $config = eval { PublicInbox::Config->new };

[PATCH 7/9] convert: fix --no-index switch

[Eric Wong]

The (currently undocumented) "--no-index" flag did not trigger
the V2Writable->done call necessary to make the import
successful.

Fixes: eea47b676127bcdb ("convert: preserve highwater mark from v1 msgmap")
---
 script/public-inbox-convert | 6 ++----
 t/convert-compact.t         | 3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index 281313e0..68b0b31a 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -151,7 +151,5 @@ if (my $mm = $old->mm) {
 	chop(my $cmt = $epoch0->qx(qw(rev-parse --verify), $head));
 	$v2w->last_epoch_commit(0, $cmt);
 }
-if ($index) {
-	$v2w->index_sync({reindex => 1});
-	$v2w->done;
-}
+$v2w->index_sync({reindex => 1}) if $index;
+$v2w->done;
diff --git a/t/convert-compact.t b/t/convert-compact.t
index fc46083b..f56d7817 100644
--- a/t/convert-compact.t
+++ b/t/convert-compact.t
@@ -76,6 +76,9 @@ my $hwm = do {
 };
 ok(defined($hwm) && $hwm > 0, "highwater mark set #$hwm");
 
+$cmd = [ '-convert', '--no-index', $ibx->{inboxdir}, "$tmpdir/no-index" ];
+ok(run_script($cmd, undef, $rdr), 'convert --no-index works');
+
 $cmd = [ '-convert', $ibx->{inboxdir}, "$tmpdir/v2" ];
 ok(run_script($cmd, undef, $rdr), 'convert works');
 @xdir = glob("$tmpdir/v2/xap*/*");

[PATCH 8/9] doc: -convert: document switches

[Eric Wong]

These switches have always been there, but were not
documented until now.
---
 Documentation/public-inbox-convert.pod | 41 ++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/Documentation/public-inbox-convert.pod b/Documentation/public-inbox-convert.pod
index 4c790dee..417e5e4e 100644
--- a/Documentation/public-inbox-convert.pod
+++ b/Documentation/public-inbox-convert.pod
@@ -4,7 +4,7 @@ public-inbox-convert - convert v1 inboxes to v2
 
 =head1 SYNOPSIS
 
-	public-inbox-convert OLD_DIR NEW_DIR
+	public-inbox-convert [OPTIONS] OLD_DIR NEW_DIR
 
 =head1 DESCRIPTION
 
@@ -14,6 +14,27 @@ and users are expected to update the "inboxdir" path in
 L<public-inbox-config(5)> to point to the path of NEW_DIR
 once they are satisfied with the conversion.
 
+=head1 OPTIONS
+
+=over
+
+=item --no-index
+
+Disables Xapian and overview DB indexing on the new repository.
+By default, public-inbox-convert creates a new index in the v2
+repository and indexes all existing messages, a lengthy
+operation for large inboxes.
+
+=item -j JOBS, --jobs=JOBS
+
+Control the number of indexing jobs and Xapian shards of the v2
+inbox.  By default, this is the detected CPU count but capped
+at 4 due to various bottlenecks.  The number of Xapian shards
+will be 1 less than the JOBS value, since there is a single
+process which distributes work to the Xapian shards.
+
+=back
+
 =head1 ENVIRONMENT
 
 =over 8
@@ -27,6 +48,21 @@ See L<public-inbox-config(5)>
 
 =head1 UPGRADING
 
+Editing "~/.public-inbox/config" (or whatever C<PI_CONFIG> is
+set to) will be required to start using the new directory.
+
+=head1 BUGS
+
+Writes from L<public-inbox-mda(1)> or L<git-fetch(1)> to the v1
+inbox which occur after the start of the conversion will not
+be picked up in the v2 inbox.
+
+Users of L<public-inbox-watch(1)> do not have to worry about
+this.  They only need to update the config file to point to the
+v2 inbox, send C<SIGHUP> public-inbox-watch process to reload
+the config file, and then C<SIGUSR1> to rescan existing
+Maildirs.
+
 =head1 CONTACT
 
 Feedback welcome via plain-text mail to L<mailto:meta@public-inbox.org>
@@ -42,4 +78,5 @@ License: AGPL-3.0+ L<https://www.gnu.org/licenses/agpl-3.0.txt>
 
 =head1 SEE ALSO
 
-L<public-inbox-init(1)>, L<public-inbox-index(1)>
+L<public-inbox-init(1)>, L<public-inbox-index(1)>, L<public-inbox-config(5)>,
+L<public-inbox-v1-format(5)>, L<public-inbox-v2-format(5)>

[PATCH 9/9] v2writable: more ways to detect online CPU count

[Eric Wong]

OpenBSD and FreeBSD support `getconf NPROCESSORS_ONLN` (no
leading underscore).  They may also have GNU nproc installed as
"gnproc".

We may also encounter Linux systems w/o GNU coreutils, but able
to use `getconf _NPROCESSORS_ONLN` (with leading underscore).
---
 lib/PublicInbox/V2Writable.pm | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 72d8d5af..82843241 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -7,6 +7,7 @@ package PublicInbox::V2Writable;
 use strict;
 use warnings;
 use base qw(PublicInbox::Lock);
+use 5.010_001;
 use PublicInbox::SearchIdxShard;
 use PublicInbox::MIME;
 use PublicInbox::Git;
@@ -32,14 +33,29 @@ my $PACKING_FACTOR = 0.4;
 # to increase Xapian shards
 our $NPROC_MAX_DEFAULT = 4;
 
+sub detect_nproc () {
+	for my $nproc (qw(nproc gnproc)) { # GNU coreutils nproc
+		`$nproc 2>/dev/null` =~ /^(\d+)$/ and return $1;
+	}
+
+	# getconf(1) is POSIX, but *NPROCESSORS* vars are not
+	for (qw(_NPROCESSORS_ONLN NPROCESSORS_ONLN)) {
+		`getconf $_ 2>/dev/null` =~ /^(\d+)$/ and return $1;
+	}
+
+	# should we bother with `sysctl hw.ncpu`?  Those only give
+	# us total processor count, not online processor count.
+	undef
+}
+
 sub nproc_shards ($) {
 	my ($creat_opt) = @_;
 	my $n = $creat_opt->{nproc} if ref($creat_opt) eq 'HASH';
 	$n //= $ENV{NPROC};
 	if (!$n) {
-		chomp($n = `nproc 2>/dev/null`);
-		# assume 2 cores if GNU nproc(1) is not available
-		$n = 2 if !$n;
+		# assume 2 cores if not detectable or zero
+		state $NPROC_DETECTED = detect_nproc() || 2;
+		$n = $NPROC_DETECTED;
 		$n = $NPROC_MAX_DEFAULT if $n > $NPROC_MAX_DEFAULT;
 	}