db-ctl-base: Use partial map/set updates for last add/set commands.

Currently, command to add one item into a large set generates the
transaction with the full new content of that set plus 'wait'
operation for the full old content of that set.  So, if we're adding
one new load-balancer into a load-balancer group in OVN using
ovn-nbctl, transaction will include all the existing load-balancers
from that groups twice.

IDL supports partial updates for sets and maps.  The problem with that
is changes are not visible to the IDL user until the transaction
is committed.  That will cause problems for chained ctl commands.
However, we still can optimize the very last command in the list.
It makes sense to do, since it's a common case for manual invocations.

Updating the 'add' command as well as 'set' for a case where we're
actually adding one new element to the map.

One downside is that we can't check the set size without examining
it and checking for duplicates, so allowing the transaction to be
sent and constraints to be checked on the server side in that case.

Not touching 'remove' operation for now, since removals may have
different type, e.g. if elements from the map are removed by the key.
The function will likely need to be fully re-written to accommodate
all the corner cases.

Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Author: igsilya

lib/db-ctl-base.c

@@ -75,7 +75,7 @@ static struct shash all_commands = SHASH_INITIALIZER(&all_commands);
 static char *get_table(const char *, const struct ovsdb_idl_table_class **);
 static char *set_column(const struct ovsdb_idl_table_class *,
                         const struct ovsdb_idl_row *, const char *,
-                        struct ovsdb_symbol_table *);
+                        struct ovsdb_symbol_table *, bool use_partial_update);
 
 
 static struct option *
