[PATCH 0/3] write-related fixes

[PATCH 0/3] write-related fixes

[Eric Wong]

Combined with 807abf67e14d35270ed0957590cde0ed1eb68635
(lei/store: auto-commit for long-running imports, 2024-11-15),
PATCH 3/3 should fix the long-running `lei import' problem
documented in https://public-inbox.org/meta/20240814001645.395576-1-e@80x24.org/

I'm still working on other stuff related to improving write
reliability of other things, but I could also be dead within an
hour :<

Eric Wong (3):
  v2writable: use DS and import now() sub
  treewide: warn on SQLite `PRAGMA optimize' failure
  v2writable: done: force synchronous awaitpid

 lib/PublicInbox/LeiMailSync.pm |  5 +++--
 lib/PublicInbox/Msgmap.pm      |  9 +++++++++
 lib/PublicInbox/OverIdx.pm     |  1 +
 lib/PublicInbox/SearchIdx.pm   |  4 +---
 lib/PublicInbox/V2Writable.pm  | 11 +++++------
 5 files changed, 19 insertions(+), 11 deletions(-)

[PATCH 1/3] v2writable: use DS and import now() sub

[Eric Wong]

There'll be more uses of this function, so import DS
to avoid surprises even though it's pulled in by other
modules, already.
---
 lib/PublicInbox/V2Writable.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 19bb66ad..f7057359 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -21,6 +21,7 @@ use PublicInbox::Msgmap;
 use PublicInbox::Spawn qw(spawn popen_rd run_die);
 use PublicInbox::Search;
 use PublicInbox::SearchIdx qw(log2stack is_ancestor check_size is_bad_blob);
+use PublicInbox::DS qw(now);
 use IO::Handle; # ->autoflush
 use POSIX ();
 use Carp qw(confess);
@@ -723,7 +724,7 @@ sub reindex_checkpoint ($$) {
 	# allow -watch or -mda to write...
 	$self->idx_init($sync->{-opt}); # reacquire lock
 	if (my $intvl = $sync->{check_intvl}) { # eidx
-		$sync->{next_check} = PublicInbox::DS::now() + $intvl;
+		$sync->{next_check} = now + $intvl;
 	}
 	$mm_tmp->atfork_parent if $mm_tmp;
 }

