> 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