Re: pgbench tap tests & minor fixes

В письме от 9 мая 2017 17:12:04 Вы написали:

Hi! Sorry for big delay. I am back now, I hope.

> > We will have to move thread->stats.cnt++; then. If we decided to move st-
> > 
> >> cnt++;. There would not be great harm, as  thread->stats.cnt++; is also
> >> done> 
> > in the code outside of processXactStats.
> > 
> > So it is just my idea to made the code better. May be there is good reason
> > for keeping it inside processXactStats, I just can't see. What do you
> > think about it?
> 
> I understand.
> 
> I wanted to have *one* single functions where stats are collected, the
> situation before was not too clear about where & when the stats where
> collected. There was some pain to avoid useless cycles, so the function
> was skipped in some cases.
> 
> I've modified the code so that the processXactStats function is always
> called, but just does the counting and skip everything else when detailed
> stats are not needed. I think it is a good compromise between simplicity
> and performance. See attached. Hopefully it will be clearer this way.
Year, this is much more clear for me. Now I understand this statistics code.

I still have three more questions. A new one: 

========
    my_command->line = expr_scanner_get_substring(sstate,
                                                  start_offset,
-                                                 end_offset);
+                                                 end_offset + 1);
========

I do not quite understand what are you fixing here, I did not find any mention 
of it in the patch introduction letter. And more, expr_scanner_get_substring 
is called twice in very similar code, and it is fixed only once. Can you tell 
more about this fix.

Old one:

(nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */

and

if (progress_timestamp && progress <= 0)


I am still sure that the right way is to do '== 0' and Assert for case when it 
is negative.

If you are sure it is good to do '<= 0', let's allow commiter to do final 
decision.

And another unclosed issue:

I still do not like command_checks_all function name (I would prefer 
command_like_more) but I can leave it for somebody more experienced (i.e. 
commiter) to make final decision, if you do not agree with me here...

/* Why I am so bothered with function name. We are adding this function to 
library that are used by all TAP-test-writers. So here things should be 100% 
clear for all. If this function was only in pgbench test code, I would not 
care about the name at all. But here I do. I consider it is important to give 
best names to the functions in shared libraries. */


Hope these are last one. Let's close the first issue, fix or leave unclosed 
others, and finish with this patch :-)

-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.