pgbench tap tests & minor fixes

В письме от 17 апреля 2017 14:51:45 пользователь Fabien COELHO написал:

When developping new features for pgbench, I usually write some tests
which are lost when the feature is committed. Given that I have submitted
some more features and that part of pgbench code may be considered for
sharing with pgsql, I think that improving the abysmal state of tests
would be a good move.

Hi! Since I used to be good at perl, I like tests, and I've dealt with
postgres TAP tests before, I think I can review you patch. If it is OK for
everyone.

For now I've just gave this patch a quick look through... But nevertheless I
have something to comment:

The attached patch:

(1) extends the existing perl tap test infrastructure with methods to test
pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes"
which allows to check for expectations.

I do not think it is good idea adding this functions to the PostgresNode.pm.
They are pgbench specific. I do not think anybody will need them outside of
pgbench tests.

Generally, these functions as they are now, should be placed is separate .pm
file. like it was done in src/test/ssl/

May be you should move some code into some kind of helpers and keep them in
PostgresNode.pm.

Or write universal functions that can be used for testing any postgres console
tool. Then they can be placed in PostgresNode.pm

(3) add a lot of new very small tests so that coverage jumps from very low
to over 90% for source files. I think that derived files (exprparse.c,
exprscan.c) should be removed from coverage analysis.

Previous coverage status:

exprparse.y 0.0 % 0 / 77 0.0 % 0 / 8
exprscan.l 0.0 % 0 / 102 0.0 % 0 / 8
pgbench.c 28.3 % 485 / 1716 43.1 % 28 / 65

New status:

exprparse.y 96.1 % 73 / 76 100.0 % 8 / 8
exprscan.l 92.8 % 90 / 97 100.0 % 8 / 8
pgbench.c 90.4 % 1542 / 1705 96.9 % 63 / 65

The test runtime is about doubled on my laptop, which is not too bad given
the coverage achieved.

What tool did you use to calculate the coverage?

Lots of small test looks good at first glance, except the point that adding
lots of small files with pgbench scripts is not great idea. This makes code
tree too overloaded with no practical reason. I am speaking of
src/bin/pgbench/t/scripts/001_0* files.

I think all the data from this file should be somehow imported into
001_pgbench.pl and these files should be created only when tests is running,
and then removed when testing is done.

I think that job for creating and removing these files should be moved to
pgbench and pgbench_likes. But there is more then one way to do it. ;-)

That's what I've noticed so far... I will give this patch more attentive look
soon.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Nikolay,

Hi! Since I used to be good at perl, I like tests, and I've dealt with
postgres TAP tests before, I think I can review you patch. If it is OK for
everyone.

I think that all good wills are welcome, especially concerning code
reviews.

For now I've just gave this patch a quick look through... But nevertheless I
have something to comment:

The attached patch:

(1) extends the existing perl tap test infrastructure with methods to test
pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes"
which allows to check for expectations.

I do not think it is good idea adding this functions to the PostgresNode.pm.

I thought it was:-)

They are pgbench specific.

Sure.

I do not think anybody will need them outside of pgbench tests.

Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it
is not to test pgbench itself. Pgbench allows to run some programmable
clients in parallel very easily, which cannot be done simply otherwise. I
think that having it there would encourage such uses.

Another point is that the client needs informations held as attributes in
the node in order to connect to the server. Having it outside would mean
picking the attributes inside, which is pretty unclean as it breaks the
abstraction. For me, all pg standard executables could have their methods
in PostgresNode.pm.

Finally, it does not cost anything to have it there.

Generally, these functions as they are now, should be placed is separate .pm
file. like it was done in src/test/ssl/

I put them there because it already manages both terminal client (psql) &
server side things (initdb, postgres...). Initially pgbench was a
"contrib" but it has been moved as a standard client a few versions ago,
so for me it seemed logical to have the support there.

May be you should move some code into some kind of helpers and keep them in
PostgresNode.pm.

Hmmm. I can put a "run any client" function with the same effect and pass
an additional argument to tell that pgbench should be run, but this looks
quite contrived for no special reason.

Or write universal functions that can be used for testing any postgres console
tool. Then they can be placed in PostgresNode.pm

There are already "psql"-specific functions... Why not pgbench specific
ones?

(3) add a lot of new very small tests so that coverage jumps from very low
to over 90% for source files. [...]

What tool did you use to calculate the coverage?

I followed the documentated recipee:

https://www.postgresql.org/docs/devel/static/regress-coverage.html

