Re: [HACKERS] pgbench tap tests & minor fixes. - Mailing list pgsql-hackers

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)