> 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.
>
I would guess that the original reason had to do with the fact that
ovsdb_idl_index_read() needs to deal with "real" rows in the db replica as
well as external row instances created by ovsdb_idl_index_init_row() to
contain the key values being searched for. Looking at the code a little
more closely, I see that ovsdb_idl_index_init_row() sets row->table, so
the key row structure would not be considered "synthetic" according
to ovsdb_idl_row_is_synthetic().
So: I think you are correct, we should be able to use ovsdb_idl_read() and
eliminate ovsdb_idl_index_read().
> 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.
>
Hmm, I just noticed that this is v5, v6 seems to be missing in patchwork.
The v6 version of this function is a bit more streamlined, and avoids
calling ovsdb_datum_destroy() so that a row structure containing a key
with a string type can avoid having to xstrdup() key strings:
void
ovsdb_idl_index_write_(struct ovsdb_idl_row *const_row,
const struct ovsdb_idl_column *column,
struct ovsdb_datum *datum,
const struct ovsdb_idl_table_class *class)
{
struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, const_row);
size_t column_idx = column - class->columns;
if (bitmap_is_set(row->written, column_idx)) {
free(row->new[column_idx].values);
free(row->new[column_idx].keys);
} else {
bitmap_set1(row->written, column_idx);
}
row->new[column_idx] = *datum;
(column->unparse)(row);
(column->parse)(row, &row->new[column_idx]);
}
So there's a little less commonality between the two.
> 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?
>
I don't know what the intended use was, but there doesn't currently seem
to be a use of the null 'value' case (generated code always passes
&data->header_ for this parameter, which could be null if 'data' is null,
but that seems pretty fragile if intentional).
I'll see if the special-case handling of null values can be eliminated.
> 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.
>
Hmm, that does seem strange/confusing. I will have to spend more time
studying that bit of conde.
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