Lots of small test looks good at first glance, except the point that adding
lots of small files with pgbench scripts is not great idea. This makes code
tree too overloaded with no practical reason. I am speaking of
src/bin/pgbench/t/scripts/001_0* files.

I think all the data from this file should be somehow imported into
001_pgbench.pl and these files should be created only when tests is running,
and then removed when testing is done.

Hmmm. I thought of that but was very unconvinced by the approach: pgbench
basically always requires a file, so what the stuff was doing was writting
the script into a file, checking for possible errors when doing so, then
unlinking the file and checking for errors again after the run. Then you
have to do some escaping the the pgbench script themselves, and the perl
script becomes pretty horrible and unreadable with plenty of perl, SQL,
backslash commands in strings... Finally, if the script is inside the perl
script it makes it hard to run the test outside of it when a problem is
found, so it is a pain.

I think that job for creating and removing these files should be moved to
pgbench and pgbench_likes. But there is more then one way to do it. ;-)

Hmmm. See above.

That's what I've noticed so far... I will give this patch more attentive look
soon.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 20 апреля 2017 19:14:34 пользователь Fabien COELHO написал:

(1) extends the existing perl tap test infrastructure with methods to
test
pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes"
which allows to check for expectations.

I do not think it is good idea adding this functions to the
PostgresNode.pm.

I thought it was:-)

They are pgbench specific.

Sure.

I do not think anybody will need them outside of pgbench tests.

Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it
is not to test pgbench itself. Pgbench allows to run some programmable
clients in parallel very easily, which cannot be done simply otherwise. I
think that having it there would encourage such uses.

Since none of us is Tom Lane, I think we have no moral right to encourage
somebody doing somebody in postgres. People do what they think better to do.
If somebody need this functions for his tests, he/she can move them to public
space. If somebody sure he would use them soon, and tell about it in this
list, it would be good reason to make them public too. Until then I would offer
to consider these function private business of pgbench testing and keep them
somewhere inside src/bin/pgbench directory

Another point is that the client needs informations held as attributes in
the node in order to connect to the server. Having it outside would mean
picking the attributes inside, which is pretty unclean as it breaks the
abstraction. For me, all pg standard executables could have their methods
in PostgresNode.pm.

you are speaking about
local $ENV{PGPORT} = $self->port;
?
Why do you need it here after all? Lots of TAP tests for bin utilities runs
them using command_like function from TestLib.pm and need no setting
$ENV{PGPORT}. Is pgbench so special? If it is so, may be it is good reason to
fix utilities from TestLib.pm, so they can take port from PostgresNode.

Finally, it does not cost anything to have it there.

The price is maintainability. If in these tests we can use command_like
instead of pgbench_likes it would increase maintainability of the code.

May be you should move some code into some kind of helpers and keep them
in
PostgresNode.pm.

Hmmm. I can put a "run any client" function with the same effect and pass
an additional argument to tell that pgbench should be run, but this looks
quite contrived for no special reason.

I read the existing code more carefully now, and as far as I can see most
console utilities uses command_like for testing. I think the best way would be
to use it for testing. Or write a warping around it, if we need additional
specific behavior for pgbench.

If we need more close integration with PostgresNode then command_like
provides, then may be we should improve command_like or add another function
that provides things that are needed.

Or write universal functions that can be used for testing any postgres
console tool. Then they can be placed in PostgresNode.pm

There are already "psql"-specific functions... Why not pgbench specific
ones?

psql -- is a core utility for postgres node. pgbench is optional.

(3) add a lot of new very small tests so that coverage jumps from very
low
to over 90% for source files. [...]

What tool did you use to calculate the coverage?

I followed the documentated recipee:

https://www.postgresql.org/docs/devel/static/regress-coverage.html

Thanks...

Lots of small test looks good at first glance, except the point that
adding
lots of small files with pgbench scripts is not great idea. This makes
code
tree too overloaded with no practical reason. I am speaking of
src/bin/pgbench/t/scripts/001_0* files.

I think all the data from this file should be somehow imported into
001_pgbench.pl and these files should be created only when tests is
running, and then removed when testing is done.

Hmmm. I thought of that but was very unconvinced by the approach: pgbench
basically always requires a file, so what the stuff was doing was writting
the script into a file, checking for possible errors when doing so, then
unlinking the file and checking for errors again after the run.

I think I was wrong about deleting file after tests are finished. Better keep
them.

