----- Original Message ----- > From: "Ben Pfaff" <b...@ovn.org> > To: "Lance Richardson" <lrich...@redhat.com> > Cc: d...@openvswitch.org, esteb...@hpe.com, "javier albornoz" > <javier.albor...@hpe.com>, "jorge sauma" > <jorge.sa...@hpe.com>, "arnoldo lutz guevara" <arnoldo.lutz.guev...@hpe.com> > Sent: Tuesday, 11 July, 2017 5:05:32 PM > Subject: Re: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > implementation > > On Sat, Jun 24, 2017 at 05:01:51PM -0400, Lance Richardson wrote: > > This patch adds support for the creation of multicolumn indexes > > in the C IDL to enable for efficient search and retrieval of database > > rows by key. > > > > Signed-off-by: Esteban Rodriguez Betancourt <esteb...@hpe.com> > > Co-authored-by: Lance Richardson <lrich...@redhat.com> > > Signed-off-by: Lance Richardson <lrich...@redhat.com> > > --- > > v5: - Coding style fixes (checkpatch.py) > > - Fixed memory leak (missing ovsdb_datum_destroy() in > > ovsdb_idl_index_destroy_row__()). > > - Some polishing of comment and log message text. > > Thanks for reviving this series. > > I don't understand ovsdb_idl_index_read(). It is almost the same as > ovsdb_idl_read(). It looks like ovsdb_idl_read() could be implemented > as a wrapper around it, but I'm also not sure why ovsdb_idl_read() can't > be used directly. Also, I don't understand its comment about "index_set > functions", since there are no functions with index_set in their names.
Hi Ben, I neglected to respond to the comment about "index_set functions" in my previous response, these are automatically generated for the IDL by the next patch. I had deleted that comment in v6 of this series (which is in the ml archive but not patchwork for some reason), it didn't seem useful to me. > > ovsdb_idl_index_write_() is mostly copy-paste of a part of > ovsdb_idl_txn_write__(), so to avoid code duplication it would be best > to factor that code out of ovsdb_idl_txn_write__() and call it from both > places. > > I don't understand the behavior of ovsdb_idl_index_find() and > ovsdb_idl_index_forward_to() for a null 'value' argument. Is there some > idiomatic usage where the null and nonnull behaviors work out nicely? > > The row_sync behavior is confusing. I remember it slightly from the > previous iteration but I don't remember being convinced it was the best > way to do things. > The "row_sync" stuff seems to be a matter of using additional keys (row uuid values first, then memory addresses) when inserting or deleting a row in order to handle duplicate keys. This still seems a bit strange, but after thinking about it a bit I believe it is a reasonable approach; these extra keys aren't used when searching, so we will always get the first entry in the list when there are multiple rows with the same key, and when inserting/deleting we still get O(log N) performance when there are many entries with identical keys by searching on the UUID value. But "row_sync" isn't at all useful in figuring out the above, nor are any of the comments in this area. I also wonder if the comparison on memory address is needed... is there any real possibility that two rows in a table will have the same UUID? Do you think reworking the "row_sync" name and adding better comments would be an acceptable way forward here? Thanks, Lance > Thanks, > > Ben. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev