Re: [HACKERS] 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.