unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / Atom feed
* Adding config cache to notmuch_database_t breaks python-cffi test
@ 2020-12-20 19:20 David Bremner
  2020-12-20 19:20 ` [PATCH] lib: cache configuration information from database David Bremner
  2020-12-20 21:03 ` Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
  0 siblings, 2 replies; 5+ messages in thread
From: David Bremner @ 2020-12-20 19:20 UTC (permalink / raw)
  To: notmuch; +Cc: flub

I hope Floris (or someone) can tell me what is going on here, I don't
understand the memory management in the CFFI bindings very well.

As part of my attempt to remodel config handling, I've added a cache
for the configuration information stored in the database. This doesn't
seem to break any other tests, and valgrind doesn't spot any memory
errors.

Here's the output from pytest

______________________________ TestIter.test_del _______________________________

self = <test_config.TestIter object at 0x7f21c50a0cd0>
db = Database(path=/tmp/pytest-of-bremner/pytest-50/test_del0, mode=Mode.READ_WRITE)

    def test_del(self, db):
        db.config['spam'] = 'ham'
        assert db.config.get('spam') == 'ham'
        del db.config['spam']
>       assert db.config.get('spam') is None
E       AssertionError: assert 'ham' is None
E        +  where 'ham' = <bound method Mapping.get of <notmuch2._config.ConfigMapping object at 0x7f21c50a0af0>>('spam')
E        +    where <bound method Mapping.get of <notmuch2._config.ConfigMapping object at 0x7f21c50a0af0>> = <notmuch2._config.ConfigMapping object at 0x7f21c50a0af0>.get
E        +      where <notmuch2._config.ConfigMapping object at 0x7f21c50a0af0> = Database(path=/tmp/pytest-of-bremner/pytest-50/test_del0, mode=Mode.READ_WRITE).config

tests/test_config.py:56: AssertionError
=========================== short test summary info ============================
FAILED tests/test_config.py::TestIter::test_del - AssertionError: assert 'ham...
==================== 1 failed, 155 passed in 29.25 seconds =====================

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] lib: cache configuration information from database
  2020-12-20 19:20 Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
@ 2020-12-20 19:20 ` David Bremner
  2020-12-20 21:03 ` Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
  1 sibling, 0 replies; 5+ messages in thread
From: David Bremner @ 2020-12-20 19:20 UTC (permalink / raw)
  To: notmuch; +Cc: flub, David Bremner

The main goal is to allow configuration information to be temporarily
overridden by a separate config file. That will require further
changes not in this commit.

The performance impact is unclear, and will depend on the balance
between number of queries and number of distinct metadata items read
on the first call to n_d_get_config.
---
 lib/config.cc             | 45 +++++++++++++++++++++++++++++++++------
 lib/database-private.h    |  3 +++
 lib/notmuch-private.h     |  3 +++
 lib/open.cc               |  2 +-
 test/T562-lib-database.sh |  2 +-
 5 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/lib/config.cc b/lib/config.cc
index 0b760dbc..7f97e528 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -84,17 +84,25 @@ notmuch_database_get_config (notmuch_database_t *notmuch,
 			     const char *key,
 			     char **value)
 {
-    std::string strval;
+    const char* stored_val;
     notmuch_status_t status;
 
+    if (! notmuch->config) {
+	if ((status = _notmuch_config_load_from_database (notmuch)))
+	    return status;
+    }
+
     if (! value)
 	return NOTMUCH_STATUS_NULL_POINTER;
 
-    status = _metadata_value (notmuch, key, strval);
-    if (status)
-	return status;
-
-    *value = strdup (strval.c_str ());
+    stored_val = _notmuch_string_map_get (notmuch->config, key);
+    if (! stored_val) {
+	/* XXX in principle this API should be fixed so empty string
+	 * is distinguished from not found */
+	*value = strdup("");
+    } else {
+	*value = strdup (stored_val);
+    }
 
     return NOTMUCH_STATUS_SUCCESS;
 }
@@ -201,3 +209,28 @@ notmuch_config_list_destroy (notmuch_config_list_t *list)
 {
     talloc_free (list);
 }
+
+notmuch_status_t
+_notmuch_config_load_from_database (notmuch_database_t *notmuch)
+{
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    notmuch_config_list_t *list;
+
+    if (notmuch->config == NULL)
+	notmuch->config = _notmuch_string_map_create (notmuch);
+
+    if (unlikely(notmuch->config == NULL))
+	return NOTMUCH_STATUS_OUT_OF_MEMORY;
+
+    status = notmuch_database_get_config_list (notmuch, "", &list);
+    if (status)
+	return status;
+
+    for (; notmuch_config_list_valid (list); notmuch_config_list_move_to_next (list)) {
+	_notmuch_string_map_append (notmuch->config,
+				    notmuch_config_list_key (list),
+				    notmuch_config_list_value (list));
+    }
+
+    return status;
+}
diff --git a/lib/database-private.h b/lib/database-private.h
index c9bc712b..d83cf0d0 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -228,6 +228,9 @@ struct _notmuch_database {
      * here, but at least they are small */
     notmuch_string_map_t *user_prefix;
     notmuch_string_map_t *user_header;
+
+    /* Cached and possibly overridden configuration */
+    notmuch_string_map_t *config;
 };
 
 /* Prior to database version 3, features were implied by the database
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 57ec7f72..0f26b371 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -699,6 +699,9 @@ struct _notmuch_indexopts {
 
 #define EMPTY_STRING(s) ((s)[0] == '\0')
 
+/* config.cc */
+notmuch_status_t
+_notmuch_config_load_from_database (notmuch_database_t * db);
 NOTMUCH_END_DECLS
 
 #ifdef __cplusplus
diff --git a/lib/open.cc b/lib/open.cc
index 1b17e63a..c04e6f77 100644
--- a/lib/open.cc
+++ b/lib/open.cc
@@ -90,7 +90,7 @@ notmuch_database_open_verbose (const char *path,
     notmuch->exception_reported = false;
     notmuch->status_string = NULL;
     notmuch->path = talloc_strdup (notmuch, path);
-
+    notmuch->config = NULL;
     strip_trailing (notmuch->path, '/');
 
     notmuch->writable_xapian_db = NULL;
diff --git a/test/T562-lib-database.sh b/test/T562-lib-database.sh
index dd4f2566..37ca16ab 100755
--- a/test/T562-lib-database.sh
+++ b/test/T562-lib-database.sh
@@ -350,7 +350,7 @@ cat <<EOF > EXPECTED
 == stdout ==
 1
 == stderr ==
-Error: A Xapian exception occurred getting metadata: Database has been closed
+A Xapian exception occurred getting metadata iterator: Database has been closed.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-- 
2.29.2

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Adding config cache to notmuch_database_t breaks python-cffi test
  2020-12-20 19:20 Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
  2020-12-20 19:20 ` [PATCH] lib: cache configuration information from database David Bremner
@ 2020-12-20 21:03 ` David Bremner
  2020-12-20 23:22   ` David Bremner
  1 sibling, 1 reply; 5+ messages in thread
From: David Bremner @ 2020-12-20 21:03 UTC (permalink / raw)
  To: notmuch; +Cc: flub

David Bremner <david@tethera.net> writes:

> I hope Floris (or someone) can tell me what is going on here, I don't
> understand the memory management in the CFFI bindings very well.
>
> As part of my attempt to remodel config handling, I've added a cache
> for the configuration information stored in the database. This doesn't
> seem to break any other tests, and valgrind doesn't spot any memory
> errors.
>
> Here's the output from pytest

Oh, I bet I know what it is. I need to update the cache when calling
notmuch_database_set_config. It's just a surprise that nothing in our
test suite does a read after write for configuration information in the
same database session.

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Adding config cache to notmuch_database_t breaks python-cffi test
  2020-12-20 21:03 ` Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
@ 2020-12-20 23:22   ` David Bremner
  2020-12-24 16:05     ` Floris Bruynooghe
  0 siblings, 1 reply; 5+ messages in thread
From: David Bremner @ 2020-12-20 23:22 UTC (permalink / raw)
  To: notmuch; +Cc: flub

David Bremner <david@tethera.net> writes:

> David Bremner <david@tethera.net> writes:
>
>> I hope Floris (or someone) can tell me what is going on here, I don't
>> understand the memory management in the CFFI bindings very well.
>>
>> As part of my attempt to remodel config handling, I've added a cache
>> for the configuration information stored in the database. This doesn't
>> seem to break any other tests, and valgrind doesn't spot any memory
>> errors.
>>
>> Here's the output from pytest
>
> Oh, I bet I know what it is. I need to update the cache when calling
> notmuch_database_set_config. It's just a surprise that nothing in our
> test suite does a read after write for configuration information in the
> same database session.
>

Yep, that was it, sorry for the noise. I guess I kindof panicked at not
understanding what was going on in the bindings tests.

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Adding config cache to notmuch_database_t breaks python-cffi test
  2020-12-20 23:22   ` David Bremner
@ 2020-12-24 16:05     ` Floris Bruynooghe
  0 siblings, 0 replies; 5+ messages in thread
From: Floris Bruynooghe @ 2020-12-24 16:05 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sun 20 Dec 2020 at 19:22 -0400, David Bremner wrote:
Hi David,

I'm just catching up on some mailing lists...

> David Bremner <david@tethera.net> writes:
>
>> David Bremner <david@tethera.net> writes:
>>
>>> I hope Floris (or someone) can tell me what is going on here, I don't
>>> understand the memory management in the CFFI bindings very well.
>>>
>>> As part of my attempt to remodel config handling, I've added a cache
>>> for the configuration information stored in the database. This doesn't
>>> seem to break any other tests, and valgrind doesn't spot any memory
>>> errors.
>>>
>>> Here's the output from pytest
>>
>> Oh, I bet I know what it is. I need to update the cache when calling
>> notmuch_database_set_config. It's just a surprise that nothing in our
>> test suite does a read after write for configuration information in the
>> same database session.
>>
>
> Yep, that was it, sorry for the noise. I guess I kindof panicked at not
> understanding what was going on in the bindings tests.

Yes, this test is checking deletion actually works.  Glad the python
tests helped catch an issue.  :)


Cheers,
Floris

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-12-24 16:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-20 19:20 Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
2020-12-20 19:20 ` [PATCH] lib: cache configuration information from database David Bremner
2020-12-20 21:03 ` Adding config cache to notmuch_database_t breaks python-cffi test David Bremner
2020-12-20 23:22   ` David Bremner
2020-12-24 16:05     ` Floris Bruynooghe

unofficial mirror of notmuch@notmuchmail.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git