@@ -1325,11 +1325,17 @@ cmd_find(struct ctl_context *ctx)
 }
 
 /* Sets the column of 'row' in 'table'. Returns NULL on success or a
- * malloc()'ed error message on failure. */
+ * malloc()'ed error message on failure.
+ *
+ * If 'use_partial_update' is true, then this function will try to use
+ * partial set/map updates, if possible.  As a side effect, result will
+ * not be reflected in the IDL until the transaction is committed.
+ * The last access to a particular column is a good candidate to use
+ * this option. */
 static char * OVS_WARN_UNUSED_RESULT
 set_column(const struct ovsdb_idl_table_class *table,
            const struct ovsdb_idl_row *row, const char *arg,
-           struct ovsdb_symbol_table *symtab)
+           struct ovsdb_symbol_table *symtab, bool use_partial_update)
 {
     const struct ovsdb_idl_column *column;
     char *key_string = NULL;
@@ -1352,7 +1358,7 @@ set_column(const struct ovsdb_idl_table_class *table,
 
     if (key_string) {
         union ovsdb_atom key, value;
-        struct ovsdb_datum datum;
+        struct ovsdb_datum *datum;
 
         if (column->type.value.type == OVSDB_TYPE_VOID) {
             error = xasprintf("cannot specify key to set for non-map column "
@@ -1371,16 +1377,22 @@ set_column(const struct ovsdb_idl_table_class *table,
             goto out;
         }
 
-        ovsdb_datum_init_empty(&datum);
-        ovsdb_datum_add_unsafe(&datum, &key, &value, &column->type, NULL);
+        datum = xmalloc(sizeof *datum);
+        ovsdb_datum_init_empty(datum);
+        ovsdb_datum_add_unsafe(datum, &key, &value, &column->type, NULL);
 
         ovsdb_atom_destroy(&key, column->type.key.type);
         ovsdb_atom_destroy(&value, column->type.value.type);
 
-        ovsdb_datum_union(&datum, ovsdb_idl_read(row, column),
-                          &column->type);
-        ovsdb_idl_txn_verify(row, column);
-        ovsdb_idl_txn_write(row, column, &datum);
+        if (use_partial_update) {
+            ovsdb_idl_txn_write_partial_map(row, column, datum);
+        } else {
+            ovsdb_datum_union(datum, ovsdb_idl_read(row, column),
+                              &column->type);
+            ovsdb_idl_txn_verify(row, column);
+            ovsdb_idl_txn_write(row, column, datum);
+            free(datum);
+        }
     } else {
         struct ovsdb_datum datum;
 
@@ -1441,7 +1453,8 @@ cmd_set(struct ctl_context *ctx)
     }
 
     for (i = 3; i < ctx->argc; i++) {
-        ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab);
+        ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab,
+                                ctx->last_command);
         if (ctx->error) {
             return;
         }
@@ -1479,7 +1492,7 @@ cmd_add(struct ctl_context *ctx)
     const struct ovsdb_idl_column *column;
     const struct ovsdb_idl_row *row;
     const struct ovsdb_type *type;
-    struct ovsdb_datum old;
+    struct ovsdb_datum new;
     int i;
 
     ctx->error = get_table(table_name, &table);
@@ -1503,7 +1516,13 @@ cmd_add(struct ctl_context *ctx)
     }
 
     type = &column->type;
-    ovsdb_datum_clone(&old, ovsdb_idl_read(row, column));
+
+    if (ctx->last_command) {
+        ovsdb_datum_init_empty(&new);
+    } else {
+        ovsdb_datum_clone(&new, ovsdb_idl_read(row, column));
+    }
+
     for (i = 4; i < ctx->argc; i++) {
         struct ovsdb_type add_type;
         struct ovsdb_datum add;
@@ -1514,23 +1533,41 @@ cmd_add(struct ctl_context *ctx)
         ctx->error = ovsdb_datum_from_string(&add, &add_type, ctx->argv[i],
                                              ctx->symtab);
         if (ctx->error) {
-            ovsdb_datum_destroy(&old, &column->type);
+            ovsdb_datum_destroy(&new, &column->type);
             return;
         }
-        ovsdb_datum_union(&old, &add, type);
+        ovsdb_datum_union(&new, &add, type);
         ovsdb_datum_destroy(&add, type);
     }
-    if (old.n > type->n_max) {
+
+    if (!ctx->last_command && new.n > type->n_max) {
         ctl_error(ctx, "\"add\" operation would put %u %s in column %s of "
                   "table %s but the maximum number is %u",
-                  old.n,
+                  new.n,
                   type->value.type == OVSDB_TYPE_VOID ? "values" : "pairs",
                   column->name, table->name, type->n_max);
-        ovsdb_datum_destroy(&old, &column->type);
+        ovsdb_datum_destroy(&new, &column->type);
         return;
     }
-    ovsdb_idl_txn_verify(row, column);
-    ovsdb_idl_txn_write(row, column, &old);
+
+    if (ctx->last_command) {
+        /* Partial updates can only be made one by one. */
+        for (i = 0; i < new.n; i++) {
+            struct ovsdb_datum *datum = xmalloc(sizeof *datum);
+
+            ovsdb_datum_init_empty(datum);
+            ovsdb_datum_add_from_index_unsafe(datum, &new, i, type);
+            if (ovsdb_type_is_map(type)) {
+                ovsdb_idl_txn_write_partial_map(row, column, datum);
+            } else {
+                ovsdb_idl_txn_write_partial_set(row, column, datum);
+            }
+        }
+        ovsdb_datum_destroy(&new, &column->type);
+    } else {
+        ovsdb_idl_txn_verify(row, column);
+        ovsdb_idl_txn_write(row, column, &new);
+    }
 
     invalidate_cache(ctx);
 }
@@ -1769,7 +1806,7 @@ cmd_create(struct ctl_context *ctx)
     }
 
     for (i = 2; i < ctx->argc; i++) {
-        ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab);
+        ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab, false);
         if (ctx->error) {
             return;
         }
