21/2f2ac9b233aff7451c0cb828f5b0f353587662

   1 Return-Path: <jani@nikula.org>\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 4A988431FB6\r
   6         for <notmuch@notmuchmail.org>; Sun,  1 Sep 2013 08:43:11 -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.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id RNmHTXttIMd2 for <notmuch@notmuchmail.org>;\r
  16         Sun,  1 Sep 2013 08:43:05 -0700 (PDT)\r
  17 Received: from mail-ee0-f42.google.com (mail-ee0-f42.google.com\r
  18  [74.125.83.42])        (using TLSv1 with cipher RC4-SHA (128/128 bits))        (No client\r
  19  certificate requested) by olra.theworths.org (Postfix) with ESMTPS id\r
  20  F3175431FAE    for <notmuch@notmuchmail.org>; Sun,  1 Sep 2013 08:43:04 -0700\r
  21  (PDT)\r
  22 Received: by mail-ee0-f42.google.com with SMTP id b45so1889931eek.1\r
  23         for <notmuch@notmuchmail.org>; Sun, 01 Sep 2013 08:43:03 -0700 (PDT)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=1e100.net; s=20130820;\r
  26         h=x-gm-message-state:from:to:subject:in-reply-to:references\r
  27         :user-agent:date:message-id:mime-version:content-type;\r
  28         bh=vibekIws0liU6uhc67V/e7GVfvokv/mcXDZb3z4g5TA=;\r
  29         b=gsHmaDM8xfohEkLA6r8m5wmGnKzI/FirKcEzzHuNgMhYx7kEcYoMOvbnv+TuBSxdOn\r
  30         hkbBA0p4N8hUb07jAeFT8VO/D0Oo31KzYC/rIcV0DJkNInihNLDVKlJ2WhXs5jOBcHLU\r
  31         8wCYx+sz9HvLMOEfFylVALbXmzweR1s5dTua+sTJbuzs4MwWagacTV7qbRIseNDQyLW5\r
  32         x/S12AesDGBB/9Vp9a9t2Z1xJkujwWS9hhKl77cxrZ6oYmOe9OAlLuF9RnVOPRGdG/ny\r
  33         yENW6TSKMkQUlyKha5qtqkNWloglbAslectkhC6odwuHOt9HQXei94t/jE9fELw6YRM0\r
  34         dYAQ==\r
  35 X-Gm-Message-State:\r
  36  ALoCoQm5bwTgYD2yYOMZ0w/+Wj8WhK5L+3oaksYgcB5XKs52GSEXQfMc23plfVbqrgy2cfX6S77n\r
  37 X-Received: by 10.14.183.2 with SMTP id p2mr4829714eem.44.1378050183682;\r
  38         Sun, 01 Sep 2013 08:43:03 -0700 (PDT)\r
  39 Received: from localhost (dsl-hkibrasgw2-58c36f-91.dhcp.inet.fi.\r
  40         [88.195.111.91])\r
  41         by mx.google.com with ESMTPSA id z12sm14238866eev.6.1969.12.31.16.00.00\r
  42         (version=TLSv1.2 cipher=RC4-SHA bits=128/128);\r
  43         Sun, 01 Sep 2013 08:43:03 -0700 (PDT)\r
  44 From: Jani Nikula <jani@nikula.org>\r
  45 To: Ben Gamari <bgamari.foss@gmail.com>, notmuch@notmuchmail.org\r
  46 Subject: Re: [PATCH 1/3] database: Add notmuch_database_compact_close\r
  47 In-Reply-To: <1377312923-32274-2-git-send-email-bgamari.foss@gmail.com>\r
  48 References: <1377312923-32274-1-git-send-email-bgamari.foss@gmail.com>\r
  49         <1377312923-32274-2-git-send-email-bgamari.foss@gmail.com>\r
  50 User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/24.3.1\r
  51         (x86_64-pc-linux-gnu)\r
  52 Date: Sun, 01 Sep 2013 18:43:03 +0300\r
  53 Message-ID: <87ppsssg3c.fsf@nikula.org>\r
  54 MIME-Version: 1.0\r
  55 Content-Type: text/plain\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: Sun, 01 Sep 2013 15:43:11 -0000\r
  69 \r
  70 On Sat, 24 Aug 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:\r
  71 > This function uses Xapian's Compactor machinery to compact the notmuch\r
  72 > database. The compacted database is built in a temporary directory and\r
  73 > later moved into place while the original uncompacted database is\r
  74 > preserved.\r
  75 >\r
  76 > Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>\r
  77 > ---\r
  78 >  configure       | 27 +++++++++++++++--\r
  79 >  lib/database.cc | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
  80 >  lib/notmuch.h   | 15 ++++++++++\r
  81 >  3 files changed, 130 insertions(+), 3 deletions(-)\r
  82 >\r
  83 > diff --git a/configure b/configure\r
  84 > index 6166917..ee9e887 100755\r
  85 > --- a/configure\r
  86 > +++ b/configure\r
  87 > @@ -277,7 +277,8 @@ printf "Checking for Xapian development files... "\r
  88 >  have_xapian=0\r
  89 >  for xapian_config in ${XAPIAN_CONFIG}; do\r
  90 >      if ${xapian_config} --version > /dev/null 2>&1; then\r
  91 > -     printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')\r
  92 > +     xapian_version=$(${xapian_config} --version | sed -e 's/.* //')\r
  93 > +     printf "Yes (%s).\n" ${xapian_version}\r
  94 >       have_xapian=1\r
  95 >       xapian_cxxflags=$(${xapian_config} --cxxflags)\r
  96 >       xapian_ldflags=$(${xapian_config} --libs)\r
  97 > @@ -289,6 +290,21 @@ if [ ${have_xapian} = "0" ]; then\r
  98 >      errors=$((errors + 1))\r
  99 >  fi\r
 100 >  \r
 101 > +# Compaction is only supported on Xapian > 1.2.6\r
 102 > +have_xapian_compact=0\r
 103 > +if [ ${have_xapian} = "1" ]; then\r
 104 > +    printf "Checking for Xapian compact support... "\r
 105 > +    case "${xapian_version}" in\r
 106 > +        0.*|1.[01].*|1.2.[0-5])\r
 107 > +            printf "No (only available with Xapian > 1.2.6).\n" ;;\r
 108 > +        [1-9]*.[0-9]*.[0-9]*)\r
 109 > +            have_xapian_compact=1\r
 110 > +            printf "Yes.\n" ;;\r
 111 > +        *)\r
 112 > +            printf "Unknown version.\n" ;;\r
 113 > +    esac\r
 114 > +fi\r
 115 > +\r
 116 >  printf "Checking for GMime development files... "\r
 117 >  have_gmime=0\r
 118 >  IFS=';'\r
 119 > @@ -729,6 +745,9 @@ HAVE_STRCASESTR = ${have_strcasestr}\r
 120 >  # build its own version)\r
 121 >  HAVE_STRSEP = ${have_strsep}\r
 122 >  \r
 123 > +# Whether the Xapian version in use supports compaction\r
 124 > +HAVE_XAPIAN_COMPACT = ${have_xapian_compact}\r
 125 > +\r
 126 >  # Whether the getpwuid_r function is standards-compliant\r
 127 >  # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS\r
 128 >  # to enable the standards-compliant version -- needed for Solaris)\r
 129 > @@ -787,13 +806,15 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\\r
 130 >                  -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\\r
 131 >                  -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\\r
 132 >                  -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\\r
 133 > -                -DSTD_ASCTIME=\$(STD_ASCTIME)\r
 134 > +                -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\\r
 135 > +                   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)\r
 136 >  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\\r
 137 >                    \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\\r
 138 >                    \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\\r
 139 >                    -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\\r
 140 >                    -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\\r
 141 >                    -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\\r
 142 > -                  -DSTD_ASCTIME=\$(STD_ASCTIME)\r
 143 > +                  -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\\r
 144 > +                     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)\r
 145 >  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)\r
 146 >  EOF\r
 147 > diff --git a/lib/database.cc b/lib/database.cc\r
 148 > index 5cc0765..b71751d 100644\r
 149 > --- a/lib/database.cc\r
 150 > +++ b/lib/database.cc\r
 151 > @@ -268,6 +268,8 @@ notmuch_status_to_string (notmuch_status_t status)\r
 152 >       return "Unbalanced number of calls to notmuch_message_freeze/thaw";\r
 153 >      case NOTMUCH_STATUS_UNBALANCED_ATOMIC:\r
 154 >       return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";\r
 155 > +    case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:\r
 156 > +     return "Unsupported operation";\r
 157 >      default:\r
 158 >      case NOTMUCH_STATUS_LAST_STATUS:\r
 159 >       return "Unknown error status value";\r
 160 > @@ -800,6 +802,95 @@ notmuch_database_close (notmuch_database_t *notmuch)\r
 161 >      notmuch->date_range_processor = NULL;\r
 162 >  }\r
 163 >  \r
 164 > +class NotmuchCompactor : public Xapian::Compactor\r
 165 > +{\r
 166 > +public:\r
 167 > +    virtual void\r
 168 > +    set_status (const std::string &table, const std::string &status)\r
 169 > +    {\r
 170 > +     if (status.length() == 0)\r
 171 > +         printf ("compacting table %s:\n", table.c_str());\r
 172 > +     else\r
 173 > +         printf ("     %s\n", status.c_str());\r
 174 > +    }\r
 175 \r
 176 We're trying to reduce the amount of prints directly from libnotmuch,\r
 177 not increase. This applies here as well as below.\r
 178 \r
 179 > +};\r
 180 > +\r
 181 > +#if HAVE_XAPIAN_COMPACT\r
 182 > +notmuch_status_t\r
 183 > +notmuch_database_compact_close (notmuch_database_t *notmuch)\r
 184 > +{\r
 185 > +    void *local = talloc_new (NULL);\r
 186 > +    NotmuchCompactor compactor;\r
 187 > +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;\r
 188 > +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;\r
 189 > +\r
 190 > +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {\r
 191 > +     ret = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 192 > +     goto DONE;\r
 193 > +    }\r
 194 > +\r
 195 > +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {\r
 196 > +     ret = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 197 > +     goto DONE;\r
 198 > +    }\r
 199 > +\r
 200 > +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {\r
 201 > +     ret = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 202 > +     goto DONE;\r
 203 > +    }\r
 204 > +\r
 205 > +    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {\r
 206 > +     ret = NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 207 > +     goto DONE;\r
 208 > +    }\r
 209 > +\r
 210 > +    try {\r
 211 > +     compactor.set_renumber(false);\r
 212 > +     compactor.add_source(xapian_path);\r
 213 > +     compactor.set_destdir(compact_xapian_path);\r
 214 > +     compactor.compact();\r
 215 > +\r
 216 > +     if (rename(xapian_path, old_xapian_path)) {\r
 217 > +         fprintf (stderr, "Error moving old database out of the way\n");\r
 218 > +         ret = NOTMUCH_STATUS_FILE_ERROR;\r
 219 > +         goto DONE;\r
 220 > +     }\r
 221 \r
 222 This fails if old_xapian_path exists.\r
 223 \r
 224 > +\r
 225 > +     if (rename(compact_xapian_path, xapian_path)) {\r
 226 > +         fprintf (stderr, "Error moving compacted database\n");\r
 227 > +         ret = NOTMUCH_STATUS_FILE_ERROR;\r
 228 > +         goto DONE;\r
 229 > +     }\r
 230 > +    } catch (Xapian::InvalidArgumentError e) {\r
 231 > +     fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());\r
 232 > +     ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;\r
 233 > +     goto DONE;\r
 234 > +    }\r
 235 > +\r
 236 > +    fprintf (stderr, "\n");\r
 237 > +    fprintf (stderr, "\n");\r
 238 > +    fprintf (stderr, "Old database has been moved to %s", old_xapian_path);\r
 239 > +    fprintf (stderr, "\n");\r
 240 > +    fprintf (stderr, "To delete run,\n");\r
 241 > +    fprintf (stderr, "\n");\r
 242 > +    fprintf (stderr, "    rm -R %s\n", old_xapian_path);\r
 243 > +    fprintf (stderr, "\n");\r
 244 > +\r
 245 > +    notmuch_database_close(notmuch);\r
 246 > +\r
 247 > +DONE:\r
 248 > +    talloc_free(local);\r
 249 \r
 250 The database does not get closed on errors. If that's intentional, it\r
 251 should be documented.\r
 252 \r
 253 > +    return ret;\r
 254 > +}\r
 255 > +#else\r
 256 > +notmuch_status_t\r
 257 > +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))\r
 258 > +{\r
 259 > +    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");\r
 260 > +    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;\r
 261 > +}\r
 262 > +#endif\r
 263 > +\r
 264 >  void\r
 265 >  notmuch_database_destroy (notmuch_database_t *notmuch)\r
 266 >  {\r
 267 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 268 > index 998a4ae..e9abd90 100644\r
 269 > --- a/lib/notmuch.h\r
 270 > +++ b/lib/notmuch.h\r
 271 > @@ -101,6 +101,7 @@ typedef enum _notmuch_status {\r
 272 >      NOTMUCH_STATUS_TAG_TOO_LONG,\r
 273 >      NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,\r
 274 >      NOTMUCH_STATUS_UNBALANCED_ATOMIC,\r
 275 > +    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,\r
 276 >  \r
 277 >      NOTMUCH_STATUS_LAST_STATUS\r
 278 >  } notmuch_status_t;\r
 279 > @@ -215,6 +216,20 @@ notmuch_database_open (const char *path,\r
 280 >  void\r
 281 >  notmuch_database_close (notmuch_database_t *database);\r
 282 >  \r
 283 > +/* Close the given notmuch database and then compact it.\r
 284 \r
 285 The implementation first compacts then closes.\r
 286 \r
 287 > + * After notmuch_database_close_compact has been called, calls to\r
 288 > + * other functions on objects derived from this database may either\r
 289 > + * behave as if the database had not been closed (e.g., if the\r
 290 > + * required data has been cached) or may fail with a\r
 291 > + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.\r
 292 > + *\r
 293 > + * notmuch_database_close_compact can be called multiple times.  Later\r
 294 > + * calls have no effect.\r
 295 \r
 296 This is not true. The Xapian compactor does not require the database to\r
 297 be open. It will happily open the database read-only and compact the\r
 298 database again if database has been closed.\r
 299 \r
 300 > + */\r
 301 > +notmuch_status_t\r
 302 > +notmuch_database_compact_close (notmuch_database_t *notmuch);\r
 303 \r
 304 I'm afraid we really need to re-think the API.\r
 305 \r
 306 I see that your CLI 'notmuch compact' command opens the database\r
 307 read-write, I assume to ensure there are no other writers, so that stuff\r
 308 doesn't get lost. However if you pass a read-write database that has\r
 309 been modified, I believe the changes will get lost (as Xapian opens the\r
 310 database read-only). We shouldn't let the API users shoot themselves in\r
 311 the foot so easily.\r
 312 \r
 313 I think I'd go for something like:\r
 314 \r
 315 notmuch_status_t\r
 316 notmuch_database_compact (const char *path);\r
 317 \r
 318 or\r
 319 \r
 320 notmuch_status_t\r
 321 notmuch_database_compact (const char *path, const char *backup);\r
 322 \r
 323 which would internally open the database as read-write to ensure no\r
 324 modifications, compact, and close. If backup != NULL, it would save the\r
 325 old database there (same mounted file system as the database is a fine\r
 326 limitation), otherwise remove.\r
 327 \r
 328 Even then I'm not completely sure what Xapian WritableDatabase will do\r
 329 on close when the database has been switched underneath it. But moving\r
 330 the database after it has been closed has a race condition too.\r
 331 \r
 332 \r
 333 BR,\r
 334 Jani.\r
 335 \r
 336 \r
 337 \r
 338 > +\r
 339 >  /* Destroy the notmuch database, closing it if necessary and freeing\r
 340 >  * all associated resources. */\r
 341 >  void\r
 342 > -- \r
 343 > 1.8.1.2\r
 344 >\r
 345 > _______________________________________________\r
 346 > notmuch mailing list\r
 347 > notmuch@notmuchmail.org\r
 348 > http://notmuchmail.org/mailman/listinfo/notmuch\r
 349 \r
 350 -- \r
 351 Jani\r