Then you
have to do some escaping the the pgbench script themselves, and the perl
script becomes pretty horrible and unreadable with plenty of perl, SQL,
backslash commands in strings...

Perl provides a lot of tools for escaping. If once chooses the right one,
there would be no need in additional backslashes.

Finally, if the script is inside the perl
script it makes it hard to run the test outside of it when a problem is
found, so it is a pain.

I've took back my words about deleting. After a first run one will have these
files "in flesh" so they would be available for further experiments.

I would speak again about maintainability. Having 36 files, most of them <100b
size -- is a very bad idea for maintainability. If each commit of new
functionality would add 36 small files, we will drown in these files quite soon.
These files should be generated on fly. I am 100% sure of it.

The way they are generated may vary... I would prefer to have the script
source code written close to the test that uses it, where it is possible, but
this is just my wishes.

PS. I've read the perl code through much more carefully. All other things are
looking quite good to me.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Nikolay,

Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it
is not to test pgbench itself. Pgbench allows to run some programmable
clients in parallel very easily, which cannot be done simply otherwise. I
think that having it there would encourage such uses.

Since none of us is Tom Lane, I think we have no moral right to encourage
somebody doing somebody in postgres.

I do not get it.

People do what they think better to do.

Probably.

[...] abstraction. For me, all pg standard executables could have their
methods in PostgresNode.pm.

you are speaking about
local $ENV{PGPORT} = $self->port;
?

Yes. My point is that this is the internal stuff of PostgresNode and that
it should stay inside that class. The point of the class is to organize
postgres instances for testing, including client-server connections. From
an object oriented point of view, the method to identify the server could
vary, thus the testing part should not need to know, unless what is tested
is this connection variations, hence it make sense to have it there.

Why do you need it here after all? Lots of TAP tests for bin utilities runs
them using command_like function from TestLib.pm and need no setting
$ENV{PGPORT}.

The test I've seen that do not need it do not connect to the server.
In order to connect to the server they get through variants from
PostgresNode which set the variables before calling the TestLib function.

Is pgbench so special? If it is so, may be it is good reason to
fix utilities from TestLib.pm, so they can take port from PostgresNode.

Nope. TestLib does not know about PostgresNode, the idea is rather that
PostgresNode knows and wraps around TestLib when needed.

If in these tests we can use command_like instead of pgbench_likes it
would increase maintainability of the code.

"command_like" variants do not look precisely at all results (status,
stdout, stderr) and are limited to what they check (eg one regex at a
time). Another thing I do not like is that they expect commands as a list
of pieces, which is not very readable.

Now I can add a command_likes which allows to manage status, stdout and
stderr and add that in TestLib & PostgresNode.

If we need more close integration with PostgresNode then command_like
provides, then may be we should improve command_like or add another function
that provides things that are needed.

Yep, this is possible.

psql -- is a core utility for postgres node. pgbench is optional.

AFAICS pgbench is a core utility as well. I do not know how not to compile
pg without pgbench. It was optional when in contrib, it is not anymore.

I think all the data from this file should be somehow imported into
001_pgbench.pl and these files should be created only when tests is
running, and then removed when testing is done.

Hmmm. I thought of that but was very unconvinced by the approach: pgbench
basically always requires a file, so what the stuff was doing was writting
the script into a file, checking for possible errors when doing so, then
unlinking the file and checking for errors again after the run.

I think I was wrong about deleting file after tests are finished. Better keep
them.

Hmmm... Then what is the point not to have them as files to begin with?

Then you have to do some escaping the the pgbench script themselves,
and the perl script becomes pretty horrible and unreadable with plenty
of perl, SQL, backslash commands in strings...

Perl provides a lot of tools for escaping.

Sure. It does not mean that Perl is the best place to store dozens of
pgbench scripts.

If once chooses the right one, there would be no need in additional
backslashes.

This does not fix the issue with having a mixture of files and languages
in the test script, which I think is basically a bad idea for readability.

Finally, if the script is inside the perl script it makes it hard to
run the test outside of it when a problem is found, so it is a pain.

I've took back my words about deleting. After a first run one will have these
files "in flesh" so they would be available for further experiments.

I'm lost. So where is the problem with having them as file in the first
place, if you want to keep them anyway?

I would speak again about maintainability. Having 36 files, most of them <100b
size -- is a very bad idea for maintainability.

I do not understand why. Running from files is a pgbench requirement. Each
test file is quite well defined, could be commented more about what it is
testing, and it is easy to see which pgbench run uses it.

