On Tue, Jun 15, 2021 at 09:53:06PM +0900, Yugo NAGATA wrote:
> I am fine with this version, but I think it would be better if we have a comment
> explaining what "tx" is for.
>
> Also, how about adding Assert(tx) instead of using "else if (tx)" because
> we are assuming that tx is always true when agg_interval is not used, right?
Agreed on both points. From what I get, this code could be clarified
much more, and perhaps partially refactored to have less spaghetti
code between the point where we call it at the end of a thread or when
gathering stats of a transaction mid-run, but that's not something to
do post-beta1. I am not completely sure that the result would be
worth it either.
Let's document things and let's the readers know better the
assumptions this area of the code relies on, for clarity. The
dependency between agg_interval and sample_rate is one of those
things, somebody needs now to look at the option parsing why only one
is possible at the time. Using an extra tx flag to track what to do
after the loop for the aggregate print to the log file is an
improvement in this direction.
--
Michael