I think that is ok to remove that last pointer check in the generic comparison function. UUID is more than enough. Regards, Esteban
> -----Original Message----- > From: Lance Richardson [mailto:[email protected]] > Sent: jueves, 13 de julio de 2017 16:48 > To: Rodriguez Betancourt, Esteban <[email protected]> > Cc: Ben Pfaff <[email protected]>; [email protected]; Albornoz, Javier > <[email protected]>; Lutz, Arnoldo > <[email protected]> > Subject: Re: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > implementation > > > From: "Rodriguez Betancourt, Esteban" <[email protected]> > > To: "Lance Richardson" <[email protected]>, "Ben Pfaff" > > <[email protected]> > > Cc: [email protected], "Javier Albornoz" <[email protected]>, > > "Arnoldo Lutz" <[email protected]> > > Sent: Thursday, 13 July, 2017 5:22:42 PM > > Subject: RE: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > > implementation > > > > Hello, > > The UUID comparison was added to guarantee that always rows with the > > same keys are sorted in the same way (between executions of a > command, > > for example). Before that we used the pointer comparison (which gives > > us some randomness in sorting) but we kept it to be really really > > sure that we are deleting/inserting the correct row. > > > > The row sync effectively was intended for finding the correct row to > > delete when there are duplicated "index keys" and there are > > deletions/updates/inserts (note that the updates are really handled as > > a delete-and-reinsert). > > Hi Esteban, > > Thanks, I think all of that makes sense. I do wonder, though, since UUIDs > should be unique whether it would make just as much to assert that the > addresses are equal if the UUIDs are equal. > > > > > We copied ovsdb_idl_index_read from ovsdb_idl_read because we were > > concerned of what would happen if somebody access the indexes when a > > transaction is being made. If the index read the new values instead of > > the old one then the rows could be lost/wrongly sorted, etc. It is the > > same story with > > ovsdb_idl_index_write_() and index_set: we wanted the indexes to > > behave correctly during a transaction with new changes. > > > > Comparing the v4 version of ovsdb_idl_index_read() against the current > ovsdb_idl_read(), the only difference between the two that I can find is that > ovsdb_idl_index_read() takes the table class as a function parameter while > ovsdb_idl_read() expects row->table to be non-NULL and takes the table > class from row->table->class. Since ovsdb_idl_index_init_row() initializes > row->table appropriately, ovsdb_idl_read() should produce exactly the same > result as ovsdb_idl_index_read(), whether a transaction on that row is in > progress or not. So it seems to me we should be able to get rid of > ovsdb_idl_index_read(). > > > ovsdb_idl_index_{find,forward_to} are used inside the autogenerated > > functions for iterating over the indexes. > > We thought that it would be nice to be able to say things like: > > > > OVSREC_EXAMPLE_FOR_EACH_RANGE(row, cursor, start, end) /* [start, > end] > > */ OVSREC_EXAMPLE_FOR_EACH_RANGE(row, cursor, NULL, end) /* [0, > end] > > */ OVSREC_EXAMPLE_FOR_EACH_RANGE(row, cursor, start, NULL) /* > [start, > > "+infty"] */ OVSREC_EXAMPLE_FOR_EACH_RANGE(row, cursor, NULL, > NULL) /* > > everything */ > > > > Regards, > > Esteban > > Thanks, that makes sense. I think it would be good to add tests for these > cases. > > Thanks again! > > Lance > > > > > -----Original Message----- > > From: Lance Richardson [mailto:[email protected]] > > Sent: jueves, 13 de julio de 2017 14:29 > > To: Ben Pfaff <[email protected]> > > Cc: [email protected]; Rodriguez Betancourt, Esteban > > <[email protected]>; Albornoz, Javier <[email protected]>; jorge > > sauma <[email protected]>; Lutz, Arnoldo > > <[email protected]> > > Subject: Re: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > > implementation > > > > > > > > ----- Original Message ----- > > > From: "Ben Pfaff" <[email protected]> > > > To: "Lance Richardson" <[email protected]> > > > Cc: [email protected], [email protected], "javier albornoz" > > > <[email protected]>, "jorge sauma" > > > <[email protected]>, "arnoldo lutz guevara" > > > <[email protected]> > > > 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 <[email protected]> > > > > Co-authored-by: Lance Richardson <[email protected]> > > > > Signed-off-by: Lance Richardson <[email protected]> > > > > --- > > > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
