--- /dev/null
+Return-Path: <bgamari.foss@gmail.com>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 1B438431FCF\r
+ for <notmuch@notmuchmail.org>; Tue, 3 Sep 2013 07:33:05 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id ePfhwJl0547z for <notmuch@notmuchmail.org>;\r
+ Tue, 3 Sep 2013 07:32:57 -0700 (PDT)\r
+Received: from mail-yh0-f43.google.com (mail-yh0-f43.google.com\r
+ [209.85.213.43]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 57F36431FC9\r
+ for <notmuch@notmuchmail.org>; Tue, 3 Sep 2013 07:32:57 -0700 (PDT)\r
+Received: by mail-yh0-f43.google.com with SMTP id b6so252912yha.16\r
+ for <notmuch@notmuchmail.org>; Tue, 03 Sep 2013 07:32:55 -0700 (PDT)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type;\r
+ bh=2E8XN5K7x3rTAx2m8ZbxK5k6dhSny6A5R2xEgdnF2Ec=;\r
+ b=VXGXdzGe4jrD7PNiUktk9IUIGLmefAO1ytt9BzfpWE/Zc8uYFyBJyCfTk69w+UryVo\r
+ MoBaoREJz+vqdZ3lShiADQWSUrOukIFi6jaueuMLozr9XT5LGlbAPHolaRMwvVqfzga4\r
+ uRXSZtM04ZuBKGRU9bSIliRdPwfe515cURLpPV2Cji5tMueKA/xU4FbxfQHm1La5qZyX\r
+ s7yThVjXZQxKj1VCGjyS0XgXQnkH+m6puShcB2IGUnJDsHi8Wx1lpm8o7Yc5/juQQ41K\r
+ oU5dnUYgQavgWoeOaKg/QZduMkh+MulaA3MXpsI5A6ZBOluw/gKWnjAoCSkN/+zTQ0Ms\r
+ MJ9w==\r
+X-Received: by 10.236.9.9 with SMTP id 9mr340884yhs.97.1378218775599;\r
+ Tue, 03 Sep 2013 07:32:55 -0700 (PDT)\r
+Received: from localhost (pool-108-8-228-201.spfdma.east.verizon.net.\r
+ [108.8.228.201])\r
+ by mx.google.com with ESMTPSA id g25sm22829171yhg.6.1969.12.31.16.00.00\r
+ (version=TLSv1.2 cipher=RC4-SHA bits=128/128);\r
+ Tue, 03 Sep 2013 07:32:54 -0700 (PDT)\r
+From: Ben Gamari <bgamari.foss@gmail.com>\r
+To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH 1/3] database: Add notmuch_database_compact_close\r
+In-Reply-To: <87ppsssg3c.fsf@nikula.org>\r
+References: <1377312923-32274-1-git-send-email-bgamari.foss@gmail.com>\r
+ <1377312923-32274-2-git-send-email-bgamari.foss@gmail.com>\r
+ <87ppsssg3c.fsf@nikula.org>\r
+User-Agent: Notmuch/0.16+15~g17b9eaa (http://notmuchmail.org) Emacs/24.2.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Tue, 03 Sep 2013 10:32:20 -0400\r
+Message-ID: <87k3iyyo0b.fsf@gmail.com>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/signed; boundary="=-=-=";\r
+ micalg=pgp-sha1; protocol="application/pgp-signature"\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Tue, 03 Sep 2013 14:33:05 -0000\r
+\r
+--=-=-=\r
+Content-Type: text/plain\r
+Content-Transfer-Encoding: quoted-printable\r
+\r
+Jani Nikula <jani@nikula.org> writes:\r
+\r
+> On Sat, 24 Aug 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:\r
+>> This function uses Xapian's Compactor machinery to compact the notmuch\r
+>> database. The compacted database is built in a temporary directory and\r
+>> later moved into place while the original uncompacted database is\r
+>> preserved.\r
+>>\r
+\r
+snip\r
+\r
+>>=20=20\r
+>> +class NotmuchCompactor : public Xapian::Compactor\r
+>> +{\r
+>> +public:\r
+>> + virtual void\r
+>> + set_status (const std::string &table, const std::string &status)\r
+>> + {\r
+>> + if (status.length() =3D=3D 0)\r
+>> + printf ("compacting table %s:\n", table.c_str());\r
+>> + else\r
+>> + printf (" %s\n", status.c_str());\r
+>> + }\r
+>\r
+> We're trying to reduce the amount of prints directly from libnotmuch,\r
+> not increase. This applies here as well as below.\r
+>\r
+Fair enough. That being said, I think that status updates are fairly\r
+important given that the compaction process can be rather long. Would\r
+the preferred interface be to provide notmuch_database_compact_close\r
+with a progress callback?\r
+\r
+>> +};\r
+>> +\r
+>> +#if HAVE_XAPIAN_COMPACT\r
+>> +notmuch_status_t\r
+>> +notmuch_database_compact_close (notmuch_database_t *notmuch)\r
+>> +{\r
+>> + void *local =3D talloc_new (NULL);\r
+>> + NotmuchCompactor compactor;\r
+>> + char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian=\r
+_path;\r
+>> + notmuch_status_t ret =3D NOTMUCH_STATUS_SUCCESS;\r
+>> +\r
+>> + if (! (notmuch_path =3D talloc_asprintf (local, "%s/%s", notmuch->p=\r
+ath, ".notmuch"))) {\r
+>> + ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>> + goto DONE;\r
+>> + }\r
+>> +\r
+>> + if (! (xapian_path =3D talloc_asprintf (local, "%s/%s", notmuch_pat=\r
+h, "xapian"))) {\r
+>> + ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>> + goto DONE;\r
+>> + }\r
+>> +\r
+>> + if (! (compact_xapian_path =3D talloc_asprintf (local, "%s.compact"=\r
+, xapian_path))) {\r
+>> + ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>> + goto DONE;\r
+>> + }\r
+>> +\r
+>> + if (! (old_xapian_path =3D talloc_asprintf (local, "%s.old", xapian=\r
+_path))) {\r
+>> + ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
+>> + goto DONE;\r
+>> + }\r
+>> +\r
+>> + try {\r
+>> + compactor.set_renumber(false);\r
+>> + compactor.add_source(xapian_path);\r
+>> + compactor.set_destdir(compact_xapian_path);\r
+>> + compactor.compact();\r
+>> +\r
+>> + if (rename(xapian_path, old_xapian_path)) {\r
+>> + fprintf (stderr, "Error moving old database out of the way\n");\r
+>> + ret =3D NOTMUCH_STATUS_FILE_ERROR;\r
+>> + goto DONE;\r
+>> + }\r
+>\r
+> This fails if old_xapian_path exists.\r
+>\r
+Ouch, yes, you are right. I suspect the right way forward here will be\r
+to check whether old_xapian_path exists before beginning compaction,\r
+allowing the user to fix the situation before it fails after finishing\r
+what might be a pretty long process.\r
+\r
+>> +\r
+>> + if (rename(compact_xapian_path, xapian_path)) {\r
+>> + fprintf (stderr, "Error moving compacted database\n");\r
+>> + ret =3D NOTMUCH_STATUS_FILE_ERROR;\r
+>> + goto DONE;\r
+>> + }\r
+>> + } catch (Xapian::InvalidArgumentError e) {\r
+>> + fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());\r
+>> + ret =3D NOTMUCH_STATUS_XAPIAN_EXCEPTION;\r
+>> + goto DONE;\r
+>> + }\r
+>> +\r
+>> + fprintf (stderr, "\n");\r
+>> + fprintf (stderr, "\n");\r
+>> + fprintf (stderr, "Old database has been moved to %s", old_xapian_pa=\r
+th);\r
+>> + fprintf (stderr, "\n");\r
+>> + fprintf (stderr, "To delete run,\n");\r
+>> + fprintf (stderr, "\n");\r
+>> + fprintf (stderr, " rm -R %s\n", old_xapian_path);\r
+>> + fprintf (stderr, "\n");\r
+>> +\r
+>> + notmuch_database_close(notmuch);\r
+>> +\r
+>> +DONE:\r
+>> + talloc_free(local);\r
+>\r
+> The database does not get closed on errors. If that's intentional, it\r
+> should be documented.\r
+>\r
+I had reasons for this but they have long fled my memory. Regardless of\r
+what it does, this behavior should be documented. I'll take care of this.\r
+\r
+>> + return ret;\r
+>> +}\r
+>> +#else\r
+>> +notmuch_status_t\r
+>> +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))\r
+>> +{\r
+>> + fprintf (stderr, "notmuch was compiled against a xapian version lac=\r
+king compaction support.\n");\r
+>> + return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;\r
+>> +}\r
+>> +#endif\r
+>> +\r
+>> void\r
+>> notmuch_database_destroy (notmuch_database_t *notmuch)\r
+>> {\r
+>> diff --git a/lib/notmuch.h b/lib/notmuch.h\r
+>> index 998a4ae..e9abd90 100644\r
+>> --- a/lib/notmuch.h\r
+>> +++ b/lib/notmuch.h\r
+>> @@ -101,6 +101,7 @@ typedef enum _notmuch_status {\r
+>> NOTMUCH_STATUS_TAG_TOO_LONG,\r
+>> NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,\r
+>> NOTMUCH_STATUS_UNBALANCED_ATOMIC,\r
+>> + NOTMUCH_STATUS_UNSUPPORTED_OPERATION,\r
+>>=20=20\r
+>> NOTMUCH_STATUS_LAST_STATUS\r
+>> } notmuch_status_t;\r
+>> @@ -215,6 +216,20 @@ notmuch_database_open (const char *path,\r
+>> void\r
+>> notmuch_database_close (notmuch_database_t *database);\r
+>>=20=20\r
+>> +/* Close the given notmuch database and then compact it.\r
+>\r
+> The implementation first compacts then closes.\r
+>\r
+\r
+>> + * After notmuch_database_close_compact has been called, calls to\r
+>> + * other functions on objects derived from this database may either\r
+>> + * behave as if the database had not been closed (e.g., if the\r
+>> + * required data has been cached) or may fail with a\r
+>> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.\r
+>> + *\r
+>> + * notmuch_database_close_compact can be called multiple times. Later\r
+>> + * calls have no effect.\r
+>\r
+> This is not true. The Xapian compactor does not require the database to\r
+> be open. It will happily open the database read-only and compact the\r
+> database again if database has been closed.\r
+>\r
+>> + */\r
+>> +notmuch_status_t\r
+>> +notmuch_database_compact_close (notmuch_database_t *notmuch);\r
+>\r
+> I'm afraid we really need to re-think the API.\r
+>\r
+It seems you are right. When writing this interface it was clear that\r
+there would be a number of opportunities for misuse. I was hoping by\r
+combining compact and close some of these would be eliminated but\r
+clearly this isn't enough.\r
+\r
+> I see that your CLI 'notmuch compact' command opens the database\r
+> read-write, I assume to ensure there are no other writers, so that stuff\r
+> doesn't get lost. However if you pass a read-write database that has\r
+> been modified, I believe the changes will get lost (as Xapian opens the\r
+> database read-only). We shouldn't let the API users shoot themselves in\r
+> the foot so easily.\r
+>\r
+That is correct; the read-write database was an attempt to force the\r
+user to exclusively lock the database they are trying to compact. It\r
+seems that things can go quite wrong[1] when a database is modified\r
+during compaction. There was a suggestion in that thread to add an\r
+option to lock the database during compaction. Perhaps it might be worth\r
+bringing this up again with Xapian upstream. I think we agree that it\r
+would be a poor idea to merge compaction functionality without having a\r
+mechanism for ensuring data integrity, especially since many users\r
+invoke notmuch in a cron job.\r
+\r
+> I think I'd go for something like:\r
+>\r
+> notmuch_status_t\r
+> notmuch_database_compact (const char *path);\r
+>\r
+> or\r
+>\r
+> notmuch_status_t\r
+> notmuch_database_compact (const char *path, const char *backup);\r
+>\r
+> which would internally open the database as read-write to ensure no\r
+> modifications, compact, and close. If backup !=3D NULL, it would save the\r
+> old database there (same mounted file system as the database is a fine\r
+> limitation), otherwise remove.\r
+>\r
+This sounds fine to me.\r
+\r
+> Even then I'm not completely sure what Xapian WritableDatabase will do\r
+> on close when the database has been switched underneath it. But moving\r
+> the database after it has been closed has a race condition too.\r
+>\r
+Good points. Not sure what the least evil way about this is. Hopefully\r
+Xapian's close operation really does just close file handles.\r
+\r
+Cheers,\r
+\r
+=2D Ben\r
+\r
+\r
+[1] http://lists.xapian.org/pipermail/xapian-discuss/2011-July/008310.html\r
+\r
+--=-=-=\r
+Content-Type: application/pgp-signature\r
+\r
+-----BEGIN PGP SIGNATURE-----\r
+Version: GnuPG v1.4.12 (GNU/Linux)\r
+\r
+iQEcBAEBAgAGBQJSJfMVAAoJEErkyLZmeNiDO1MIAKvujl0gYgh6G+CBmZdKdw3p\r
+Cb2o/kQ4Tl/5RU7rHkysAWmr32LeK4O9dSBXK9nR1QnU7bd54ekdrbFhm6ZIGMcU\r
+lHSQQdZ3rHINNiyXD5jvcpTEt2Lb1Jz2Rz7y/rAPon6WpjbkRCncYcHQ0qjGlUNg\r
+PnrLFTN1Xo/32AAUIgxgLE9KUP+h0QhOSTCvvth/CCCKAR3z6BNOBpOmSO6sKLJV\r
+1uWjBJ4KiU2HPc/KYMJX+ek2L/FL+JDf7XgSJbph6kp0Rjd4XsuTdheSDogjnGL4\r
+R2QoVosKnUd+0rO+VeYI90PKqRdduQaTDniiPRCCi7zmXV0+wsZnwYIwWyCX0Qo=\r
+=nhTs\r
+-----END PGP SIGNATURE-----\r
+--=-=-=--\r
projects / notmuch-archives.git / commitdiff