Quoting David Bremner (2025-07-06 15:34:12)
> Anton Khirnov <anton@khirnov.net> writes:
> 
> >  	if (status)
> >  	    *status = NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND;
> >  	return NULL;
> > +    } catch (const Xapian::DatabaseModifiedError &error) {
> > +	if (status)
> > +	    *status = NOTMUCH_PRIVATE_STATUS_OPERATION_INVALIDATED;
> > +	return NULL;
> 
> I wonder if this status code (name) should refer directly to iterators.
> Do you envision using it in non-iterator contexts?

I don't have specific plans, but in principle that exception could be
raised from any Xapian operation, and it's probably better to use this
status code in such a case rather than a generic
NOTMUCH_STATUS_XAPIAN_EXCEPTION (since it's a part of normal Xapian
operation and not an "actual" error).

> > +    if (! messages->is_of_list_type)
> > +	valid = _notmuch_mset_messages_valid (messages);
> > +    else
> > +	valid = messages->iterator != NULL;
> > +
> > +    return valid ?
> > +	NOTMUCH_STATUS_SUCCESS : NOTMUCH_STATUS_ITERATOR_EXHAUSTED;
> 
> Is this better than two returns just above?

It avoids duplicating the "success : exhausted" expression, so IMO more
readable. But I can change it if you prefer.

> 
> >  notmuch_bool_t
> >  notmuch_messages_valid (notmuch_messages_t *messages)
> >  {
> [snip]
> > +    return notmuch_messages_status(messages) == NOTMUCH_STATUS_SUCCESS;
> >  }
> 
> I think it would be better to gradually migrate existing code to the new
> function, since this code is eminently inlinable. That probably means
> deprecating notmuch_messges_valid

Is breaking existing user code really worth the small size/performance
benefit of inlining?

> 
> >  bool
> > @@ -142,7 +157,7 @@ notmuch_messages_move_to_next (notmuch_messages_t *messages)
> >  	return;
> >      }
> >  
> > -    if (messages->iterator == NULL)
> > +    if (! notmuch_messages_valid(messages))
> >  	return;
> >  
> >      messages->iterator = messages->iterator->next;
> > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> > index 367e23e6..f4702dd8 100644
> > --- a/lib/notmuch-private.h
> > +++ b/lib/notmuch-private.h
> 
> >  notmuch_message_list_t *
> > diff --git a/lib/notmuch.h b/lib/notmuch.h
> > index 937fa24e..c9923da2 100644
> > --- a/lib/notmuch.h
> > +++ b/lib/notmuch.h
> > @@ -232,6 +232,20 @@ typedef enum {
> > +    /**
> > +     * The iterator being examined has been exhausted and contains no more
> > +     * items.
> > +     */
> > +    NOTMUCH_STATUS_ITERATOR_EXHAUSTED,
> > +    /**
> > +     * An operation that was being performed on the database has been
> > +     * invalidated while in progress, and must be re-executed.
> > +     *
> > +     * This will typically happen while iterating over query results and the
> > +     * underlying Xapian database is modified by another process so that the
> > +     * currently open version cannot be read anymore.
> > +     */
> > +    NOTMUCH_STATUS_OPERATION_INVALIDATED,
> 
> Note that you should update notmuch_status_to_string when adding new
> status values

Done locally.

I also forgot to add NOTMUCH_PRIVATE_STATUS_ITERATOR_EXHAUSTED, which is
now also fixed.

> 
> > @@ -1516,10 +1530,35 @@ notmuch_thread_destroy (notmuch_thread_t *thread);
> >   *
> >   * See the documentation of notmuch_query_search_messages for example
> 
> >   * code showing how to iterate over a notmuch_messages_t object.
> > + *
> > + * Note that an iterator may become invalid either due to getting exhausted or
> > + * due to a runtime error. Use notmuch_messages_status to distinguish
> > + * between those cases.
> >   */
> 
> I guess we need to decide if we really expect people to use both
> functions in new code.

I'd prefer to avoid breaking user code, as I don't expect a lot of it
will actually implement query re-execution, and having to change
existing callers for no functionality benefit feels user-hostile.

> 
> >  notmuch_bool_t
> >  notmuch_messages_valid (notmuch_messages_t *messages);
> >  
> > +/**
> > + *
> > + * See the documentation of notmuch_query_search_messages for example
> > + * code showing how to iterate over a notmuch_messages_t object.
> 
> It would be good if the referred to code actually used
> notmuch_messages_status, to encourage good error handling. The idiom "!
> status" as success is pretty entrenched, so I think
> notmuch_messages_valid is not needed there.
> 
>  query = notmuch_query_create (database, query_string);
> 
>  if (notmuch_query_search_messages (query, &messages) != NOTMUCH_STATUS_SUCCESS)
>      return EXIT_FAILURE;
> 
>  for (;
>       ! notmuch_messages_status (messages);
>       notmuch_messages_move_to_next (messages))
>  {
>      message = notmuch_messages_get (messages);
>      ....
>      notmuch_message_destroy (message);
>  }
> 
>  if (notmuch_messages_status (messages) != NOTMUCH_STATUS_ITERATOR_EXHAUSTED)
>      return EXIT_FAILURE;
> 
>  notmuch_query_destroy (query);

Sure, applied locally. No references to notmuch_messages_valid() remain
in notmuch.h, except for the function prototype itself.

> > +    if (message == NULL) {
> > +	if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
> > +	    INTERNAL_ERROR ("a messages iterator contains a non-existent
> > document ID.\n");
> 
> As you know (because you sent this patch series!) we really want to
> avoid the library aborting. Are we confident that this cannot arise from
> concurrent access (I guess via Xapian snapshotting mechanisms?). Perhaps
> a brief comment about _why_ this should never happen would be a good idea.

Well, I'm just keeping existing behaviour here. AFAIU it would mean that
the database is inconsistent, which could happen e.g. through a bug in
notmuch (or Xapian), or good old database corruption. I guess the code
could be changed to signal an error status, but that's not really in
scope of this patchset.
> 
> >      }
> > +    status = notmuch_messages_status(messages_ro);
> >  
> > -    printf("SUCCESS\n");
> > +    if (status == NOTMUCH_STATUS_ITERATOR_EXHAUSTED)
> > +	printf("SUCCESS\n");
> > +    else
> > +	printf("status: %u\n", status);
> 
> It might be my mistake in rebasing, but this test fails for me with
> "status: 26". Assuming you update notmuch_status_to_string, that can be
> made less cryptic, and I'll double check

I also need to change the test to use notmuch_status_to_string(), after
which it prints
 status: Operation invalidated due to concurrent database modification

> > +test_subtest_known_broken
> 
> It should not in fact be known broken after this commit, right?

I'm reluctant to mark it as fixed (harcoding
NOTMUCH_STATUS_OPERATION_INVALIDATED as the correct behaviour), because
future Xapian changes might make iteration succeed, which is what the
caller actually wants to happen (and thus the properly correct result
of this test).

What I could do is re-execute the query on
NOTMUCH_STATUS_OPERATION_INVALIDATED, and see if the second try exhausts
the iterator. What do you think?

-- 
Anton Khirnov
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org