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
signature.asc
Description: PGP signature
