Eric Wong <e...@yhbt.net> wrote:
> Expanding threads via over.sqlite3 for mbox.gz downloads without
> Xapian effectively collapsing on the THREADID column leads to
> repeated messages getting downloaded.
>
> To avoid that situation, use a "has_threadid" Xapian metadata
> flag that's only set on --reindex (and brand new Xapian DBs).
>
> This allows admins to upgrade WWW or do --reindex in any order;
> without worrying about users eating up bandwidth and CPU cycles.
> ---
> lib/PublicInbox/Mbox.pm | 2 +-
> lib/PublicInbox/Search.pm | 10 ++++++++-
> lib/PublicInbox/SearchIdx.pm | 16 ++++++++++++--
> lib/PublicInbox/SearchView.pm | 21 ++++++++++++-------
> lib/PublicInbox/V2Writable.pm | 11 ++++++++++
> t/init.t | 1 +
> t/psgi_search.t | 39 +++++++++++++++++++++++++++++++++--
> 7 files changed, 87 insertions(+), 13 deletions(-)
>
> diff --git a/lib/PublicInbox/Mbox.pm b/lib/PublicInbox/Mbox.pm
> index c9b11c21..0223bead 100644
> --- a/lib/PublicInbox/Mbox.pm
> +++ b/lib/PublicInbox/Mbox.pm
> @@ -262,7 +262,7 @@ sub mbox_all {
> $ctx->{ids} = $srch->mset_to_artnums($mset);
> require PublicInbox::MboxGz;
> my $fn;
> - if ($q->{t}) {
> + if ($q->{t} && $srch->has_threadid) {
> $fn = 'results-thread-'.$q_string;
> PublicInbox::MboxGz::mbox_gz($ctx, \&results_thread_cb, $fn);
> } else {
> diff --git a/lib/PublicInbox/Search.pm b/lib/PublicInbox/Search.pm
> index bc820b64..01bbe73d 100644
> --- a/lib/PublicInbox/Search.pm
> +++ b/lib/PublicInbox/Search.pm
> @@ -311,6 +311,12 @@ sub _do_enquire {
> retry_reopen($self, \&_enquire_once, [ $self, $query, $opts ]);
> }
>
> +# returns true if all docs have the THREADID value
> +sub has_threadid ($) {
> + my ($self) = @_;
> + (xdb($self)->get_metadata('has_threadid') // '') eq '1';
> +}
> +
> sub _enquire_once { # retry_reopen callback
> my ($self, $query, $opts) = @{$_[0]};
> my $xdb = xdb($self);
> @@ -328,7 +334,9 @@ sub _enquire_once { # retry_reopen callback
> }
>
> # `mairix -t / --threads' or JMAP collapseThreads
> - $enquire->set_collapse_key(THREADID) if $opts->{thread};
> + if ($opts->{thread} && has_threadid($self)) {
> + $enquire->set_collapse_key(THREADID);
> + }
>
> my $offset = $opts->{offset} || 0;
> my $limit = $opts->{limit} || 50;
> diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
> index baa6f41a..ade55756 100644
> --- a/lib/PublicInbox/SearchIdx.pm
> +++ b/lib/PublicInbox/SearchIdx.pm
> @@ -131,6 +131,7 @@ sub idx_acquire {
> ($is_shard && need_xapian($self)))) {
> File::Path::mkpath($dir);
> nodatacow_dir($dir);
> + $self->{-set_has_threadid_once} = 1;
> }
> }
> return unless defined $flag;
> @@ -590,9 +591,17 @@ sub v1_checkpoint ($$;$) {
>
> $self->{mm}->{dbh}->commit;
> if ($newest && need_xapian($self)) {
> - my $cur = $self->{xdb}->get_metadata('last_commit');
> + my $xdb = $self->{xdb};
> + my $cur = $xdb->get_metadata('last_commit');
> if (need_update($self, $cur, $newest)) {
> - $self->{xdb}->set_metadata('last_commit', $newest);
> + $xdb->set_metadata('last_commit', $newest);
> + }
> +
> + # let SearchView know a full --reindex was done so it can
> + # generate ->has_threadid-dependent links
> + if ($sync->{reindex} && !ref($sync->{reindex})) {
> + my $n = $xdb->get_metadata('has_threadid');
> + $xdb->set_metadata('has_threadid', '1') if $n ne '1';
> }
> }
>
> @@ -816,6 +825,9 @@ sub set_metadata_once {
> return if $self->{shard}; # only continue if undef or 0, not >0
> my $xdb = $self->{xdb};
>
> + if (delete($self->{-set_has_threadid_once})) {
> + $xdb->set_metadata('has_threadid', '1');
> + }
> if (delete($self->{-set_indexlevel_once})) {
> my $level = $xdb->get_metadata('indexlevel');
> if (!$level || $level ne 'medium') {
> diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm
> index dd69564a..76428dfb 100644
> --- a/lib/PublicInbox/SearchView.pm
> +++ b/lib/PublicInbox/SearchView.pm
> @@ -188,13 +188,20 @@ sub search_nav_top {
> $rv .= qq{<a\nhref="?$s">summary</a>|<b>nested</b>};
> }
> my $A = $q->qs_html(x => 'A', r => undef);
> - $rv .= qq{|<a\nhref="?$A">Atom feed</a>]} .
> - qq{\n\t\t\tdownload mbox.gz: } .
> - # we set name=z w/o using it since it seems required for
> - # lynx (but works fine for w3m).
> - qq{<input\ntype=submit\nname=z\nvalue="results only"/>|} .
> - qq{<input\ntype=submit\nname=x\nvalue="full threads"/>} .
> - qq{</pre></form><pre>};
> + $rv .= qq{|<a\nhref="?$A">Atom feed</a>]};
> + if ($ctx->{-inbox}->search->has_threadid) {
> + $rv .= qq{\n\t\t\tdownload mbox.gz: } .
> + # we set name=z w/o using it since it seems required for
> + # lynx (but works fine for w3m).
> + qq{<input\ntype=submit\nname=z\n} .
> + q{value="results only"/>} .
> + qq{|<input\ntype=submit\nname=x\n} .
> + q{value="full threads"/>};
> + } else { # BOFH needs to --reindex
> + $rv .= qq{\n\t\t\t\t\t\tdownload: } .
> + qq{<input\ntype=submit\nname=z\nvalue="mbox.gz"/>}
> + }
> + $rv .= qq{</pre></form><pre>};
> }
>
> sub search_nav_bot {
> diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
> index 0a91a132..a32fdcf3 100644
> --- a/lib/PublicInbox/V2Writable.pm
> +++ b/lib/PublicInbox/V2Writable.pm
> @@ -1337,6 +1337,17 @@ sub index_sync {
> xapian_only($self, $opt, $sync, $art_beg);
> }
>
> + # --reindex on the command-line
> + if ($opt->{reindex} && !ref($opt->{reindex}) && $idxlevel ne 'basic') {
> + local $self->{parallel} = 0;
> + my $s0 = PublicInbox::SearchIdx->new($self->{ibx}, 0, 0);
> + if (my $xdb = $s0->idx_acquire) {
> + my $n = $xdb->get_metadata('has_threadid');
> + $xdb->set_metadata('has_threadid', 1) if $n ne '1';
> + }
> + $s0->idx_release;
> + }
Assigning {parallel} there is useless; I originally used
SearchIdxShard instead of SearchIdx but avoided the *Shard
bit to avoid an extra idx_acquire.
Anyways, we actually need to flock the inbox.lock there to
avoid contending with -index/-watch/-mda etc.
So I'll squash this in:
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index a32fdcf3..b0148dba 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -1339,13 +1339,14 @@ sub index_sync {
# --reindex on the command-line
if ($opt->{reindex} && !ref($opt->{reindex}) && $idxlevel ne 'basic') {
- local $self->{parallel} = 0;
+ $self->lock_acquire;
my $s0 = PublicInbox::SearchIdx->new($self->{ibx}, 0, 0);
if (my $xdb = $s0->idx_acquire) {
my $n = $xdb->get_metadata('has_threadid');
$xdb->set_metadata('has_threadid', 1) if $n ne '1';
}
$s0->idx_release;
+ $self->lock_release;
}
# reindex does not pick up new changes, so we rerun w/o it:
--
unsubscribe: one-click, see List-Unsubscribe header
archive: https://public-inbox.org/meta/