[PATCH 2/3] treewide: warn on SQLite `PRAGMA optimize' failure

[Eric Wong]

While `PRAGMA optimize' isn't a strict requirement for proper
functionality anywhere, displaying the failure can help detect
bigger problems in the future in case of failing hardware.
---
 lib/PublicInbox/LeiMailSync.pm | 5 +++--
 lib/PublicInbox/Msgmap.pm      | 9 +++++++++
 lib/PublicInbox/OverIdx.pm     | 1 +
 lib/PublicInbox/SearchIdx.pm   | 4 +---
 lib/PublicInbox/V2Writable.pm  | 7 ++-----
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm
index c498421c..d0f6d7b4 100644
--- a/lib/PublicInbox/LeiMailSync.pm
+++ b/lib/PublicInbox/LeiMailSync.pm
@@ -49,8 +49,9 @@ sub lms_write_prepare { ($_[0]->{dbh} //= dbh_new($_[0])); $_[0] }
 sub lms_pause {
 	my ($self) = @_;
 	$self->{fmap} = {};
-	my $dbh = delete $self->{dbh};
-	eval { $dbh->do('PRAGMA optimize') } if $dbh;
+	my $dbh = delete $self->{dbh} // return;
+	eval { $dbh->do('PRAGMA optimize') };
+	warn 'W: optimize ', $dbh->sqlite_db_filename, ': ', $@ if $@;
 }
 
 sub create_tables {
diff --git a/lib/PublicInbox/Msgmap.pm b/lib/PublicInbox/Msgmap.pm
index c4bc766d..3101fd7d 100644
--- a/lib/PublicInbox/Msgmap.pm
+++ b/lib/PublicInbox/Msgmap.pm
@@ -275,4 +275,13 @@ sub check_inodes {
 	$self->{dbh} //= PublicInbox::Over::dbh_new($self, !$rw);
 }
 
+sub mm_commit {
+	my ($self) = @_;
+	my $dbh = $self->{dbh} // return;
+	$dbh->commit;
+	eval { $dbh->do('PRAGMA optimize') };
+	warn 'W: optimize ', $dbh->sqlite_db_filename, ': ', $@ if $@;
+	$dbh;
+}
+
 1;
diff --git a/lib/PublicInbox/OverIdx.pm b/lib/PublicInbox/OverIdx.pm
index 4f8533f7..879ae045 100644
--- a/lib/PublicInbox/OverIdx.pm
+++ b/lib/PublicInbox/OverIdx.pm
@@ -439,6 +439,7 @@ sub commit_lazy {
 	delete $self->{txn} or return;
 	$self->{dbh}->commit;
 	eval { $self->{dbh}->do('PRAGMA optimize') };
+	warn 'W: optimize ', $self->{dbh}->sqlite_db_filename, ': ', $@ if $@;
 }
 
 sub begin_lazy {
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 7829c7d4..48ba806a 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -860,9 +860,7 @@ sub v1_checkpoint ($$;$) {
 		}
 	}
 	${$sync->{max}} = $self->{batch_bytes};
-
-	$self->{mm}->{dbh}->commit;
-	eval { $self->{mm}->{dbh}->do('PRAGMA optimize') };
+	$self->{mm}->mm_commit;
 	my $xdb = $self->{xdb};
 	if ($newest && $xdb) {
 		my $cur = $xdb->get_metadata('last_commit');
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index f7057359..721fbb4a 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -522,11 +522,8 @@ sub checkpoint ($;$) {
 	$self->{im}->barrier if $self->{im};
 	my $shards = $self->{idx_shards};
 	if ($shards) {
-		my $dbh = $self->{mm}->{dbh} if $self->{mm};
-
-		# SQLite msgmap data is second in importance
-		$dbh->commit if $dbh;
-		eval { $dbh->do('PRAGMA optimize') };
+		# SQLite msgmap is second in importance (not in eidx)
+		my $dbh = $self->{mm} ? $self->{mm}->mm_commit : undef;
 
 		# SQLite overview is third
 		$self->{oidx}->commit_lazy;

[PATCH 3/3] v2writable: done: force synchronous awaitpid

[Eric Wong]

We need to shut down shards synchronously to reliably release
the inbox write lock when inside the DS event loop (as the
lei/store subprocess is, unlike most v2writable users).

While more testing is being conducted, this seems to fix
long-running `lei import' failures to lei/store.  It seems like
a good idea anyways to ensure exit status of shard workers are
correct before returning from ->done.
---
 lib/PublicInbox/V2Writable.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 721fbb4a..9f686bfa 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -579,6 +579,7 @@ sub active { !!$_[0]->{im} }
 # public
 sub done {
 	my ($self) = @_;
+	local $PublicInbox::DS::in_loop; # sync awaitpid in shard_close
 	my $err = '';
 	if (my $im = delete $self->{im}) {
 		eval { $im->done }; # PublicInbox::Import::done

Re: [PATCH 3/3] v2writable: done: force synchronous awaitpid

[Eric Wong]

Pushed as commit 99fc3d76968aacaea55f5b98b5b5a4d2314bc97c
with the following commit message:

    v2writable: done: force synchronous awaitpid

    We need to shut down shards synchronously to reliably release
    the inbox write lock when inside the DS event loop (as the
    lei/store subprocess is, unlike most v2writable users).

    This seems to fix long-running `lei import' failures to
    lei/store after repeated tests.  It is a good idea anyways to
    ensure exit status of shard workers are correct before returning
    from ->done.