On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote:
> When inserting data into a "singleton" table (one that has maxRows ==
> 1), there is a check that ensures that the table is currently empty
> before inserting the row. The intention is to prevent races where
> multiple clients might attempt to insert rows at the same time.
>
> The problem is that this singleton check can cause legitimate
> transactions to fail. Specifically, a transaction that attempts to
> delete the current content of the table and insert new data will cause
> the singleton check to fail since the table currently has data.
>
> This patch corrects the issue by keeping a count of the rows being
> deleted and added to singleton tables. If the total is larger than zero,
> then the net operation is attempting to insert rows. If the total is
> less than zero, then the net operation is attempting to remove rows. If
> the total is zero, then the operation is inserting and deleting an equal
> number of rows (or is just updating rows). We only add the singleton
> check if the total is larger than zero.
>
> This patch also includes a new test for singleton tables that ensures
> that the maxRows constraint works as expected.
>
> Signed-off-by: Mark Michelson <[email protected]>
Good catch. (It's kind of weird to delete and repopulate a singleton
table, but we should support it.)
I think that the following patch achieves the same end with less
bookkeeping overhead. What do you think? It doesn't break anything in
the testsuite, but I didn't check that it actually achieves the purpose.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index c6ff78e25a04..1453025acbd6 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3929,6 +3929,39 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
/* Add updates. */
any_updates = false;
+
+ /* For tables constrained to have only a single row (a fairly common OVSDB
+ * pattern for storing global data), identify whether we're inserting a
+ * row. If so, then verify that the table is empty before inserting the
+ * row. This gives us a clear verification-related failure if there was an
+ * insertion race with another client. */
+ for (size_t i = 0; i < txn->db->class_->n_tables; i++) {
+ struct ovsdb_idl_table *table = &txn->db->tables[i];
+ if (table->class_->is_singleton) {
+ /* Count the number of rows in the table before and after our
+ * transaction commits. This is O(n) in the number of rows in the
+ * table, but that's OK since we know that the table should only
+ * have one row. */
+ size_t initial_rows = 0;
+ size_t final_rows = 0;
+ HMAP_FOR_EACH (row, hmap_node, &table->rows) {
+ initial_rows += row->old_datum != NULL;
+ final_rows += row->new_datum != NULL;
+ }
+
+ if (initial_rows == 0 && final_rows == 1) {
+ struct json *op = json_object_create();
+ json_array_add(operations, op);
+ json_object_put_string(op, "op", "wait");
+ json_object_put_string(op, "table", table->class_->name);
+ json_object_put(op, "where", json_array_create_empty());
+ json_object_put(op, "timeout", json_integer_create(0));
+ json_object_put_string(op, "until", "==");
+ json_object_put(op, "rows", json_array_create_empty());
+ }
+ }
+ }
+
HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) {
const struct ovsdb_idl_table_class *class = row->table->class_;
@@ -3947,23 +3980,6 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
struct json *row_json;
size_t idx;
- if (!row->old_datum && class->is_singleton) {
- /* We're inserting a row into a table that allows only a
- * single row. (This is a fairly common OVSDB pattern for
- * storing global data.) Verify that the table is empty
- * before inserting the row, so that we get a clear
- * verification-related failure if there was an insertion
- * race with another client. */
- struct json *op = json_object_create();
- json_array_add(operations, op);
- json_object_put_string(op, "op", "wait");
- json_object_put_string(op, "table", class->name);
- json_object_put(op, "where", json_array_create_empty());
- json_object_put(op, "timeout", json_integer_create(0));
- json_object_put_string(op, "until", "==");
- json_object_put(op, "rows", json_array_create_empty());
- }
-
struct json *op = json_object_create();
json_object_put_string(op, "op",
row->old_datum ? "update" : "insert");
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