@@ -2620,7 +2657,8 @@ ctl_list_db_tables_usage(void)
 /* Initializes 'ctx' from 'command'. */
 void
 ctl_context_init_command(struct ctl_context *ctx,
-                         struct ctl_command *command)
+                         struct ctl_command *command,
+                         bool last)
 {
     ctx->argc = command->argc;
     ctx->argv = command->argv;
@@ -2629,6 +2667,7 @@ ctl_context_init_command(struct ctl_context *ctx,
     ds_swap(&ctx->output, &command->output);
     ctx->table = command->table;
     ctx->try_again = false;
+    ctx->last_command = last;
     ctx->error = NULL;
 }
 
@@ -2640,7 +2679,7 @@ ctl_context_init(struct ctl_context *ctx, struct ctl_command *command,
                  void (*invalidate_cache_cb)(struct ctl_context *))
 {
     if (command) {
-        ctl_context_init_command(ctx, command);
+        ctl_context_init_command(ctx, command, false);
     }
     ctx->idl = idl;
     ctx->txn = txn;
@@ -2684,7 +2723,7 @@ ctl_set_column(const char *table_name, const struct ovsdb_idl_row *row,
     if (error) {
         return error;
     }
-    error = set_column(table, row, arg, symtab);
+    error = set_column(table, row, arg, symtab, false);
     if (error) {
         return error;
     }

lib/db-ctl-base.h

@@ -239,9 +239,15 @@ struct ctl_context {
     /* A command may set this member to true if some prerequisite is not met
      * and the caller should wait for something to change and then retry. */
     bool try_again;
+
+    /* If set during the context initialization, command implementation
+     * may use optimizations that will leave database changes invisible
+     * to IDL, e.g. use partial set updates. */
+    bool last_command;
 };
 
-void ctl_context_init_command(struct ctl_context *, struct ctl_command *);
+void ctl_context_init_command(struct ctl_context *, struct ctl_command *,
+                              bool last);
 void ctl_context_init(struct ctl_context *, struct ctl_command *,
                       struct ovsdb_idl *, struct ovsdb_idl_txn *,
                       struct ovsdb_symbol_table *,

tests/ovs-vsctl.at

@@ -1071,9 +1071,13 @@ AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
 AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
   [1], [], [ovs-vsctl: cannot specify key to set for non-map column connection_mode
 ])
-AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
+AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y -- show])],
   [1], [], [ovs-vsctl: "add" operation would put 2 values in column datapath_id of table Bridge but the maximum number is 1
 ])
+AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])], [1], [], [stderr])
+AT_CHECK([sed "/^.*|WARN|.*/d" < stderr], [0], [dnl
+ovs-vsctl: transaction error: {"details":"set must have 0 to 1 members but 2 are present","error":"syntax error","syntax":"[[\"set\",[\"x\",\"y\"]]]"}
+])
 AT_CHECK([RUN_OVS_VSCTL([remove netflow `cat netflow-uuid` targets '"1.2.3.4:567"'])],
   [1], [], [ovs-vsctl: "remove" operation would put 0 values in column targets of table NetFlow but the minimum number is 1
 ])

utilities/ovs-vsctl.c

@@ -2711,9 +2711,9 @@ post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx)
 
 static void
 vsctl_context_init_command(struct vsctl_context *vsctl_ctx,
-                           struct ctl_command *command)
+                           struct ctl_command *command, bool last_command)
 {
-    ctl_context_init_command(&vsctl_ctx->base, command);
+    ctl_context_init_command(&vsctl_ctx->base, command, last_command);
     vsctl_ctx->verified_ports = false;
 }
 
@@ -2859,7 +2859,8 @@ do_vsctl(const char *args, struct ctl_command *commands, size_t n_commands,
     }
     vsctl_context_init(&vsctl_ctx, NULL, idl, txn, ovs, symtab);
     for (c = commands; c < &commands[n_commands]; c++) {
-        vsctl_context_init_command(&vsctl_ctx, c);
+        vsctl_context_init_command(&vsctl_ctx, c,
+                                   c == &commands[n_commands - 1]);
         if (c->syntax->run) {
             (c->syntax->run)(&vsctl_ctx.base);
         }

vtep/vtep-ctl.c

@@ -2207,9 +2207,9 @@ static const struct ctl_table_class tables[VTEPREC_N_TABLES] = {
 
 static void
 vtep_ctl_context_init_command(struct vtep_ctl_context *vtepctl_ctx,
-                              struct ctl_command *command)
+                              struct ctl_command *command, bool last_command)
 {
-    ctl_context_init_command(&vtepctl_ctx->base, command);
+    ctl_context_init_command(&vtepctl_ctx->base, command, last_command);
     vtepctl_ctx->verified_ports = false;
 
 }
@@ -2304,7 +2304,8 @@ do_vtep_ctl(const char *args, struct ctl_command *commands,
     }
     vtep_ctl_context_init(&vtepctl_ctx, NULL, idl, txn, vtep_global, symtab);
     for (c = commands; c < &commands[n_commands]; c++) {
-        vtep_ctl_context_init_command(&vtepctl_ctx, c);
+        vtep_ctl_context_init_command(&vtepctl_ctx, c,
+                                      c == &commands[n_commands - 1]);
         if (c->syntax->run) {
             (c->syntax->run)(&vtepctl_ctx.base);
         }