[ovs-dev] [PATCH] ovsdb: compact databases more strictly

[Ben Pfaff]

On Fri, Mar 09, 2018 at 08:05:22AM -0600, Mark Michelson wrote:
> Hi Daniel,
> 
> Mostly this looks correct. I had one small finding and have noted it in-line
> down below.
> 
> On 03/08/2018 04:20 PM, Daniel Alvarez wrote:
> >Before this patch, the databases were automatically compacted when a
> >transaction is logged when:
> >
> >* It's been > 10 minutes after last compaction AND
> >* At least 100 commits have occurred AND
> >* Database has grown at least 4x since last compaction (and it's > 10M)
> >
> >This patch changes the conditions as follows:
> >
> >* It's been > 10 minutes after last compaction AND
> >* At least 100 commits have occurred AND either
> >    - It's been > 24 hours after the last compaction OR
> >    - Database has grown at least 2x since last compaction (and it's > 10M)
> >
> >Reported-by: Daniel Alvarez <dalvarez at redhat.com>
> >Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html
> >Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
> >---
> >  ovsdb/file.c            | 17 +++++++++++++----
> >  ovsdb/log.c             |  2 +-
> >  ovsdb/ovsdb-server.1.in |  6 ++++--
> >  3 files changed, 18 insertions(+), 7 deletions(-)
> >
> >diff --git a/ovsdb/file.c b/ovsdb/file.c
> >index 4b7ad52ab..02e0e8b76 100644
> >--- a/ovsdb/file.c
> >+++ b/ovsdb/file.c
> >@@ -46,6 +46,10 @@ VLOG_DEFINE_THIS_MODULE(ovsdb_file);
> >   * compacting fails. */
> >  #define COMPACT_RETRY_MSEC      (60 * 1000)      /* 1 minute. */
> >+/* Maximum number of milliseconds before compacting the database regardless
> >+ * of its size. */
> >+#define COMPACT_MAX_MSEC        (24 * 60 * 60 * 1000) /* 24 hours. */
> >+
> >  /* A transaction being converted to JSON for writing to a file. */
> >  struct ovsdb_file_txn {
> >      struct json *json;          /* JSON for the whole transaction. */
> >@@ -586,6 +590,7 @@ ovsdb_file_commit(struct ovsdb_replica *replica,
> >      struct ovsdb_file *file = ovsdb_file_cast(replica);
> >      struct ovsdb_file_txn ftxn;
> >      struct ovsdb_error *error;
> >+    long long int next_compact_elapsed;
> >      ovsdb_file_txn_init(&ftxn);
> >      ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, &ftxn);
> >@@ -604,11 +609,15 @@ ovsdb_file_commit(struct ovsdb_replica *replica,
> >      /* If it has been at least COMPACT_MIN_MSEC ms since the last time we
> >       * compacted (or at least COMPACT_RETRY_MSEC ms since the last time we
> >       * tried), and if there are at least 100 transactions in the database, and
> >-     * if the database is at least 10 MB, and the database is at least 4x the
> >-     * size of the previous snapshot, then compact the database. */
> >-    if (time_msec() >= file->next_compact
> >+     * if the database is at least 10 MB, and the database is at least 2x the
> >+     * size of the previous snapshot, then compact the database. However, if
> >+     * it has been over COMPACT_MAX_MSEC ms, the database size is not taken
> >+     * into account. */
> >+    next_compact_elapsed = time_msec() - file->next_compact;
> >+    if (next_compact_elapsed >= 0
> >          && file->n_transactions >= 100
> >-        && ovsdb_log_grew_lots(file->log)) {
> >+        && (next_compact_elapsed >= COMPACT_MAX_MSEC
> 
> I think this line is not right. If the idea is to compact if it has been
> more than 24 hours since the last time we did, then we should be doing
> 
> time_msec() - file->last_compact >= COMPACT_MAX_MSEC
> 
> instead of
> 
> time_msec() - file->next_compact >= COMPACT_MAX_MSEC

Oops, I missed this before I committed.  Daniel, would you mind sending
a fix?