[ovs-dev] [PATCH 3/5] ovsdb-idl: Redesign use of indexes.

[Ben Pfaff]

On Mon, Jun 11, 2018 at 04:16:06PM -0400, Mark Michelson wrote:
> So far all I've reviewed is the documentation. See my comments below.
> 
> On 06/08/2018 05:59 PM, Ben Pfaff wrote:
> >The design of the compound index feature in the C OVSDB IDL was unusual.
> >Indexes were generally referenced only by name rather than by pointer, and
> >could be obtained only from the top-level ovsdb_idl object.  To iterate or
> >otherwise search an index required explicitly creating a special
> >ovsdb_idl_cursor object, which at least seemed somewhat heavy-weight given
> >that it required a string lookup in a table of indexes.
> >
> >This commit redesigns the compound index interface.  It discards the use of
> >names for indexes, instead having clients pass in a pointer to the index
> >object itself.  It simplifies how indexes are created, gets rid of the need
> >for explicit cursor objects, and updates all of the users to the new
> >interface.
> >
> >The underlying reason for this commit is to make it easier in
> >ovn-controller to keep track of the dependencies for a given function, by
> >making the indexes explicit arguments to any function that needs to use
> >them.
> >
> >Signed-off-by: Ben Pfaff <blp at ovn.org>
> >+one of the simpler convenience functions ``ovsdb_idl_index_create`()`` or
> >+``ovsdb_idl_index_create2()``.  All indexes must be created before the first
> >+call to ovsdb\_idl\_run().
> 
> I think there's a typo in the first sentence. I think it's supposed to say:
> 
> "or one of the simpler convenience functions ovsdb_idl_index_create1() or
> ovsdb_idl_index_create2()."

Thanks, fixed.

> >  Index Creation Example
> >  ^^^^^^^^^^^^^^^^^^^^^^
> >@@ -185,19 +180,15 @@ Index Creation Example
> >          ovsdb_idl_add_column(*idl, &ovsrec_test_col_enumField);
> >          ovsdb_idl_add_column(*idl, &ovsrec_test_col_boolField);
> >-        /* Create an index.
> >-         * This index is created using (stringField, numericField) as key.
> >-         * Also shows the usage of some arguments of add column, although
> >-         * for a string column it is unnecesary to pass a custom comparator.
> >-         */
> >-        struct ovsdb_idl_index *index;
> >-        index = ovsdb_idl_create_index(*idl, &ovsrec_table_test,
> >-                                       "by_stringField");
> >-        ovsdb_idl_index_add_column(index, &ovsrec_test_col_stringField,
> >-                                   OVSDB_INDEX_ASC, stringField_comparator);
> >-        ovsdb_idl_index_add_column(index, &ovsrec_test_col_numericField,
> >-                                   OVSDB_INDEX_DESC, NULL);
> >-        /* Done. */
> >+        struct ovsdb_idl_index_column columns[] = {
> >+            { .column = &ovsrec_test_col_stringField,
> >+              .comparer = stringField_comparator },
> >+            { .column = &ovsrec_test_col_numericField,
> >+              .order = OVSDB_INDEX_DESC },
> >+        };
> 
> I don't like the change to using struct initializers over function calls for
> initialization. It's possible to create an ovsdb_idl_index_column without
> specifying an ordering or comparator, which can lead to hard-to-debug
> issues. 

That's actually intentional because most of the time the defaults (NULL
and 0 aka ascending order) are what you want and it just adds visual
noise to specify them explicitly.  The two examples above take advantage
of the defaults.

> I suggest hiding ovsdb_idl_index_column initialization behind
> functions. This forces the user to specify an ordering or comparator
> when applicable.

...

> >-1. Create a cursor.
> >  2. Create index row objects with index columns set to desired search key
> >     values (one is needed for equality iterators, two for range iterators,
> >     a search key is not needed for the full index iterator).
> >-3. Pass the cursor, an iteration variable, and the key values to the iterator.
> >+3. Pass the index, an iteration variable, and the key values to the iterator.
> >  4. Use the values within iterator loop.
> 
> This list now starts with item 2.

Oops.  Fixed.