If each commit of new functionality would add 36 small files, we will
drown in these files quite soon. These files should be generated on fly.
I am 100% sure of it.

Good for you:-)

I think that it would lead to a horrible test script. I can write horrible
test scripts, but I wish to avoid it.

PS. I've read the perl code through much more carefully. All other things are
looking quite good to me.

Ok, something is positive:-)

To sum up:

- I agree to add a generic command TestLib & a wrapper in PostgresNode,
instead of having pgbench specific things in the later, then call
them from pgbench test script.

- I still think that moving the pgbench scripts inside the test script
is a bad idea (tm).

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

To sum up:

- I agree to add a generic command TestLib & a wrapper in PostgresNode,
instead of having pgbench specific things in the later, then call
them from pgbench test script.

- I still think that moving the pgbench scripts inside the test script
is a bad idea (tm).

Here is a v2 along those lines.

I have also separated some basic test which do not need a server running,
as done in other tap tests.

--
Fabien.

В письме от 23 апреля 2017 22:02:25 пользователь Fabien COELHO написал:

Hello Nikolay,

Hmmm. The pre-existing TAP test in pgbench is about concurrent commits,
it
is not to test pgbench itself. Pgbench allows to run some programmable
clients in parallel very easily, which cannot be done simply otherwise. I
think that having it there would encourage such uses.

Since none of us is Tom Lane, I think we have no moral right to encourage
somebody doing somebody in postgres.

I do not get it.

People do what they think better to do.

Probably.

[...] abstraction. For me, all pg standard executables could have their
methods in PostgresNode.pm.

you are speaking about
local $ENV{PGPORT} = $self->port;
?

Yes. My point is that this is the internal stuff of PostgresNode and that
it should stay inside that class. The point of the class is to organize
postgres instances for testing, including client-server connections. From
an object oriented point of view, the method to identify the server could
vary, thus the testing part should not need to know, unless what is tested
is this connection variations, hence it make sense to have it there.

Why do you need it here after all? Lots of TAP tests for bin utilities
runs
them using command_like function from TestLib.pm and need no setting
$ENV{PGPORT}.

The test I've seen that do not need it do not connect to the server.
In order to connect to the server they get through variants from
PostgresNode which set the variables before calling the TestLib function.

Is pgbench so special? If it is so, may be it is good reason to
fix utilities from TestLib.pm, so they can take port from PostgresNode.

Nope. TestLib does not know about PostgresNode, the idea is rather that
PostgresNode knows and wraps around TestLib when needed.

Actually as I look at this part, I feeling an urge to rewrite this code, and
change it so, that all command_like calls were called in a context of certain
node, but it is subject for another patch. In this patch it would be good to
use TestLib functions as they are now, or with minimum modifications.

If in these tests we can use command_like instead of pgbench_likes it
would increase maintainability of the code.

"command_like" variants do not look precisely at all results (status,
stdout, stderr) and are limited to what they check (eg one regex at a
time). Another thing I do not like is that they expect commands as a list
of pieces, which is not very readable.

checking all this things sounds as a good idea.

Now I can add a command_likes which allows to manage status, stdout and
stderr and add that in TestLib & PostgresNode .

This is good idea too, I still do not understand why do you want to add it to
PostgresNode.

And name command_likes -- a very bad solution, as it can easily be confused
with command_like. If it is possible to do it backward compatible, I would try
to improve command_like, so it can check all the results. If it is not, I
would give this function a name that brings no confusion.

I think all the data from this file should be somehow imported into
001_pgbench.pl and these files should be created only when tests is
running, and then removed when testing is done.

Hmmm. I thought of that but was very unconvinced by the approach: pgbench
basically always requires a file, so what the stuff was doing was
writting
the script into a file, checking for possible errors when doing so, then
unlinking the file and checking for errors again after the run.

I think I was wrong about deleting file after tests are finished. Better
keep them.

Hmmm... Then what is the point not to have them as files to begin with?

Not to have them in source code tree in git.

The rest would be in the answer to another sum up letter.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 24 апреля 2017 09:01:18 пользователь Fabien COELHO написал:

To sum up:

- I agree to add a generic command TestLib & a wrapper in PostgresNode,

instead of having pgbench specific things in the later, then call
them from pgbench test script.

- I still think that moving the pgbench scripts inside the test script

is a bad idea (tm).

My sum up is the following:

I see my job as a reviewer is to tell "I've read the code, and I am sure it is
good".

I can tell this about this code, if:

- There is no pgbench specific staff in src/test/perl. Or there should be
_really_big_ reason for it.

