Thread: [HACKERS] pgbench tap tests & minor fixes
Hello,
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.
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.
(2) reuse this infrastructure for the prior existing test about concurrent
inserts.
(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.
(4) fixes a two minor issues. These fixes may be considered for
backpatching to 10, although I doubt anyone will complain, so I would not
bother. Namely:
- the -t/-R/-L combination was not working properly, fix that
by moving client statistics in processXactStats, adjust some
formula, and add a few comments for details I had to discover.
- add a check that --progress-timestamp => --progress
I'm unsure of the portability of some of the tests (\shell and \setshell),
especially on Windows. If there is an issue, these test will have to be
skipped on this platform.
Some of the tests may fail with a very low probability (eg 1/2**84?).
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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.
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.
В письме от 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.
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
thelater, 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.
> 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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.
В письме от 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.
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
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.
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.
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.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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
notovershoot */ (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.
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
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.
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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.
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.
В письме от 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 { /*
detailedstats 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.
Hello Nikolay, > 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? The idea is that it must be counted once. There is a "detailed" statistic collection fonction which is called when needed (that is the if with all the different options which imply better statistics), else we just do the counting part and nothing else. > [...] 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. Yep. The increments are just a poor's man (few cycle) stat collection, when no detailed stats are needed. > 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. Hmmm. > 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. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 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.
Hello Nikolay,
> Year, this is much more clear for me. Now I understand this statistics code.
Great.
> 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 fix a truncation which appears in an error message later on.
> I did not find any mention of it in the patch introduction letter.
Indeed. Just a minor bug fix to avoid an error message to be truncated. If
you remove it, then you can get:
missing argument in command "sleep"
\slee
Note the missing "p"...
> 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.
I fixed the one which was generating truncated messages. I did not notice
other truncated messages while testing, so I assume that other calls are
correct, but I did not investigate further, so I may be wrong. Maybe in
other instances the truncation removes a "\n" which is intended?
> Old one:
>
> (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */
> 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.
- nxacts is a counter, it could wrap around at least theoretically.
- progress is checked for >=0, so ==0 is fine.
Note that the same trick is used in numerous places in pgbench code, and I
did not wrote most of them:
st->nvariables <= 0
duration <= 0
total->cnt <= 0
(nxacts <= 0 && duration <= 0)
> If you are sure it is good to do '<= 0', let's allow commiter to do final
> decision.
I'm sure that it is a minor thing, and that the trick is already used in
the code. I changed progress because there is a clearly checked, but I
kept nxacts because of the theoritical wrap around.
> 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...
I've propose the name because it checks for everything (multiple stdout,
multiple stderr, status), hence "all". The "like" just refers to stdout
regex, so is quite limited, and "checks all" seems to be a good summary of
what is done, while "like more" is pretty unclear to me, because it is
relative to "like", so I have to check what "like" does and then assume
that it does more...
> /* 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.
Yep. "checks_all" is clearer to me that "like_more" which is relative to
another function.
> 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 :-)
Here is a v6.
- it uses "progress == 0"
- it does not change "nxacts <= 0" because of possible wrapping
- it fixes two typos in a perl comment about the checks_all function
- I kept "checks all" because this talks more to me than "like more"
if a native English speaker or someone else has an opinion, it is
welcome.
Also, if someone could run a test on windows, it would be great.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 30 мая 2017 17:24:26 Вы написали: > > 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 fix a truncation which appears in an error message later on. > > > I did not find any mention of it in the patch introduction letter. > > Indeed. Just a minor bug fix to avoid an error message to be truncated. If > you remove it, then you can get: > > missing argument in command "sleep" > \slee > > Note the missing "p"... > > > 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. > > I fixed the one which was generating truncated messages. I did not notice > other truncated messages while testing, so I assume that other calls are > correct, but I did not investigate further, so I may be wrong. Maybe in > other instances the truncation removes a "\n" which is intended? I did some investigation: The code there really suppose that there is always \n at the end of the line, and truncates the line. It is done in /* Get location of the ending newline */ end_offset = expr_scanner_offset(sstate) - 1; just two lines above the code we are discussing. When you have one line code /sleep 2ms with no "end of line" symbol at the end, it will cut off "s" instead of "\n" You've fix it, but now it will leave \n, in all sleeps in multiline scripts. So this should be fixed in both expr_scanner_get_substring cases, and keep last symbol only if it is not "\n". > Here is a v6. > > - it uses "progress == 0" > > - it does not change "nxacts <= 0" because of possible wrapping > > - it fixes two typos in a perl comment about the checks_all function > > - I kept "checks all" because this talks more to me than "like more" > if a native English speaker or someone else has an opinion, it is > welcome. Ok, let's leave this for commiter to make final decision. > Also, if someone could run a test on windows, it would be great. I'll try to ask a friend of mine to run this on windows... -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.
Hello Nikolay, > I did some investigation: The code there really suppose that there is always > \n at the end of the line, and truncates the line. It is done in > > /* Get location of the ending newline */ > end_offset = expr_scanner_offset(sstate) - 1; > > just two lines above the code we are discussing. > > When you have one line code /sleep 2ms with no "end of line" symbol at the > end, it will cut off "s" instead of "\n" > > You've fix it, but now it will leave \n, in all sleeps in multiline scripts. > > So this should be fixed in both expr_scanner_get_substring cases, and keep last > symbol only if it is not "\n". Indeed, this is a mess. The code assumes all stuff is a line ending with '\n', but this is not always the case. >> Also, if someone could run a test on windows, it would be great. > I'll try to ask a friend of mine to run this on windows... That would be great! Here is a v7 which chomps the final newline only if there is one. Thanks again, -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 8 июня 2017 19:56:02 пользователь Fabien COELHO написал: > > So this should be fixed in both expr_scanner_get_substring cases, and keep > > last symbol only if it is not "\n". > > Indeed, this is a mess. The code assumes all stuff is a line ending with > '\n', but this is not always the case. > > Here is a v7 which chomps the final newline only if there is one. Sorry, but I still have problems with new solution... First is function name. "expr_scanner_get_line" does not deal with any line at all... it gets substring and "chomps" it. As for me, I would do expr_scanner_chomp_substring(PsqlScanState, int, int&); that changes end_offset as desired... And use it instead of end_offset = expr_scanner_offset(sstate) - 1; The second issue: you are removing all trailing \n and \r. I think you should remove only one \n at the end of the string, and one \r before \n if there was one. I do not think there will be any practical difference between my and yours solutions here, but mine is more neat, I think... I do not have enough imagination to think about if "\n\r\r\r\r\r\n" case can happen, and what will happen of we remove them all... So I suggest to remove only the last one. -- Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
> As for me, I would do expr_scanner_chomp_substring(PsqlScanState, int, int&); > that changes end_offset as desired... Why not. > And use it instead of end_offset = expr_scanner_offset(sstate) - 1; I removed these? > The second issue: you are removing all trailing \n and \r. I think you should > remove only one \n at the end of the string, and one \r before \n if there was > one. So chomp one eol. Is the attached version better to your test? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 15 июня 2017 21:10:12 Вы написали: > > As for me, I would do expr_scanner_chomp_substring(PsqlScanState, int, > > int&); that changes end_offset as desired... > > Why not. > > > And use it instead of end_offset = expr_scanner_offset(sstate) - 1; > > I removed these? > > > The second issue: you are removing all trailing \n and \r. I think you > > should remove only one \n at the end of the string, and one \r before \n > > if there was one. > > So chomp one eol. > > Is the attached version better to your test? I've expected from expr_scanner_chomp_substring to decrement end_offset, so it would work more like perl chomp function, but the way you've done is also good. The sad think is that in v7 and v8 TAP tests fails. (in v6 it still works, I have local branches for all your patches versions). I did not check it bdefore in v7, just read the code. It was my mistake -- Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
В письме от 25 июня 2017 19:03:54 пользователь Fabien COELHO написал: > Hello Nikolay, > > >> Is the attached version better to your test? > > > > I've expected from expr_scanner_chomp_substring to decrement end_offset, > > so it would work more like perl chomp function, but the way you've done > > is also good. > > Ok. > > > The sad think is that in v7 and v8 TAP tests fails. (in v6 it still works, > > I have local branches for all your patches versions). I did not check it > > bdefore in v7, just read the code. It was my mistake > > Could you be more precise please? Which TAP tests are failing? Could you > give the log showing the issues encountered? I am building dev postgres with --enable-cassert and get a lot of 'pgbench: exprscan.l:354: expr_scanner_get_substring: Assertion `end_offset <= strlen(state->scanbuf)' failed. may be this it because of "end_offset + 1" in expr_scanner_chomp_substring ? Why there is + 1 there? > > I did "make check" and "make check-world", both PASS. > > ISTM that manually in pgbench right know with patch v8 I have: > > sh> make check > rm -rf '/home/fabien/DEV/GIT/postgresql'/tmp_install > /bin/mkdir -p '/home/fabien/DEV/GIT/postgresql'/tmp_install/log > make -C '../../..' DESTDIR='/home/fabien/DEV/GIT/postgresql'/tmp_install > install >'/home/fabien/DEV/GIT/postgresql'/tmp_install/log/install.log > 2>&1 rm -rf /home/fabien/DEV/GIT/postgresql/src/bin/pgbench/tmp_check/log > cd . && TESTDIR='/home/fabien/DEV/GIT/postgresql/src/bin/pgbench' > PATH="/home/fabien/DEV/GIT/postgresql/tmp_install/usr/local/pgsql/bin:$PATH > " > LD_LIBRARY_PATH="/home/fabien/DEV/GIT/postgresql/tmp_install/usr/local/pgsq > l/lib" PGPORT='65432' > PG_REGRESS='/home/fabien/DEV/GIT/postgresql/src/bin/pgbench/../../../src/te > st/regress/pg_regress' prove -I ../../../src/test/perl/ -I . t/*.pl > t/001_pgbench_with_server.pl .. ok > t/002_pgbench_no_server.pl .... ok > All tests successful. > Files=2, Tests=360, 6 wallclock secs ( 0.04 usr 0.02 sys + 4.53 cusr > 0.22 csys = 4.81 CPU) Result: PASS > > Which looks ok. -- Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
> may be this it because of "end_offset + 1" in expr_scanner_chomp_substring ? > Why there is + 1 there? Thanks for the debug! Here is a v9 which does a rebase after the reindentation and fixes the bad offset. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
В письме от 25 июня 2017 20:42:58 пользователь Fabien COELHO написал: > > may be this it because of "end_offset + 1" in expr_scanner_chomp_substring > > ? Why there is + 1 there? > > Thanks for the debug! Here is a v9 which does a rebase after the > reindentation and fixes the bad offset. Sorry I am back here after a big pause (I hope) I've applied the patch to the current master, and it seems that one test now fails: regress_log_002_pgbench_no_server: not ok 70 - pgbench option error: prepare after script err /(?^:query mode .* before any)/ # Failed test 'pgbench option error: prepare after script err /(?^:query mode .* before any)/' # at /home/nataraj/dev/review- pgbench/ccc/postgresql/src/bin/pgbench/../../../src/test/perl/TestLib.pm line 369. # 'connection to database "" failed: # could not connect to server: No such file or directory # Is the server running locally and accepting # connections on Unix domain socket "/tmp/.s.PGSQL.5432"? # ' # doesn't match '(?^:query mode .* before any)' opts=-i -S, stat=1, out=(?^:^$), err=(?^:cannot be used in initialization), name=pgbench option error: init vs run# Running: pgbench -i -S -- Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
Hello Nikolay, > I've applied the patch to the current master, and it seems that one test now > fails: Indeed, Tom removed the -M option order constraint, so the test which check for that does not apply anymore. Here is a v10 without this test. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
While reviewing another patch, I figured out at least one potential issue on windows when handling execution status. The attached v11 small updates is more likely to pass on windows. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi All! I am about to set "Ready for commit" status to this patch. So there is my summary for commiter, so one does not need to carefully read all the thread. This patch is consists of three parts. May be they should be commited separately, then Fabien will split them, I think. 1. The tests. Tests are quite good. May be I myself would write them in another style, but "there is more than one way to do it" in perl, you know. These test covers more than 90% of C code of pgbench, which is good. (I did not check this myself, but see no reason not to trust Fabien) The most doubtful part of the patch is the following: the patch introduce command_checks_all function in TestLib.pm that works like command_like function but also allows to check STDERR output and exit status. First: I have some problem with the name, I would call it command_like_more or something similar, but I decided to leave it for commiter to make final choice. Second: I think that command_checks and command_like do very similar things. I think that later all lests should be rewritten to use command_checks, and get rid of command_like, And I do not know how to put this better in the developing workflow. May be it should be another patch after this one is commited. 2. Patch for -t/-R/-L case. Current pgbench does not process -t/-R/-L case correctly. This was also fixed in this patch. Now if you set number of transactions using -t/--transactions, combining with -R/--rate or -L/--latency-limit, you can be sure there would be not more than were specified in -t and they are properly counted. This part is quite clear 3. \n process in process_backslash_command error output process_backslash_command raises an error if there are some problems with backslash commands, and prints the command that has error. But it considers that there is always \n symbol at the end of the command and just chop it out. But when the backslash command is at the end of the file, there is no \n at the end of line. So this patch introduces expr_scanner_chomp_substring function that works just like expr_scanner_get_substring but it skips \n or \r\n symbols at the end of line. I still have some problems with function name here, but have no idea how to do it better. So that's it. I hope my involvement in the review process were useful. Will be happy to see this patch commited into master :-) -- Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
Hello Nikolay, Thanks for the review! As for function names, committers can have their say. I'm somehow not dissatisfied with the current version, but I also agree with you that they are imperfect. As for included bug fixes, I can do separate patches, but I think that it is enough to first commit the pgbench files and then the tap-test files, in that order. I'll see what committers say. When/if this patch is committed, it should enable to add more tests quite easily to the numerous pgbench patches already in the pipe for quite some time... In particular, adding the new functions and operators to pgbench expressions patch is waiting for this to go to ready to committers. A further caveat to committers: I'm unsure about what happens on Windows. I've done my best so that it should work. -- Fabien.
On Thu, Aug 31, 2017 at 9:29 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote: > I am about to set "Ready for commit" status to this patch. So there is my > summary for commiter, so one does not need to carefully read all the thread. > > This patch is consists of three parts. May be they should be commited > separately, then Fabien will split them, I think. I am not sure why, but this email has broken the whole thread on my gmail client... Did you change the subject name indirectly? The thread is shaped correctly in postgresql.org though. Thanks! -- Michael
Michael Paquier wrote: > On Thu, Aug 31, 2017 at 9:29 PM, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > I am about to set "Ready for commit" status to this patch. So there is my > > summary for commiter, so one does not need to carefully read all the thread. > > > > This patch is consists of three parts. May be they should be commited > > separately, then Fabien will split them, I think. > > I am not sure why, but this email has broken the whole thread on my > gmail client... Did you change the subject name indirectly? The thread > is shaped correctly in postgresql.org though. Threading in postgresql.org uses the References and In-Reply-To headers in order to build the threads, and ignores Subject, whereas Gmail breaks threads as soon as the subject is changed. The message by Nikolay has a period at end of subject. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> As for included bug fixes, I can do separate patches, but I think that it
> is enough to first commit the pgbench files and then the tap-test files,
> in that order. I'll see what committers say.
Starting to look at this. I concur that the bug fixes ought to be
committed separately, but since they're in separate files it's not hard
to disassemble the patch.
A couple of thoughts --
* I do not think we need expr_scanner_chomp_substring. Of the three
existing callers of expr_scanner_get_substring, one is doing a manual
chomp afterwards, and the other two need chomps per your patch.
Seems to me we should just include the chomp logic in
expr_scanner_get_substring. Maybe it'd be worth adding a "bool chomp"
argument to it, but I think that would be more for clarity than
usefulness.
* I do not like the API complexity of command_checks_all, specifically
not the business of taking either a scalar or an array for the cmd,
out, and err arguments. I think it'd be clearer and less bug-prone
to just take arrays, full stop. Maybe it's just that I'm a crummy Perl
programmer, but I do not see uses of this "ref ... eq 'ARRAY'" business
elsewhere in our test scaffolding, and this doesn't seem like a great
place to introduce it. At worst you'd need to add brackets around the
arguments in a few callers.
* In the same vein, I don't know what this does:
sub pgbench($$$$$)
and I see no existing instances of it elsewhere in our tree. I think it'd
be better not to require advanced degrees in Perl programming in order to
read the test cases.
* Another way in which command_checks_all introduces API complexity is
that it accounts for a variable number of tests depending on how many
patterns are provided. This seems like a mess. I see that you have
in some places (not very consistently) annotated call sites as to how
many tests they account for, but who's going to do the math to make
sure everything adds up? Maybe it'd be better to not use like(), or
do something else so that each command_checks_all call counts as one
Test::More test rather than N. Or just forget prespecifying a test
count and use done_testing instead.
regards, tom lane
Nikolay Shaplov <dhyan@nataraj.su> writes:
> 2. Patch for -t/-R/-L case.
> Current pgbench does not process -t/-R/-L case correctly. This was also fixed
> in this patch.
This portion of the patch seems uncontroversial, so I've pushed it, with
some minor cosmetic adjustments (mostly comments).
regards, tom lane
Hello Tom, >> As for included bug fixes, I can do separate patches, but I think that it >> is enough to first commit the pgbench files and then the tap-test files, >> in that order. I'll see what committers say. > > Starting to look at this. I concur that the bug fixes ought to be > committed separately, but since they're in separate files it's not hard > to disassemble the patch. Ok. > A couple of thoughts -- > > * I do not think we need expr_scanner_chomp_substring. Of the three > existing callers of expr_scanner_get_substring, one is doing a manual > chomp afterwards, and the other two need chomps per your patch. > Seems to me we should just include the chomp logic in > expr_scanner_get_substring. Maybe it'd be worth adding a "bool chomp" > argument to it, but I think that would be more for clarity than > usefulness. Ok. I thought that I would get a slap on the hand if I changed the initial function, but I get one not for changing it:-) > * I do not like the API complexity of command_checks_all, specifically > not the business of taking either a scalar or an array for the cmd, > out, and err arguments. I think it'd be clearer and less bug-prone > to just take arrays, full stop. Maybe it's just that I'm a crummy Perl > programmer, but I do not see uses of this "ref ... eq 'ARRAY'" business > elsewhere in our test scaffolding, and this doesn't seem like a great > place to introduce it. At worst you'd need to add brackets around the > arguments in a few callers. Hmmm. I find it quite elegant and harmless, but it can be removed. > * In the same vein, I don't know what this does: > > sub pgbench($$$$$) > > and I see no existing instances of it elsewhere in our tree. I think it'd > be better not to require advanced degrees in Perl programming in order to > read the test cases. It just says that 5 scalars are expected, so it would complain if "type" or number do not match. Now, why give type hints if they are not mandatory, as devs can always detect their errors by extensive testing instead:-) But I agree that it is not a usual Perl stance and it can be removed. > * Another way in which command_checks_all introduces API complexity is > that it accounts for a variable number of tests depending on how many > patterns are provided. This seems like a mess. I see that you have > in some places (not very consistently) annotated call sites as to how > many tests they account for, but who's going to do the math to make > sure everything adds up? Perl:-) I run the test, figure out the number it found in the resulting error message, and update the number in the source. Not too hard:-) > Maybe it'd be better to not use like(), or do something else so that > each command_checks_all call counts as one Test::More test rather than > N. > Or just forget prespecifying a test count and use done_testing > instead. Yep, "done_testing" looks fine, I'll investigate that, but other test seemed to insist on declaring the expected number. Now "done_testing" may be a nicer option if some tests need to be skipped on some platform. -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> * I do not think we need expr_scanner_chomp_substring. Of the three
>> existing callers of expr_scanner_get_substring, one is doing a manual
>> chomp afterwards, and the other two need chomps per your patch.
> Ok. I thought that I would get a slap on the hand if I changed the initial
> function, but I get one not for changing it:-)
Well, more for not looking at the other caller and noting it needed
this too. Anyway, done with the addition of a "chomp" parameter,
leaving only the TAP test changes to consider.
I'll set the CF entry back to "waiting on author" pending your
revisions of those.
regards, tom lane
> Anyway, done with the addition of a "chomp" parameter, leaving only the > TAP test changes to consider. Ok, thanks. > I'll set the CF entry back to "waiting on author" pending your > revisions of those. See attached. Removed scalar/array ref hack, plenty [] added everywhere to match. Removed perl function prototypes. Does not try to announce the number of tests, just tell when done. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> * In the same vein, I don't know what this does:
>> sub pgbench($$$$$)
>> and I see no existing instances of it elsewhere in our tree. I think it'd
>> be better not to require advanced degrees in Perl programming in order to
>> read the test cases.
> It just says that 5 scalars are expected, so it would complain if "type"
> or number do not match. Now, why give type hints if they are not
> mandatory, as devs can always detect their errors by extensive testing
> instead:-)
My concern is basically about maintaining coding style consistency.
I don't want readers to come across something like this and have to
wonder "why is this function like this, when hundreds of apparently
similar functions elsewhere in the tree aren't? Is there some reason
why it needs to use this feature when the rest don't?". If the answer
to that is "yes" then there should be a comment explaining why. If
the answer is "no", well, it shouldn't be randomly different from the
rest of our code.
Now, having gone and looked up what that construct does, I wouldn't
necessarily be against a patch that introduced use of these poor man's
prototypes throughout our Perl code. It's the inconsistency that I'm
down on. But others with more Perl experience than I might have good
reasons why it's not like that already. I do have to say that many of
the examples I've seen look more like line noise than readable code.
>> * Another way in which command_checks_all introduces API complexity is
>> that it accounts for a variable number of tests depending on how many
>> patterns are provided. This seems like a mess. I see that you have
>> in some places (not very consistently) annotated call sites as to how
>> many tests they account for, but who's going to do the math to make
>> sure everything adds up?
> Perl:-) I run the test, figure out the number it found in the resulting
> error message, and update the number in the source. Not too hard:-)
Yeah, but then what error is all that work protecting you from?
regards, tom lane
Hello Tom, >>> sub pgbench($$$$$) > > My concern is basically about maintaining coding style consistency. Yes, I'm fine with that. I have removed it in the v12 patch. > reasons why it's not like that already. I do have to say that many of > the examples I've seen look more like line noise than readable code. Sure. I agree that the readability is debatable. The usefulness is only that an error is raised at "compile" time instead of having a strange behavior at runtime. >> I run the test, figure out the number it found in the resulting >> error message, and update the number in the source. > > Yeah, but then what error is all that work protecting you from? I'm not sure I understand your point. I agree that Perl doing the counting may hide issues. Now it is more of an incremental thing, if a test is added the counter is upgraded accordingly, and the local consistency can be checked. Anyway, as some tests may have to be skipped on some platforms, it seems that the done_testing approach is sounder. The v12 patch uses that. Thanks for your comments. -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>>> I run the test, figure out the number it found in the resulting
>>> error message, and update the number in the source.
>> Yeah, but then what error is all that work protecting you from?
> I'm not sure I understand your point. I agree that Perl doing the counting
> may hide issues. Now it is more of an incremental thing, if a test is
> added the counter is upgraded accordingly, and the local consistency can
> be checked.
Well, IIUC the point of "tests => nnn" is to protect against the
possibility that some coding mistake caused some of the tests to not
get run. If you change the test script and then take Perl's word for
what nnn should be, what protection have you got that you didn't introduce
such a mistake along with whatever your intended change was? It seems
pointless to maintain the tests count that way.
> Anyway, as some tests may have to be skipped on some platforms, it seems
> that the done_testing approach is sounder. The v12 patch uses that.
Sounds good.
regards, tom lane
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> [ pgbench-tap-12.patch ]
Pushed, with some minor fooling with comments and after running it
through perltidy. (I have no opinions about Perl code formatting,
but perltidy does ...)
The only substantive change I made was to drop the test that attempted
to connect to no-such-host.postgresql.org. That's (a) unnecessary,
as this is a test of pgbench not libpq; (b) likely to provoke a wide
range of platform-specific error messages, which we'd have to account
for given that the test is looking for specific output; and (c) likely
to draw complaints from buildfarm owners and packagers who do not like
test scripts that try to make random external network connections.
Like you, I'm a bit worried about the code for extracting an exit
status from IPC::Run::run. We'll have to keep an eye on the buildfarm
for a bit. If there's any trouble, I'd be inclined to drop it down
to just success/fail rather than checking the exact exit code.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello Tom,
> Pushed, with some minor fooling with comments and after running it
> through perltidy. (I have no opinions about Perl code formatting,
> but perltidy does ...)
Why not. I do not like the result much, but it homogeneity is not a bad
thing.
> The only substantive change I made was to drop the test that attempted
> to connect to no-such-host.postgresql.org. That's (a) unnecessary,
> as this is a test of pgbench not libpq; (b) likely to provoke a wide
> range of platform-specific error messages, which we'd have to account
> for given that the test is looking for specific output; and (c) likely
> to draw complaints from buildfarm owners and packagers who do not like
> test scripts that try to make random external network connections.
Ok. Note that it was only an unsuccessful DNS request, but I understand
the concern. The libpq tap test mostly focus on url parsing but seem to
include host names ("example.com"/host) and various IPs.
> Like you, I'm a bit worried about the code for extracting an exit
> status from IPC::Run::run. We'll have to keep an eye on the buildfarm
> for a bit. If there's any trouble, I'd be inclined to drop it down
> to just success/fail rather than checking the exact exit code.
Ok. If this occurs, there is a $windows_os variable that can be tested to
soften the test only if necessary, and keep the exact check for systems
that can.
Thanks for the review, the improvements and the push. Now the various
patches about pgbench in the queue should include tap-tests.
Hopefully one line will be greener on https://coverage.postgresql.org/.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/08/2017 09:40 AM, Tom Lane wrote: > Fabien COELHO <coelho@cri.ensmp.fr> writes: >> [ pgbench-tap-12.patch ] > Pushed, with some minor fooling with comments and after running it > through perltidy. (I have no opinions about Perl code formatting, > but perltidy does ...) > > The only substantive change I made was to drop the test that attempted > to connect to no-such-host.postgresql.org. That's (a) unnecessary, > as this is a test of pgbench not libpq; (b) likely to provoke a wide > range of platform-specific error messages, which we'd have to account > for given that the test is looking for specific output; and (c) likely > to draw complaints from buildfarm owners and packagers who do not like > test scripts that try to make random external network connections. > > Like you, I'm a bit worried about the code for extracting an exit > status from IPC::Run::run. We'll have to keep an eye on the buildfarm > for a bit. If there's any trouble, I'd be inclined to drop it down > to just success/fail rather than checking the exact exit code. > > bowerbird seems to have been made unhappy. cheers andrew -- Andrew Dunstan 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
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 09/08/2017 09:40 AM, Tom Lane wrote:
>> Like you, I'm a bit worried about the code for extracting an exit
>> status from IPC::Run::run. We'll have to keep an eye on the buildfarm
>> for a bit. If there's any trouble, I'd be inclined to drop it down
>> to just success/fail rather than checking the exact exit code.
> bowerbird seems to have been made unhappy.
I saw that failure, but it appears to be a server-side crash:
2017-09-10 19:39:03.395 EDT [1100] LOG: server process (PID 11464) was terminated by exception 0xC0000005
2017-09-10 19:39:03.395 EDT [1100] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value.
Given the lack of any log outputs from process 11464, it's hard to tell
what it was doing, but it seems not to be any of the backends running
pgbench queries. So maybe an autovac worker? I dunno. Anyway, it's
difficult to credit that this commit caused the failure, even if it did
happen during the new test case. I'm inclined to write it off as another
one of the random crashes that bowerbird seems prone to.
If the failure proves repeatable, then of course we'll need to look
more closely.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/11/2017 01:58 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 09/08/2017 09:40 AM, Tom Lane wrote: >>> Like you, I'm a bit worried about the code for extracting an exit >>> status from IPC::Run::run. We'll have to keep an eye on the buildfarm >>> for a bit. If there's any trouble, I'd be inclined to drop it down >>> to just success/fail rather than checking the exact exit code. >> bowerbird seems to have been made unhappy. > I saw that failure, but it appears to be a server-side crash: > > 2017-09-10 19:39:03.395 EDT [1100] LOG: server process (PID 11464) was terminated by exception 0xC0000005 > 2017-09-10 19:39:03.395 EDT [1100] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. > > Given the lack of any log outputs from process 11464, it's hard to tell > what it was doing, but it seems not to be any of the backends running > pgbench queries. So maybe an autovac worker? I dunno. Anyway, it's > difficult to credit that this commit caused the failure, even if it did > happen during the new test case. I'm inclined to write it off as another > one of the random crashes that bowerbird seems prone to. > > If the failure proves repeatable, then of course we'll need to look > more closely. > > Hmm, it had several failures and now a success. Will keep an eye on it. cheers andrew -- Andrew Dunstan 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
On 2017-09-11 15:02:21 -0400, Andrew Dunstan wrote: > > > On 09/11/2017 01:58 PM, Tom Lane wrote: > > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > >> On 09/08/2017 09:40 AM, Tom Lane wrote: > >>> Like you, I'm a bit worried about the code for extracting an exit > >>> status from IPC::Run::run. We'll have to keep an eye on the buildfarm > >>> for a bit. If there's any trouble, I'd be inclined to drop it down > >>> to just success/fail rather than checking the exact exit code. > >> bowerbird seems to have been made unhappy. > > I saw that failure, but it appears to be a server-side crash: > > > > 2017-09-10 19:39:03.395 EDT [1100] LOG: server process (PID 11464) was terminated by exception 0xC0000005 > > 2017-09-10 19:39:03.395 EDT [1100] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. > > > > Given the lack of any log outputs from process 11464, it's hard to tell > > what it was doing, but it seems not to be any of the backends running > > pgbench queries. So maybe an autovac worker? I dunno. Anyway, it's > > difficult to credit that this commit caused the failure, even if it did > > happen during the new test case. I'm inclined to write it off as another > > one of the random crashes that bowerbird seems prone to. > > > > If the failure proves repeatable, then of course we'll need to look > > more closely. > > > > > > > Hmm, it had several failures and now a success. Will keep an eye on it. There just was another failure like that https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&dt=2017-09-19%2001%3A42%3A20 I first thought it might be the new recovery tests, or the changes leading to its addition, but it's a different test and in the middle of the run. Even so, I'd have looked towards my commit, except that there's a number of previous reports that look similar. Any chance you could get backtraces on these? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers