Thanks Aaron, that is a valid point.
As the AI review suggested, currently there is no path that would lead to
silently
dropping the history.
The problem might occur in the future if someone would call
ovsdb_txn_history_init
with history enabled (need_txn_history sets to true) and forgot to call
ovsdb_txn_history_update. That would leave txn_history_time_max at 0 and shrink
the history to a single entry.
To solve this potential problem we could - as the AI review suggested - add this
to the documentation or change the ovsdb_txn_history_init function to
void
ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history,
long long int txn_history_time_max)
The downside of this solution is that a caller must provide a history time
limit
even if the need_txn_history is set to false.
I have the v2 patch with changed ovsdb_txn_history_init but I will wait for the
general
review first.
Thanks,
Tomasz
środa, 30 września 2026 19:50, Aaron Conole <[email protected]> napisał(a):
> Tomasz Wałaszek via dev <[email protected]> writes:
>
> > Currently, the transaction history keeps a fixed number of transactions,
> > hardcoded to 100. On a busy system this may cover only a short period
> > of time, so a client that disconnects has a high chance of not finding
> > its last transaction in the history, which leads to a download of the
> > whole database.
> >
> > This patch replaces the fixed number of transactions with a time based
> > limit. Transactions are removed from the history once they are older
> > than the limit, regardless of how many of them are in the history. The
> > limit defaults to 60 seconds and can be changed with a new database
> > configuration option 'transaction-history-time-limit' for clustered and
> > relay databases. The value is in seconds.
> >
> > The limit on the number of atoms, which prevents the transaction history
> > from growing larger than the database itself, still applies.
> >
> > Reported-at:
> > https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431593.html
> > Signed-off-by: Tomasz Wałaszek <[email protected]>
> > ---
>
> [ ... ]
>
> > @@ -1725,3 +1734,11 @@ ovsdb_txn_history_destroy(struct ovsdb *db)
> > db->n_txn_history = 0;
> > db->n_txn_history_atoms = 0;
> > }
> > +
> > +void
> > +ovsdb_txn_history_update(struct ovsdb *db, long long int
> > txn_history_time_max)
> > +{
> > + ovs_assert(txn_history_time_max >= 0);
> > + db->txn_history_time_max = txn_history_time_max;
> > + ovsdb_txn_history_run(db);
>
> An AI generated this comment, and it looked relevant to a human reviewer
> so it is forwarded here.
>
> Concern (defensive): `ovsdb_txn_history_init()` does not initialize
> `txn_history_time_max`; it relies on `xzalloc` in `ovsdb_create()`
> leaving it 0 and on callers calling `ovsdb_txn_history_update()`. Today
> every path that enables history goes through ovsdb-server's
> `open_db()`/`database_update_config()` (verified: relay.c:310 re-enable
> keeps the already-set value, `ovsdb_replace()` only destroys nodes), so
> there is no live bug. But a 0 value means "expire everything older than
> this millisecond", i.e., history collapses to one entry — a future
> caller of `ovsdb_txn_history_init(db, true)` that forgets the update
> call would get silently degenerate behavior. Setting a sane default in
> `init()` (or documenting the dependency) would be safer. Low severity.
>_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