- All the testing is done via calls of TestLib.pm functions. May be these
functions should be improved somehow. May be there should be some warper
around them. But not direct IPC::Run::run call.

- All the pgbench scripts are stored in one file. 36 files are not acceptable.
I would include them in the test script itself. May be it can be done in other
ways. But not 36 less then 100 byte files in source code tree...

May be I am wrong. I am not a guru. But then somebody else should say "I've
read the code, and I am sure it is good" instead of me. And it would be his
responsibility then. But if you ask me, issues listed above are very
important, and until they are solved I can not tell "the code is good", and I
see no way to persuade me. May be just ask somebody else...

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Apr 24, 2017 at 4:45 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote:

I can tell this about this code, if:

- There is no pgbench specific staff in src/test/perl. Or there should be
_really_big_ reason for it.

- All the testing is done via calls of TestLib.pm functions. May be these
functions should be improved somehow. May be there should be some warper
around them. But not direct IPC::Run::run call.

- All the pgbench scripts are stored in one file. 36 files are not acceptable.
I would include them in the test script itself. May be it can be done in other
ways. But not 36 less then 100 byte files in source code tree...

May be I am wrong. I am not a guru. But then somebody else should say "I've
read the code, and I am sure it is good" instead of me. And it would be his
responsibility then. But if you ask me, issues listed above are very
important, and until they are solved I can not tell "the code is good", and I
see no way to persuade me. May be just ask somebody else...

A few things that I notice:

1. This patch makes assorted cosmetic and non-cosmetic changes to
pgbench.c. That is not expected for a testing patch. If those
changes need to be made because they are bug fixes or whatever, then
they should be committed separately. A bunch of them look cosmetic
and could be left out or all committed together, according to the
committer's discretion, but the functional ones shouldn't just be
randomly included into a commit that says "add TAP tests for pgbench".

