66/d3c55f1932596e9b7cf55e1a2c50d0d8964e29

   1 Return-Path: <bgamari.foss@gmail.com>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 1B438431FCF\r
   6         for <notmuch@notmuchmail.org>; Tue,  3 Sep 2013 07:33:05 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id ePfhwJl0547z for <notmuch@notmuchmail.org>;\r
  17         Tue,  3 Sep 2013 07:32:57 -0700 (PDT)\r
  18 Received: from mail-yh0-f43.google.com (mail-yh0-f43.google.com\r
  19         [209.85.213.43]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 57F36431FC9\r
  22         for <notmuch@notmuchmail.org>; Tue,  3 Sep 2013 07:32:57 -0700 (PDT)\r
  23 Received: by mail-yh0-f43.google.com with SMTP id b6so252912yha.16\r
  24         for <notmuch@notmuchmail.org>; Tue, 03 Sep 2013 07:32:55 -0700 (PDT)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type;\r
  28         bh=2E8XN5K7x3rTAx2m8ZbxK5k6dhSny6A5R2xEgdnF2Ec=;\r
  29         b=VXGXdzGe4jrD7PNiUktk9IUIGLmefAO1ytt9BzfpWE/Zc8uYFyBJyCfTk69w+UryVo\r
  30         MoBaoREJz+vqdZ3lShiADQWSUrOukIFi6jaueuMLozr9XT5LGlbAPHolaRMwvVqfzga4\r
  31         uRXSZtM04ZuBKGRU9bSIliRdPwfe515cURLpPV2Cji5tMueKA/xU4FbxfQHm1La5qZyX\r
  32         s7yThVjXZQxKj1VCGjyS0XgXQnkH+m6puShcB2IGUnJDsHi8Wx1lpm8o7Yc5/juQQ41K\r
  33         oU5dnUYgQavgWoeOaKg/QZduMkh+MulaA3MXpsI5A6ZBOluw/gKWnjAoCSkN/+zTQ0Ms\r
  34         MJ9w==\r
  35 X-Received: by 10.236.9.9 with SMTP id 9mr340884yhs.97.1378218775599;\r
  36         Tue, 03 Sep 2013 07:32:55 -0700 (PDT)\r
  37 Received: from localhost (pool-108-8-228-201.spfdma.east.verizon.net.\r
  38         [108.8.228.201])\r
  39         by mx.google.com with ESMTPSA id g25sm22829171yhg.6.1969.12.31.16.00.00\r
  40         (version=TLSv1.2 cipher=RC4-SHA bits=128/128);\r
  41         Tue, 03 Sep 2013 07:32:54 -0700 (PDT)\r
  42 From: Ben Gamari <bgamari.foss@gmail.com>\r
  43 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  44 Subject: Re: [PATCH 1/3] database: Add notmuch_database_compact_close\r
  45 In-Reply-To: <87ppsssg3c.fsf@nikula.org>\r
  46 References: <1377312923-32274-1-git-send-email-bgamari.foss@gmail.com>\r
  47         <1377312923-32274-2-git-send-email-bgamari.foss@gmail.com>\r
  48         <87ppsssg3c.fsf@nikula.org>\r
  49 User-Agent: Notmuch/0.16+15~g17b9eaa (http://notmuchmail.org) Emacs/24.2.1\r
  50         (x86_64-pc-linux-gnu)\r
  51 Date: Tue, 03 Sep 2013 10:32:20 -0400\r
  52 Message-ID: <87k3iyyo0b.fsf@gmail.com>\r
  53 MIME-Version: 1.0\r
  54 Content-Type: multipart/signed; boundary="=-=-=";\r
  55         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  56 X-BeenThere: notmuch@notmuchmail.org\r
  57 X-Mailman-Version: 2.1.13\r
  58 Precedence: list\r
  59 List-Id: "Use and development of the notmuch mail system."\r
  60         <notmuch.notmuchmail.org>\r
  61 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  62         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  63 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  64 List-Post: <mailto:notmuch@notmuchmail.org>\r
  65 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  66 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  67         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  68 X-List-Received-Date: Tue, 03 Sep 2013 14:33:05 -0000\r
  69 \r
  70 --=-=-=\r
  71 Content-Type: text/plain\r
  72 Content-Transfer-Encoding: quoted-printable\r
  73 \r
  74 Jani Nikula <jani@nikula.org> writes:\r
  75 \r
  76 > On Sat, 24 Aug 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:\r
  77 >> This function uses Xapian's Compactor machinery to compact the notmuch\r
  78 >> database. The compacted database is built in a temporary directory and\r
  79 >> later moved into place while the original uncompacted database is\r
  80 >> preserved.\r
  81 >>\r
  82 \r
  83 snip\r
  84 \r
  85 >>=20=20\r
  86 >> +class NotmuchCompactor : public Xapian::Compactor\r
  87 >> +{\r
  88 >> +public:\r
  89 >> +    virtual void\r
  90 >> +    set_status (const std::string &table, const std::string &status)\r
  91 >> +    {\r
  92 >> +    if (status.length() =3D=3D 0)\r
  93 >> +        printf ("compacting table %s:\n", table.c_str());\r
  94 >> +    else\r
  95 >> +        printf ("     %s\n", status.c_str());\r
  96 >> +    }\r
  97 >\r
  98 > We're trying to reduce the amount of prints directly from libnotmuch,\r
  99 > not increase. This applies here as well as below.\r
 100 >\r
 101 Fair enough. That being said, I think that status updates are fairly\r
 102 important given that the compaction process can be rather long.  Would\r
 103 the preferred interface be to provide notmuch_database_compact_close\r
 104 with a progress callback?\r
 105 \r
 106 >> +};\r
 107 >> +\r
 108 >> +#if HAVE_XAPIAN_COMPACT\r
 109 >> +notmuch_status_t\r
 110 >> +notmuch_database_compact_close (notmuch_database_t *notmuch)\r
 111 >> +{\r
 112 >> +    void *local =3D talloc_new (NULL);\r
 113 >> +    NotmuchCompactor compactor;\r
 114 >> +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian=\r
 115 _path;\r
 116 >> +    notmuch_status_t ret =3D NOTMUCH_STATUS_SUCCESS;\r
 117 >> +\r
 118 >> +    if (! (notmuch_path =3D talloc_asprintf (local, "%s/%s", notmuch->p=\r
 119 ath, ".notmuch"))) {\r
 120 >> +    ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 121 >> +    goto DONE;\r
 122 >> +    }\r
 123 >> +\r
 124 >> +    if (! (xapian_path =3D talloc_asprintf (local, "%s/%s", notmuch_pat=\r
 125 h, "xapian"))) {\r
 126 >> +    ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 127 >> +    goto DONE;\r
 128 >> +    }\r
 129 >> +\r
 130 >> +    if (! (compact_xapian_path =3D talloc_asprintf (local, "%s.compact"=\r
 131 , xapian_path))) {\r
 132 >> +    ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 133 >> +    goto DONE;\r
 134 >> +    }\r
 135 >> +\r
 136 >> +    if (! (old_xapian_path =3D talloc_asprintf (local, "%s.old", xapian=\r
 137 _path))) {\r
 138 >> +    ret =3D NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 139 >> +    goto DONE;\r
 140 >> +    }\r
 141 >> +\r
 142 >> +    try {\r
 143 >> +    compactor.set_renumber(false);\r
 144 >> +    compactor.add_source(xapian_path);\r
 145 >> +    compactor.set_destdir(compact_xapian_path);\r
 146 >> +    compactor.compact();\r
 147 >> +\r
 148 >> +    if (rename(xapian_path, old_xapian_path)) {\r
 149 >> +        fprintf (stderr, "Error moving old database out of the way\n");\r
 150 >> +        ret =3D NOTMUCH_STATUS_FILE_ERROR;\r
 151 >> +        goto DONE;\r
 152 >> +    }\r
 153 >\r
 154 > This fails if old_xapian_path exists.\r
 155 >\r
 156 Ouch, yes, you are right. I suspect the right way forward here will be\r
 157 to check whether old_xapian_path exists before beginning compaction,\r
 158 allowing the user to fix the situation before it fails after finishing\r
 159 what might be a pretty long process.\r
 160 \r
 161 >> +\r
 162 >> +    if (rename(compact_xapian_path, xapian_path)) {\r
 163 >> +        fprintf (stderr, "Error moving compacted database\n");\r
 164 >> +        ret =3D NOTMUCH_STATUS_FILE_ERROR;\r
 165 >> +        goto DONE;\r
 166 >> +    }\r
 167 >> +    } catch (Xapian::InvalidArgumentError e) {\r
 168 >> +    fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());\r
 169 >> +    ret =3D NOTMUCH_STATUS_XAPIAN_EXCEPTION;\r
 170 >> +    goto DONE;\r
 171 >> +    }\r
 172 >> +\r
 173 >> +    fprintf (stderr, "\n");\r
 174 >> +    fprintf (stderr, "\n");\r
 175 >> +    fprintf (stderr, "Old database has been moved to %s", old_xapian_pa=\r
 176 th);\r
 177 >> +    fprintf (stderr, "\n");\r
 178 >> +    fprintf (stderr, "To delete run,\n");\r
 179 >> +    fprintf (stderr, "\n");\r
 180 >> +    fprintf (stderr, "    rm -R %s\n", old_xapian_path);\r
 181 >> +    fprintf (stderr, "\n");\r
 182 >> +\r
 183 >> +    notmuch_database_close(notmuch);\r
 184 >> +\r
 185 >> +DONE:\r
 186 >> +    talloc_free(local);\r
 187 >\r
 188 > The database does not get closed on errors. If that's intentional, it\r
 189 > should be documented.\r
 190 >\r
 191 I had reasons for this but they have long fled my memory. Regardless of\r
 192 what it does, this behavior should be documented. I'll take care of this.\r
 193 \r
 194 >> +    return ret;\r
 195 >> +}\r
 196 >> +#else\r
 197 >> +notmuch_status_t\r
 198 >> +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))\r
 199 >> +{\r
 200 >> +    fprintf (stderr, "notmuch was compiled against a xapian version lac=\r
 201 king compaction support.\n");\r
 202 >> +    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;\r
 203 >> +}\r
 204 >> +#endif\r
 205 >> +\r
 206 >>  void\r
 207 >>  notmuch_database_destroy (notmuch_database_t *notmuch)\r
 208 >>  {\r
 209 >> diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 210 >> index 998a4ae..e9abd90 100644\r
 211 >> --- a/lib/notmuch.h\r
 212 >> +++ b/lib/notmuch.h\r
 213 >> @@ -101,6 +101,7 @@ typedef enum _notmuch_status {\r
 214 >>      NOTMUCH_STATUS_TAG_TOO_LONG,\r
 215 >>      NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,\r
 216 >>      NOTMUCH_STATUS_UNBALANCED_ATOMIC,\r
 217 >> +    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,\r
 218 >>=20=20\r
 219 >>      NOTMUCH_STATUS_LAST_STATUS\r
 220 >>  } notmuch_status_t;\r
 221 >> @@ -215,6 +216,20 @@ notmuch_database_open (const char *path,\r
 222 >>  void\r
 223 >>  notmuch_database_close (notmuch_database_t *database);\r
 224 >>=20=20\r
 225 >> +/* Close the given notmuch database and then compact it.\r
 226 >\r
 227 > The implementation first compacts then closes.\r
 228 >\r
 229 \r
 230 >> + * After notmuch_database_close_compact has been called, calls to\r
 231 >> + * other functions on objects derived from this database may either\r
 232 >> + * behave as if the database had not been closed (e.g., if the\r
 233 >> + * required data has been cached) or may fail with a\r
 234 >> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.\r
 235 >> + *\r
 236 >> + * notmuch_database_close_compact can be called multiple times.  Later\r
 237 >> + * calls have no effect.\r
 238 >\r
 239 > This is not true. The Xapian compactor does not require the database to\r
 240 > be open. It will happily open the database read-only and compact the\r
 241 > database again if database has been closed.\r
 242 >\r
 243 >> + */\r
 244 >> +notmuch_status_t\r
 245 >> +notmuch_database_compact_close (notmuch_database_t *notmuch);\r
 246 >\r
 247 > I'm afraid we really need to re-think the API.\r
 248 >\r
 249 It seems you are right. When writing this interface it was clear that\r
 250 there would be a number of opportunities for misuse. I was hoping by\r
 251 combining compact and close some of these would be eliminated but\r
 252 clearly this isn't enough.\r
 253 \r
 254 > I see that your CLI 'notmuch compact' command opens the database\r
 255 > read-write, I assume to ensure there are no other writers, so that stuff\r
 256 > doesn't get lost. However if you pass a read-write database that has\r
 257 > been modified, I believe the changes will get lost (as Xapian opens the\r
 258 > database read-only). We shouldn't let the API users shoot themselves in\r
 259 > the foot so easily.\r
 260 >\r
 261 That is correct; the read-write database was an attempt to force the\r
 262 user to exclusively lock the database they are trying to compact. It\r
 263 seems that things can go quite wrong[1] when a database is modified\r
 264 during compaction.  There was a suggestion in that thread to add an\r
 265 option to lock the database during compaction. Perhaps it might be worth\r
 266 bringing this up again with Xapian upstream. I think we agree that it\r
 267 would be a poor idea to merge compaction functionality without having a\r
 268 mechanism for ensuring data integrity, especially since many users\r
 269 invoke notmuch in a cron job.\r
 270 \r
 271 > I think I'd go for something like:\r
 272 >\r
 273 > notmuch_status_t\r
 274 > notmuch_database_compact (const char *path);\r
 275 >\r
 276 > or\r
 277 >\r
 278 > notmuch_status_t\r
 279 > notmuch_database_compact (const char *path, const char *backup);\r
 280 >\r
 281 > which would internally open the database as read-write to ensure no\r
 282 > modifications, compact, and close. If backup !=3D NULL, it would save the\r
 283 > old database there (same mounted file system as the database is a fine\r
 284 > limitation), otherwise remove.\r
 285 >\r
 286 This sounds fine to me.\r
 287 \r
 288 > Even then I'm not completely sure what Xapian WritableDatabase will do\r
 289 > on close when the database has been switched underneath it. But moving\r
 290 > the database after it has been closed has a race condition too.\r
 291 >\r
 292 Good points. Not sure what the least evil way about this is. Hopefully\r
 293 Xapian's close operation really does just close file handles.\r
 294 \r
 295 Cheers,\r
 296 \r
 297 =2D Ben\r
 298 \r
 299 \r
 300 [1] http://lists.xapian.org/pipermail/xapian-discuss/2011-July/008310.html\r
 301 \r
 302 --=-=-=\r
 303 Content-Type: application/pgp-signature\r
 304 \r
 305 -----BEGIN PGP SIGNATURE-----\r
 306 Version: GnuPG v1.4.12 (GNU/Linux)\r
 307 \r
 308 iQEcBAEBAgAGBQJSJfMVAAoJEErkyLZmeNiDO1MIAKvujl0gYgh6G+CBmZdKdw3p\r
 309 Cb2o/kQ4Tl/5RU7rHkysAWmr32LeK4O9dSBXK9nR1QnU7bd54ekdrbFhm6ZIGMcU\r
 310 lHSQQdZ3rHINNiyXD5jvcpTEt2Lb1Jz2Rz7y/rAPon6WpjbkRCncYcHQ0qjGlUNg\r
 311 PnrLFTN1Xo/32AAUIgxgLE9KUP+h0QhOSTCvvth/CCCKAR3z6BNOBpOmSO6sKLJV\r
 312 1uWjBJ4KiU2HPc/KYMJX+ek2L/FL+JDf7XgSJbph6kp0Rjd4XsuTdheSDogjnGL4\r
 313 R2QoVosKnUd+0rO+VeYI90PKqRdduQaTDniiPRCCi7zmXV0+wsZnwYIwWyCX0Qo=\r
 314 =nhTs\r
 315 -----END PGP SIGNATURE-----\r
 316 --=-=-=--\r