Re: pgbench tap tests & minor fixes

В письме от 28 апреля 2017 22:39:41 пользователь Fabien COELHO написал:

> Here is a v3, with less files. I cannot say I find it better, but it
> still works.

Actually I like it. It is what I expect it to be ;-)

For me it is more readable then before, as all the data that is used for
testing is on the same screen and I can see it all in one place.

I would also like to hear from Robert, what does he think about it, but I like
the perl code as it is.

> The "command_likes" function has been renamed "command_checks".

This is one thing have my doubts about. There is no way to understand what the
difference between command_likes and command_checks. One can't get while
reading the name. There is no way to understand that these functions do almost
same things, but one of them makes better checks.

My perl experience tells me that this new function should be called
command_likes_more. It is a kind of tradition in test naming things "simple,
more, most, etc". It is not English readable name, but any perl developer will
understand it's meaning.

I would not insist on this. It is a kind of suggestion.

Now to the second part. Now I payed attention to the C part of the patch. And
I have questions and notes.

1. I quite agree with Robert, that this options patch is a separate issue and
it would be better do deal with them separately. But I wild not insist. It
will just made our work on this commit longer, instead of two shorter works.

2. I do not completely understand what you have done in this patch, as this
code is not familiar to me so I would ask questions.

2.1
if (latency_limit)
{
int64 now_us;

if (INSTR_TIME_IS_ZERO(now))
INSTR_TIME_SET_CURRENT(now);
now_us = INSTR_TIME_GET_MICROSEC(now);
while (thread->throttle_trigger < now_us - latency_limit
&&
/* with -t, do not overshoot */
(nxacts <= 0 || st->cnt < nxacts))
{
processXactStats(thread, st, &now, true, agg);
/* next rendez-vous */
wait = getPoissonRand(thread, throttle_delay);
thread->throttle_trigger += wait;
st->txn_scheduled = thread->throttle_trigger;
}

if (nxacts > 0 && st->cnt >= nxacts)
{
st->state = CSTATE_FINISHED;
break;
}
}

First of all I do not completely understand this while and your changes in it.

It is inside doCustom function that is used to run a bunch of transactions.

There is for (;;) in it that is dedicated to run one transaction after
another (it is state machine, so each iteration is one state change, but
several state changes leads to running one transaction after anther
nevertheless)

This while() is inside... Originally It detects that the transaction is late,
"counts" how many throttle_delay will fit in this total delay, and for each
throttle_delay do some logging in processXactStats and counts thread->
latency_late total number there too.

Please note that in original code thread->stats.cnt++; is one only when not
(progress || throttle_delay || latency_limit)

And you've added st->cnt ++; with no conditions. So if transaction is delayed
for N*throttle_delay miliseconds, your st->cnt will be incremented N times.

Are you sure it is what you want to do? If so, I need more explanations
because I do not understand why do you want to count it that way, and what
exactly do you count here.

So more about this while:

Why do you check nxacts > 0? It _must_ be grater then 0.
I think it one want to be sure that some value is valid, he uses Assert.

For me it is like that: if I check using "if" that nxacts > 0 then there is
legal possibility that nxacts has negative value. If i do Assert, there is no
legal possibility for it, but I just want to be sure.

More notes about code styling:

while (thread->throttle_trigger < now_us - latency_limit &&
/* with -t, do not overshoot */
(nxacts <= 0 || st->cnt < nxacts))

and

else
/* just count */
thread->stats.cnt++;

I've do not recall seeing in postgres code new line comments in the middle of
loop condition, and in the middle of if/else statement if there is no {}
there... It seems to me that comments here should go on the same line at the
right. (If I am not right, please fellows, correct me)
May be it is a reason for checking coding style for this case.

The second style issue:

else
thread->stats.cnt++, st->cnt++;

I do not recall using comma operation on cases like these, in postgres code. I
think
else
{
thread->stats.cnt++;
st->cnt++;
}

would be much more readable. (Fellows, please correct me if I am wrong)

As for the comments, there is great utility pg_bsd_indent that goes with
postgres code. You can write comment like

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

not caring about the length of the line, and then run pg_bsd_indent. It will
reformat the code as it should be.

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