2. I agree that the way the expression evaluation tests are
structured, with lots of small files, is not great. The problem with
it in my view is not so much that it creates a lot of small files, but
that you end up with half of the test definition in 001_pgbench.pl and
the other half spread across all of those small files. It'd be easy
to end up with those things getting out of sync (small files that
aren't in @errors, for example) and isn't real evident on a quick
glance how those files actually get used. I think it would be better
to move the expected output into @errors instead of having a separate
file for it in each case.

3. The comment for check_pgbench_logs() is just "... with logs",
which, at least to me, is not at all clear.

4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl.
Now, those file names seem completely uninformative to me. I assume
that if the pgbench directory has tests in a file called "pgbench",
that's either because there is only one file of tests, or because it
contains the most basic tests. But then we have another file called
"basic". So basically these could be called "foo" and "bar" and it
would provide about the same amount of information; I think we can do
better. Maybe call it 002_without_server or something; that actually
explains the distinction.

5. I don't think adding something like command_likes() is a problem
particularly; it's similar to command_like() which we have already
got, and seems like a reasonable extension of the same idea. But I
don't like the fact that the code looks quite different, and it seems
like it might be better to just extend command_like() and
command_fails_like to each allow $expected_stdout to optionally be an
array of patterns that are tested in turn rather than just a single
pattern. That seems better than adding another very similar function.

I generally support this effort to improve test coverage of pgbench.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

Nope. TestLib does not know about PostgresNode, the idea is rather that
PostgresNode knows and wraps around TestLib when needed.

Actually as I look at this part, I feeling an urge to rewrite this code, and
change it so, that all command_like calls were called in a context of certain
node, but it is subject for another patch. In this patch it would be good to
use TestLib functions as they are now, or with minimum modifications.

I do not understand. In the v2 I sent ISTM that I did exactly as it is
done for other test functions:

- the test function itself is in TestLib

- PostgresNode wraps around it to provide the necessary connection
information which it is holding.

Maybe a better pattern could be thought of, but at least now my addition
is consistent with pre-existing code.

Now I can add a command_likes which allows to manage status, stdout and
stderr and add that in TestLib & PostgresNode.

This is good idea too, I still do not understand why do you want to add it to
PostgresNode.

There is a command_like function in TestLib and a method of the same name
in PostgresNode to provide the function with connections informations.

And name command_likes -- a very bad solution, as it can easily be confused
with command_like.

That is a good point. What other name do you suggest?

If it is possible to do it backward compatible, I would try
to improve command_like, so it can check all the results. If it is not, I
would give this function a name that brings no confusion.

The problem I have with the pre-existing functions is that they do a
partial job: whether status is ok nor not, whether stdout contains
something XOR whether stderr contains something. I want to do all that
repeatedly, hence the enhanced version which checks all three with list of
patterns.

Now maybe I can extend the existing "command_like" to check for extra
arguments, to get the expected status and manage list of patterns for both
stdout & stderr instead of a single regex, but for me this is somehow a
distinct function.

Hmmm. I thought of that but was very unconvinced by the approach:
pgbench basically always requires a file, so what the stuff was doing
was writting the script into a file, checking for possible errors
when doing so, then unlinking the file and checking for errors again
after the run.

I think I was wrong about deleting file after tests are finished. Better
keep them.

Hmmm... Then what is the point not to have them as files to begin with?

Not to have them in source code tree in git.

I do not see that as a problem, the point of git is to manage file
contents anyway.

Now, as I said, I will write unreadable code if required, I will only be
sad about it.

The rest would be in the answer to another sum up letter.

Ok.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Nikolay,

- I agree to add a generic command TestLib & a wrapper in PostgresNode,

instead of having pgbench specific things in the later, then call
them from pgbench test script.

- I still think that moving the pgbench scripts inside the test script
is a bad idea (tm).

My sum up is the following:

I see my job as a reviewer is to tell "I've read the code, and I am sure
it is good".

I'm ok with that. Does not mean I cannot argue if I happen to disagree on
a point.

I can tell this about this code, if:

- There is no pgbench specific staff in src/test/perl. Or there should be
_really_big_ reason for it.

ISTM that v2 I sent does not contain any pgbench specific code. However It
contains new generic code to check for status and list of re on stdout &
stderr.

- All the testing is done via calls of TestLib.pm functions. May be these
functions should be improved somehow. May be there should be some warper
around them. But not direct IPC::Run::run call.

There is no call to IPC out of TestLib.pm in v2 I sent.

- All the pgbench scripts are stored in one file. 36 files are not acceptable.
I would include them in the test script itself. May be it can be done in other
ways. But not 36 less then 100 byte files in source code tree...

I will write an ugly stuff if it is require to pass this patch, but I'm
unconvinced that this is a good idea.

What it is issue with having 36 small files in a directory?

Pg source tree currently contains about 79 under 100 bytes files related
to the insufficient test it is running, so this did not seem to be an
issue in the past.

May be I am wrong. I am not a guru. But then somebody else should say "I've
read the code, and I am sure it is good" instead of me. And it would be his
responsibility then. But if you ask me, issues listed above are very
important, and until they are solved I can not tell "the code is good", and I
see no way to persuade me. May be just ask somebody else...

Of all the issues you list, 2/3 are already fixed in the v2 I sent
attached to the mail you are responding to, and I actually think that the
last point is a bad idea, which I can implement and be sad about.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Robert,

1. This patch makes assorted cosmetic and non-cosmetic changes to
pgbench.c. That is not expected for a testing patch.

Indeed, cosmetic changes should be avoided.

If those changes need to be made because they are bug fixes or whatever,

Yep, this is the case, minor bugs, plus comment I added to make clear
things I had to guess when doing the debug. There are thread & clients
stats, and they were not well managed.

then they should be committed separately.

Fine with me. Note that the added tests check that the bugs/issues are
fixed. That would suggest (1) apply the pretty small bug fixes to pgbench
and (2) add the test, in order to avoid adding non working tests, or
having to change test afterwards.

A bunch of them look cosmetic and could be left out or all committed
together, according to the committer's discretion, but the functional
ones shouldn't just be randomly included into a commit that says "add
TAP tests for pgbench".

The only real cosmectic one is the "." added to the comment. Others are
useful comments which helped debug.

2. I agree that the way the expression evaluation tests are structured,
with lots of small files, is not great. The problem with it in my view
is not so much that it creates a lot of small files, but that you end up
with half of the test definition in 001_pgbench.pl and the other half
spread across all of those small files.

Yep.

It'd be easy to end up with those things getting out of sync (small
files that aren't in @errors, for example)

Hmmm. They are not expected to change, mostly new tests and files should
be added when new features are added.

and isn't real evident on a quick glance how those files actually get
used.

Sure. This is also more or less true of existing tests and has not been a
problem before.

I think it would be better to move the expected output into @errors
instead of having a separate file for it in each case.

The files do not contain the "expected output", they are the pgbench
scripts to run through pgbench with the -f option.

The problem I see with the alternative is to have the contents of plenty
pgbench scripts (sql comments + sql commands + backslash commands) stored
in the middle of a perl script making it pretty unreadable, having to
write and remove files on each test, having problems with running a given
test again if it fails because the needed files are not there and have to
be thought for in the middle of the perl script or not have been cleaned
up by the test script which is not very clean...

So between "not great" and "quite bad", I chose "not great". I can do
"quite bad", but I would prefer to avoid it:-)

3. The comment for check_pgbench_logs() is just "... with logs",
which, at least to me, is not at all clear.

Indeed.

4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl. Now,
those file names seem completely uninformative to me.

I can rename them. "001_pgbench.pl" is just the pre-existing one that I
just edited. The "basic" is a new one with basic option error tests,
replicating what is done in other TAP tests.

would provide about the same amount of information; I think we can do
better. Maybe call it 002_without_server or something; that actually
explains the distinction.

Ok. Why not.

5. I don't think adding something like command_likes() is a problem
particularly; it's similar to command_like() which we have already
got, and seems like a reasonable extension of the same idea. But I
don't like the fact that the code looks quite different,

Indeed, I put comments:-) Otherwise it does not do the same thing.

and it seems like it might be better to just extend command_like() and
command_fails_like to each allow $expected_stdout to optionally be an
array of patterns that are tested in turn rather than just a single
pattern. That seems better than adding another very similar function.

It is not so similar: I want to test (1) precise exit status, (2) one or
range of re on stdout, and (3) one or range of re on stderr.

The available commands just allow to check stdout once if status is ok, OR
stderr if status is not ok. No functions check for the precise status, no
functions check for a range of re, no functions checks for both stdout &
stderr, or only in special cases (help, version, invalid option...).

Basically what I do does not fit any function prototype cleanly, so adding
a new functions looked like the right approach. Probably a better name
could be thought of.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Fabien.

В письме от 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO wrote:

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

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

Do parts of this need to be backpatched? I notice that you're patching
pgbench.c, probably to fix some bug(s); is the idea that we would
backpatch all the new tests on whatever old branches need the bugfixes
too? If so, how far back do the fixes need to go?

ISTM TestLib::command_checks() needs a comment explaining what it does.
Its API seems pretty opaque.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Alvaro,

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

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

Do parts of this need to be backpatched?

I would not bother too much about backpatching.

I notice that you're patching pgbench.c, probably to fix some bug(s);

The bug fix part is about small issues that I noticed while writing
extensive tests. Probably nobody would have noticed otherwise for some
time.

is the idea that we would backpatch all the new tests on whatever old
branches need the bugfixes too? If so, how far back do the fixes need to
go?

I'd say 9.6. There has been quite some changes and significant
restructuring on pgbench wrt to prior versions.

ISTM TestLib::command_checks() needs a comment explaining what it does.
Its API seems pretty opaque.

Ok. That I can do. I'm wondering about Windows portability that I cannot
check.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Nikolay,

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 ;-) [...]

Well, if someone is happy about the code, that's good:-)

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

This is one thing have my doubts about. [...]
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".

Hmmm. I've done such things in the past, but it felt more like a lack of
imagination than good naming practices:-)

What about "command_checks_all"?

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.

Yep. I'm fine with the patch being committed in two parts, one for the C
file and the perl test separately. However the perl test would not pass
without the C changes, so it would be commit C changes and then the tests.

...

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

...

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.

When under throttling (-R) with latency limit (-L), if the stochastic
process schedules transactions which are already too late, they are
skipped by this while loop. However, if a number of transaction is
specified (-t), this resulted in more transaction being considered in the
report than the number that was asked for, and the displayed statistics
were wrong. The added condition allows to not overshoot the target number
of transactions in that case.

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.

Yep.

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

The same incrementation is done in accumStats function, among other
things, in the other branch.

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.

Yep. This is the number of transactions "considered" by the client, both
those executed and those skipped.

Are you sure it is what you want to do?

Yes.

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.

There are two counts: one in the threads, and one in the clients. They
were not cleanly accounted for, so I've updated the code so that what is
counted is clearer, and so that thread & client accounting is done in the
same place/functions, and that formula which use these counts are
consistent.

The stats are updated through the processXactsStats/accumStats which
collect many statistics about execution time and so, but this precise
collection is skipped when the stats are not needed in order to avoid some
cycles, and just simple counting is done.

Why do you check nxacts > 0? It _must_ be grater then 0.

