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:lrich...@redhat.com] > Sent: jueves, 13 de julio de 2017 16:48 > To: Rodriguez Betancourt, Esteban <esteb...@hpe.com> > Cc: Ben Pfaff <b...@ovn.org>; d...@openvswitch.org; Albornoz, Javier > <javier.albor...@hpe.com>; Lutz, Arnoldo > <arnoldo.lutz.guev...@hpe.com> > Subject: Re: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > implementation > > > From: "Rodriguez Betancourt, Esteban" <esteb...@hpe.com> > > To: "Lance Richardson" <lrich...@redhat.com>, "Ben Pfaff" > > <b...@ovn.org> > > Cc: d...@openvswitch.org, "Javier Albornoz" <javier.albor...@hpe.com>, > > "Arnoldo Lutz" <arnoldo.lutz.guev...@hpe.com> > > 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:lrich...@redhat.com] > > Sent: jueves, 13 de julio de 2017 14:29 > > To: Ben Pfaff <b...@ovn.org> > > Cc: d...@openvswitch.org; Rodriguez Betancourt, Esteban > > <esteb...@hpe.com>; Albornoz, Javier <javier.albor...@hpe.com>; jorge > > sauma <jorge.sa...@hpe.com>; Lutz, Arnoldo > > <arnoldo.lutz.guev...@hpe.com> > > Subject: Re: [ovs-dev] [PATCH v5 3/4] ovsdb-idl: idl compound indexes > > implementation > > > > > > > > ----- 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
