On Mon, Jun 3, 2024 at 2:01 PM Ilya Maximets <[email protected]> wrote: > > On 6/3/24 06:20, Mike Pattrick wrote: > > Currently all OVSDB database queries except for UUID lookups all result > > in linear lookups over the entire table, even if an index is present. > > > > This patch modifies ovsdb_query() to attempt an index lookup first, if > > possible. If no matching indexes are present then a linear index is > > still conducted. > > > > Reported-at: https://issues.redhat.com/browse/FDP-590 > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > NEWS | 3 ++ > > ovsdb/query.c | 102 +++++++++++++++++++++++++++++++++++---- > > ovsdb/row.h | 28 +++++++++++ > > ovsdb/transaction.c | 27 ----------- > > tests/ovsdb-execution.at | 34 ++++++++++++- > > tests/ovsdb-server.at | 2 +- > > tests/ovsdb-tool.at | 2 +- > > 7 files changed, 159 insertions(+), 39 deletions(-) > > Hi, Mike. Thanks for the patch. > > Besides what Simon asked, the patch has a few other issues: > > 1. Lookup is performed only on the committed index and it doesn't include > rows that are in-flight in the current transaction. > > Unlike rows in a hash table, indexes are updated only after the whole > transaction is committed. With this change we'll not be able to find > newly added rows. > > Another thing related to this is that it is allowed to have duplicates > within a transaction as long as they are removed before the transaction > ends. So it is possible that multiple rows will satisfy the condition > on indexed columns while the transaction is in-flight. > > Consider the following commands executed in a sandbox: > > # ovs-vsctl set-manager "tcp:my-first-target" > # ovsdb-client transact unix:$(pwd)/sandbox/db.sock ' > ["Open_vSwitch", > {"op": "select", > "table": "Manager", > "columns": ["_uuid", "target"], > "where": [["target", "==", "tcp:my-first-target"]]}, > {"op": "insert", > "table": "Manager", > "uuid-name": "duplicate", > "row": {"target": "tcp:my-first-target"}}, > {"op": "select", > "table": "Manager", > "columns": ["_uuid", "target"], > "where": [["target", "==", "tcp:my-first-target"]]}, > {"op": "delete", > "table": "Manager", > "where":[["_uuid","==",["named-uuid","duplicate"]]]}, > {"op": "select", > "table": "Manager", > "columns": ["_uuid", "target"], > "where": [["target", "==", "tcp:my-first-target"]]}]' > > Transaction must succeed. The first selection should return 1 row, > the second should return both duplicates and the third should again > return one row.
This is a good point, I hadn't anticipated this use-case but it does have a large impact on this change. After working through a few implementations, I wasn't able to find a solution that wasn't overly complex. For the next version, I've instead opted to exclude indexed lookups from transactions that modify the associated row. The next version should address this and the other feedback. Cheers, M > > Ideally, implementation should not leak the transaction details to > the query module, though I'm not sure if that is 100% achievable. > > 2. Taking above case into account, this change needs way more unit tests > with different order of operations and complex data updates. > > 3. Since this is a performance-oriented change, please, include some > performance numbers in the commit message as well, including impact > on non-indexed lookups, if any. > > 4. There seems to be a lot of logic overlap with existing functions like > ovsdb_condition_match_every_clause(), ovsdb_index_search() and > ovsdb_row_hash_columns(). Can we re-use those instead? For example, > by creating a row from the conditions before the lookup? What a > performance impact will look like? > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