Only under -t (number of transaction). It is 0 under -T (time to run),
which is the usual approach to run tests, but for creating small
deterministic tests I used -t a lot, hence I noticed that some things were
not right.

I could do "nacts == 0" instead of "nacts <= 0", because probably "nacts

= 0", but it seemed safer with <= which complements > in the while

condition.

I think it one want to be sure that some value is valid, he uses Assert.

Nope, the test is really needed.

More notes about code styling: [...]

I've tried to improve comments placement.

The second style issue: [...]

I like the comma operator from time to time, but I can use {} as well.

Please find attached a v4. To sum it up:

About pgbench.c changes:
- make -t work properly with -R and -L and display sensible stats
- check that --progress-timestamp => --progress

About TAP test changes:
- add an infrastructure to run a command and check for its
exit status, stdout and stderr.
- add many tests to pgbench, achieving nearly full coverage.
360 tests running time under 7 seconds on my laptop
(versus 3 tests which ran in about 4 seconds previously).

One open question is whether some test may fail on Windows, if someone
would test that it would be great!

--
Fabien.

В письме от 8 мая 2017 22:08:51 пользователь Fabien COELHO написал:

while (thread->throttle_trigger < now_us -
latency_limit &&

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

...

if (nxacts > 0 && st->cnt >= nxacts)
{

st->state = CSTATE_FINISHED;
break;

}

st->cnt -- number of transactions finished successed or failed, right?

one iteration of for(;;) is for one transaction or really less. Right? We
can't process two tansactions in one iteration of this loop. So we can't
increase st->cnt more then once during one iteration?

So let's look at the while loop:

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;
}

Let's imagine that thread->throttle_trigger is now_us - 10 000,
latency_limit is 5 000 and throttle_delay is 100

How many times you would call processXactStats in this while loop?
And each time it would do st->cnt++

And this while loop is inside for(;;) in which as I said above we can do st-

cnt++ not more than once. I see no logic here.

PS This is a fast reply. May be it will make things clear fast wither for me
or for you. I will carefully answer your full letter tomorrow (I hope nothing
will prevent me from doing it)

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

st->cnt -- number of transactions finished successed or failed, right?

Or *skipped*. That is why I changed the declaration comment.

one iteration of for(;;) is for one transaction or really less. Right?

No, under -R -L late schedules are simply skipped.

We can't process two tansactions in one iteration of this loop. So we
can't increase st->cnt more then once during one iteration?

Yes we can, if they are skipped because the scheduling was too late to
execute them on time.

processXactStats(thread, st, &now, true, agg);

Let's imagine that thread->throttle_trigger is now_us - 10 000,
latency_limit is 5 000 and throttle_delay is 100

How many times you would call processXactStats in this while loop?

Around 100 times to catch up.

And each time it would do st->cnt++

Yep. The "true" argument tells the stats that the transaction was skipped,
though. It just counting late transaction that could not be processed.

And this while loop is inside for(;;) in which as I said above we can do
st->cnt++ not more than once. I see no logic here.

The logic is that at most one *real* transaction is actually performed in
the for, but there may be any number of "skipped" (unexecuted) ones, which
is fine.

PS This is a fast reply. May be it will make things clear fast wither for me
or for you. I will carefully answer your full letter tomorrow (I hope nothing
will prevent me from doing it)

Today was a holiday in France. Tomorrow is not.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

В письме от 8 мая 2017 23:17:49 пользователь Fabien COELHO написал:

st->cnt -- number of transactions finished successed or failed, right?

Or *skipped*. That is why I changed the declaration comment.

Oh! I now I get the idea... The code became clear to me.
Thanks for the patience...

Meanwhile I have another question. I'd like to understand why it have been
done this way.

Why do you place st->cnt++; inside processXactStats function?

It is called two times, once there is alternative st->cnt++ in the else

if (progress || throttle_delay || latency_limit ||
per_script_stats || use_log)
processXactStats(thread, st, &now, false, agg);
else
{
/* detailed stats are not needed, just count */
thread->stats.cnt++;
st->cnt++;
}

This code would mislead me to conclusion that st->cnt is incremented only on
"else" branch. But actually it is incremented anyway, but you should carefully
read processXactStats function to understand it.

The second time processXactStats is called inside the while loop we've
discussed in previous letters. There is no alternative st->cnt++ here, if we
move st->cnt++ out of processXactStats to the body of the loop, we will be
able to write detailed comment, explaining why exactly we increment it, so I
would be the last person who was confused by this part of the code.

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?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers