[ovs-dev] [PATCH 1/3] ovsdb-idl: Add support for on-demand columns

[Ben Pfaff]

On Tue, Mar 01, 2016 at 05:42:19PM +0000, Arguello, Sebastian wrote:
> The IDL only supports reading from columns that are being monitored.
> In the case where the column represent a frequently changing entity (e.g. counter),
> and the reads are relatively infrequent (e.g. CLI client), there is a
> significant overhead in replication.
> 
> This patch introduces a new column mode called OVSDB_IDL_ON_DEMAND.
> An on-demand column will have its default value if it has never been updated.
> This can happen if: 1) the user has not called explicitly a fetch operation
> over it or 2) the server reply with the actual value has not been
> received/processed (i.e. ovsdb_idl_run() has not been called). The on-demand
> columns will keep the last value received from the OVSDB server.
> 
> The on-demand columns are not updated by the IDL automatically, they are
> updated when the IDL user asks it by the calling one of the new fetching
> functions that were added to the IDL. When this happens, the IDL sends a select
> operation to request the data from the server. After calling ovsdb_idl_run(),
> the IDL updates the replica with the information received from the server.
> 
> With this new column mode, the state of the replica could diverge from the
> state of the database, as some of the columns could be outdated. The process
> using the IDL is responsible for requesting the information before using it.
> 
> The main user visible changes in this patch are:
>   - There is a new function that adds on-demand columns:
>     ovsdb_idl_add_on_demand_column()
>   - Functions for fetching a cells (columns for specific rows),
>     columns, and table were added: ovsdb_idl_fetch_row(),
>     ovsdb_idl_fetch_column(), and ovsdb_idl_fetch_table()
>   - Functions for verifying if the fetch requests of on-demand columns
>     were processed were added: ovsdb_idl_is_row_fetch_pending(),
>     ovsdb_idl_is_column_fetch_pending(), ovsdb_idl_is_table_fetch_pending()
>   - When an on-demand column is updated, the IDL seqno is changed as well
> 
> Note that the Python IDL already has a feature similar to this called
> Read-only columns.
> 
> Signed-off-by: Sebastian Arguello <sebastian.arguello at hpe.com>
> ---
> This is the pull request with this change: https://github.com/openvswitch/ovs/pull/110

Thanks for working on this.  I think it will have useful applications.
I have some comments.

I believe that there is a race between requesting a fetch for a column
or a table and insertion of a new row: if client 1 requests a fetch for
a column or table and then client 2 inserts a new row, then client 1
will see a row without the requested data.  I think that's just the way
things work and users of the IDL need to expect it.  If so, I think that
it should be documented in the comments for the functions in question,
so that users of the IDL don't get any surprises.

I don't see anything to handle the case where the database connection
drops.  If that happens, then any outstanding requests will never
receive replies, so all the pending-fetch counters need to be reset and
the outstanding fetch requests need to be discarded (or, alternatively,
they could be re-issued).

The code in ovsdb_idl_run() and ovsdb_idl_parse_fetch_reply() seems to
assume that if the JSON hash value can be found in the outstanding fetch
requests, then there's no need to compare the actual JSON.  I don't see
why this is safe.

The log messages refer to <fetch_reply>, but that's not one of the
nonterminals in the context-free grammar for OVSDB as are the other uses
of the <name> notation.

It's not clear to me why struct ovsdb_idl_fetch_node's 'columns' member
is a shash instead of just an array of pointers to ovsdb_idl_column.  It
doesn't look to me like anything ever uses the 'name' part of each shash
node to do a lookup; instead, the only use of this data structure is to
iterate over the set of columns.

I see a few log messages that use raw errno values, like this:
        VLOG_WARN_RL(&syntax_rl,
                     "Error while sending column fetch request (%d)", status);
Please use ovs_strerror() to convert these to human-readable messages.

The ovsdb_idl_fetch_*() functions have some code in common.  Is there a
way to factor some of this out, in a natural way, to reduce the amount
of common code?

Thanks,

Ben.