Re: [HACKERS] pgbench tap tests & minor fixes.
On 2017-09-11 15:02:21 -0400, Andrew Dunstan wrote: > > > On 09/11/2017 01:58 PM, Tom Lane wrote: > > Andrew Dunstan 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 0xC005 > > 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
Re: [HACKERS] pgbench tap tests & minor fixes.
On 09/11/2017 01:58 PM, Tom Lane wrote: > Andrew Dunstan 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 0xC005 > 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 Dunstanhttps://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
Re: [HACKERS] pgbench tap tests & minor fixes.
Andrew Dunstan 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 0xC005 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
Re: [HACKERS] pgbench tap tests & minor fixes.
On 09/08/2017 09:40 AM, Tom Lane wrote: > Fabien COELHO 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 Dunstanhttps://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
Re: [HACKERS] pgbench tap tests & minor fixes.
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
Re: [HACKERS] pgbench tap tests & minor fixes.
Fabien COELHO 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
Re: [HACKERS] pgbench tap tests & minor fixes.
Fabien COELHO 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Michael Paquier wrote: > On Thu, Aug 31, 2017 at 9:29 PM, Nikolay Shaplov 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 Herrerahttps://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
Re: [HACKERS] pgbench tap tests & minor fixes.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Fabien COELHO 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
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.diff --git a/src/bin/pgbench/t/001_pgbench.pl b/src/bin/pgbench/t/001_pgbench.pl deleted file mode 100644 index 34d686e..000 --- a/src/bin/pgbench/t/001_pgbench.pl +++ /dev/null @@ -1,25 +0,0 @@ -use strict; -use warnings; - -use PostgresNode; -use TestLib; -use Test::More tests => 3; - -# Test concurrent insertion into table with UNIQUE oid column. DDL expects -# GetNewOidWithIndex() to successfully avoid violating uniqueness for indexes -# like pg_class_oid_index and pg_proc_oid_index. This indirectly exercises -# LWLock and spinlock concurrency. This test makes a 5-MiB table. -my $node = get_new_node('main'); -$node->init; -$node->start; -$node->safe_psql('postgres', - 'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; ' - . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);'); -my $script = $node->basedir . '/pgbench_script'; -append_to_file($script, - 'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);'); -$node->command_like( - [ qw(pgbench --no-vacuum --client=5 --protocol=prepared - --transactions=25 --file), $script ], - qr{processed: 125/125}, - 'concurrent OID generation'); diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl new file mode 100644 index 000..5db2fd0 --- /dev/null +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -0,0 +1,441 @@ +use strict; +use warnings; + +use PostgresNode; +use TestLib; +use Test::More; + +# start a pgbench specific server +my $node = get_new_node('main'); +$node->init; +$node->start; + +# invoke pgbench +sub pgbench +{ + my ($opts, $stat, $out, $err, $name, $files) = @_; + my @cmd = ('pgbench', split /\s+/, $opts); + my @filenames = (); + if (defined $files) + { + # note: files are ordered for determinism + for my $fn (sort keys %$files) + { + my $filename = $node->basedir . '/' . $fn; + push @cmd, '-f', $filename; + # cleanup file weight + $filename =~ s/\@\d+$//; + #push @filenames, $filename; + append_to_file($filename, $$files{$fn}); + } + } + $node->command_checks_all(\@cmd, $stat, $out, $err, $name); + # cleanup? + #unlink @filenames or die "cannot unlink files (@filenames): $!"; +} + +# Test concurrent insertion into table with UNIQUE oid column. DDL expects +# GetNewOidWithIndex() to successfully avoid violating uniqueness for indexes +# like pg_class_oid_index and pg_proc_oid_index. This indirectly exercises +# LWLock and spinlock concurrency. This test makes a 5-MiB table. + +$node->safe_psql('postgres', + 'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; ' + . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);'); + +pgbench('--no-vacuum --client=5 --protocol=prepared --transactions=25', + 0, [ qr{processed: 125/125} ], [ qr{^$} ], 'concurrency OID generation', + { '001_pgbench_concurrent_oid_generation' => +'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);' }); + +# cleanup +$node->safe_psql('postgres', 'DROP TABLE oid_tbl;'); + +# Trigger various connection errors +pgbench('no-such-database', + 1, [ qr{^$} ], + [ qr{connection to database "no-such-database" failed}, +qr{FATAL: database "no-such-database" does not exist} ], + 'no such database'); + +pgbench('-U no-such-user template0', + 1, [ qr{^$} ], + [ qr{connection to database "template0" failed}, +qr{FATAL: role "no-such-user" does not exist} ], + 'no such user'); + +pgbench('-h no-such-host.postgresql.org', + 1, [ qr{^$} ], + [ qr{connection to database "postgres" failed}, +# actual message may vary: +# - Name or service not known +# - Temporary failure in name resolution +qr{could not translate host name "no-such-host.postgresql.org" to address: } ], + 'no such host'); + +pgbench('-S -t 1', + 1, [ qr{^$} ], [ qr{Perhaps you need to do initialization} ], + 'run without init'); + +# Initialize pgbench tables scale 1 +pgbench('-i', + 0, [ qr{^$} ], + [ qr{creating tables}, qr{vacuum}, qr{set primary keys}, qr{done\.} ], + 'pgbench scale 1 initialization', +); + +# Again, with all possible options +pgbench( + # unlogged => faster test + '--initialize --scale=1 --unlogged --fillfactor=98 --foreign-keys --quiet' . + ' --tablespace=pg_default --index-tablespace=pg_default', + 0, [ qr{^$}i ], + [ qr{creating tables}, qr{vacuum}, qr{set primary keys}, +qr{set foreign keys}, qr{done\.} ], + 'pgbench scale 1 initialization'); + +# Run all builtins for a few transactions: 20 checks +pgbench( + '--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t' . + ' --connect -n -v -n', + 0, + [ qr{builtin: TPC-B}, qr{clients: 2\b}, qr{processed: 10/10}, +qr{mode
Re: [HACKERS] pgbench tap tests & minor fixes.
Fabien COELHO 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Nikolay Shaplov 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Fabien COELHO 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
On Thu, Aug 31, 2017 at 9:29 PM, Nikolay Shaplov 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..8255eff 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -360,6 +360,23 @@ expr_scanner_get_substring(PsqlScanState state, } /* + * get current expression line without one ending newline + */ +char * +expr_scanner_chomp_substring(PsqlScanState state, int start_offset, int end_offset) +{ + const char *p = state->scanbuf; + /* chomp eol */ + if (end_offset > start_offset && p[end_offset] == '\n') + { + end_offset--; + if (end_offset > start_offset && p[end_offset] == '\r') + end_offset--; + } + return expr_scanner_get_substring(state, start_offset, end_offset); +} + +/* * Get the line number associated with the given string offset * (which must not be past the end of where we've lexed to). */ diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae78c7b..b72fe04 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2364,15 +2371,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) */ case CSTATE_END_TX: -/* - * transaction finished: calculate latency and log the - * transaction - */ -if (progress || throttle_delay || latency_limit || - per_script_stats || use_log) - processXactStats(thread, st, &now, false, agg); -else - thread->stats.cnt++; +/* transaction finished: calculate latency and do log */ +processXactStats(thread, st, &now, false, agg); if (is_connect) { @@ -2381,7 +2381,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2519,17 +2518,20 @@ processXactStats(TState *thread, CState *st, instr_time *now, { double latency = 0.0, lag = 0.0; + bool detailed = progress || throttle_delay || latency_limit || + per_script_stats || use_log; - if ((!skipped) && INSTR_TIME_IS_ZERO(*now)) - INSTR_TIME_SET_CURRENT(*now); - - if (!skipped) + if (detailed && !skipped) { + if (INSTR_TIME_IS_ZERO(*now)) + INSTR_TIME_SET_CURRENT(*now); + /* compute latency & lag */ latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled; lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* detailed thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,7 +2541,13 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + { + /* no detailed stats, just count */ thread->stats.cnt++; + } + + /* client stat is just counting */ + st->cnt ++; if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3026,8 +3034,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) PQExpBufferData word_buf; int word_offset; int offsets[MAX_ARGS]; /* offsets of argument words */ - int start_offset, -end_offset; + int start_offset; int lineno; int j; @@ -3081,13 +3088,10 @@ process_backslash_command(PsqlScanState sstate, const char *source) my_command->expr = expr_parse_result; - /* Get location of the ending newline */ - end_offset = expr_scanner_offset(sstate)
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..8255eff 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -360,6 +360,23 @@ expr_scanner_get_substring(PsqlScanState state, } /* + * get current expression line without one ending newline + */ +char * +expr_scanner_chomp_substring(PsqlScanState state, int start_offset, int end_offset) +{ + const char *p = state->scanbuf; + /* chomp eol */ + if (end_offset > start_offset && p[end_offset] == '\n') + { + end_offset--; + if (end_offset > start_offset && p[end_offset] == '\r') + end_offset--; + } + return expr_scanner_get_substring(state, start_offset, end_offset); +} + +/* * Get the line number associated with the given string offset * (which must not be past the end of where we've lexed to). */ diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae78c7b..b72fe04 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2364,15 +2371,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) */ case CSTATE_END_TX: -/* - * transaction finished: calculate latency and log the - * transaction - */ -if (progress || throttle_delay || latency_limit || - per_script_stats || use_log) - processXactStats(thread, st, &now, false, agg); -else - thread->stats.cnt++; +/* transaction finished: calculate latency and do log */ +processXactStats(thread, st, &now, false, agg); if (is_connect) { @@ -2381,7 +2381,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2519,17 +2518,20 @@ processXactStats(TState *thread, CState *st, instr_time *now, { double latency = 0.0, lag = 0.0; + bool detailed = progress || throttle_delay || latency_limit || + per_script_stats || use_log; - if ((!skipped) && INSTR_TIME_IS_ZERO(*now)) - INSTR_TIME_SET_CURRENT(*now); - - if (!skipped) + if (detailed && !skipped) { + if (INSTR_TIME_IS_ZERO(*now)) + INSTR_TIME_SET_CURRENT(*now); + /* compute latency & lag */ latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled; lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* detailed thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,7 +2541,13 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + { + /* no detailed stats, just count */ thread->stats.cnt++; + } + + /* client stat is just counting */ + st->cnt ++; if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3026,8 +3034,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) PQExpBufferData word_buf; int word_offset; int offsets[MAX_ARGS]; /* offsets of argument words */ - int start_offset, -end_offset; + int start_offset; int lineno; int j; @@ -3081,13 +3088,10 @@ process_backslash_command(PsqlScanState sstate, const char *source) my_command->expr = expr_parse_result; - /* Get location of the
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..8255eff 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -360,6 +360,23 @@ expr_scanner_get_substring(PsqlScanState state, } /* + * get current expression line without one ending newline + */ +char * +expr_scanner_chomp_substring(PsqlScanState state, int start_offset, int end_offset) +{ + const char *p = state->scanbuf; + /* chomp eol */ + if (end_offset > start_offset && p[end_offset] == '\n') + { + end_offset--; + if (end_offset > start_offset && p[end_offset] == '\r') + end_offset--; + } + return expr_scanner_get_substring(state, start_offset, end_offset); +} + +/* * Get the line number associated with the given string offset * (which must not be past the end of where we've lexed to). */ diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 4d364a1..6bc6eb6 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2364,15 +2371,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) */ case CSTATE_END_TX: -/* - * transaction finished: calculate latency and log the - * transaction - */ -if (progress || throttle_delay || latency_limit || - per_script_stats || use_log) - processXactStats(thread, st, &now, false, agg); -else - thread->stats.cnt++; +/* transaction finished: calculate latency and do log */ +processXactStats(thread, st, &now, false, agg); if (is_connect) { @@ -2381,7 +2381,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2519,17 +2518,20 @@ processXactStats(TState *thread, CState *st, instr_time *now, { double latency = 0.0, lag = 0.0; + bool detailed = progress || throttle_delay || latency_limit || + per_script_stats || use_log; - if ((!skipped) && INSTR_TIME_IS_ZERO(*now)) - INSTR_TIME_SET_CURRENT(*now); - - if (!skipped) + if (detailed && !skipped) { + if (INSTR_TIME_IS_ZERO(*now)) + INSTR_TIME_SET_CURRENT(*now); + /* compute latency & lag */ latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled; lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* detailed thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,7 +2541,13 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + { + /* no detailed stats, just count */ thread->stats.cnt++; + } + + /* client stat is just counting */ + st->cnt ++; if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3030,8 +3038,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) PQExpBufferData word_buf; int word_offset; int offsets[MAX_ARGS]; /* offsets of argument words */ - int start_offset, -end_offset; + int start_offset; int lineno; int j; @@ -3085,13 +3092,10 @@ process_backslash_command(PsqlScanState sstate, const char *source) my_command->expr = expr_parse_result; - /* Get location of the ending newline */ - end_offset = ex
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..0c802af 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -360,6 +360,23 @@ expr_scanner_get_substring(PsqlScanState state, } /* + * get current expression line without one ending newline + */ +char * +expr_scanner_chomp_substring(PsqlScanState state, int start_offset, int end_offset) +{ + const char *p = state->scanbuf; + /* chomp eol */ + if (end_offset > start_offset && p[end_offset] == '\n') + { + end_offset--; + if (end_offset > start_offset && p[end_offset] == '\r') + end_offset--; + } + return expr_scanner_get_substring(state, start_offset, end_offset + 1); +} + +/* * Get the line number associated with the given string offset * (which must not be past the end of where we've lexed to). */ diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 14aa587..0f4e125 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2364,15 +2371,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) */ case CSTATE_END_TX: -/* - * transaction finished: calculate latency and log the - * transaction - */ -if (progress || throttle_delay || latency_limit || - per_script_stats || use_log) - processXactStats(thread, st, &now, false, agg); -else - thread->stats.cnt++; +/* transaction finished: calculate latency and do log */ +processXactStats(thread, st, &now, false, agg); if (is_connect) { @@ -2381,7 +2381,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2519,17 +2518,20 @@ processXactStats(TState *thread, CState *st, instr_time *now, { double latency = 0.0, lag = 0.0; + bool detailed = progress || throttle_delay || latency_limit || + per_script_stats || use_log; - if ((!skipped) && INSTR_TIME_IS_ZERO(*now)) - INSTR_TIME_SET_CURRENT(*now); - - if (!skipped) + if (detailed && !skipped) { + if (INSTR_TIME_IS_ZERO(*now)) + INSTR_TIME_SET_CURRENT(*now); + /* compute latency & lag */ latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled; lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* detailed thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,7 +2541,13 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + { + /* no detailed stats, just count */ thread->stats.cnt++; + } + + /* client stat is just counting */ + st->cnt ++; if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3030,8 +3038,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) PQExpBufferData word_buf; int word_offset; int offsets[MAX_ARGS]; /* offsets of argument words */ - int start_offset, -end_offset; + int s
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l index dc1367b..40a2a52 100644 --- a/src/bin/pgbench/exprscan.l +++ b/src/bin/pgbench/exprscan.l @@ -360,6 +360,20 @@ expr_scanner_get_substring(PsqlScanState state, } /* + * get current expression line without ending newline + */ +char * +expr_scanner_get_line(PsqlScanState state, int start_offset, int end_offset) +{ + const char *p = state->scanbuf; + /* chomp eols */ + while (end_offset > start_offset && + (p[end_offset] == '\n' || p[end_offset] == '\r')) + end_offset--; + return expr_scanner_get_substring(state, start_offset, end_offset + 1); +} + +/* * Get the line number associated with the given string offset * (which must not be past the end of where we've lexed to). */ diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..383aa78 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2364,15 +2371,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) */ case CSTATE_END_TX: -/* - * transaction finished: calculate latency and log the - * transaction - */ -if (progress || throttle_delay || latency_limit || - per_script_stats || use_log) - processXactStats(thread, st, &now, false, agg); -else - thread->stats.cnt++; +/* transaction finished: calculate latency and do log */ +processXactStats(thread, st, &now, false, agg); if (is_connect) { @@ -2381,7 +2381,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2519,17 +2518,20 @@ processXactStats(TState *thread, CState *st, instr_time *now, { double latency = 0.0, lag = 0.0; + bool detailed = progress || throttle_delay || latency_limit || + per_script_stats || use_log; - if ((!skipped) && INSTR_TIME_IS_ZERO(*now)) - INSTR_TIME_SET_CURRENT(*now); - - if (!skipped) + if (detailed && !skipped) { + if (INSTR_TIME_IS_ZERO(*now)) + INSTR_TIME_SET_CURRENT(*now); + /* compute latency & lag */ latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled; lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* detailed thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,7 +2541,13 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; }
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..b28558c 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,8 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + while (thread->throttle_trigger < now_us - latency_limit && + (nxacts <= 0 || st->cnt < nxacts)) /* with -t, do not overshoot */ { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ @@ -2053,6 +2054,12 @@ doCustom(TS
Re: [HACKERS] pgbench tap tests & minor fixes
Hello, st->cnt -- number of transactions finished successed or failed, right? Or *skipped*. That is why I changed the declaration comment. one iteration of for(;;) is for one transaction or really less. Right? No, under -R -L late schedules are simply skipped. We can't process two tansactions in one iteration of this loop. So we can't increase st->cnt more then once during one iteration? Yes we can, if they are skipped because the scheduling was too late to execute them on time. processXactStats(thread, st, &now, true, agg); Let's imagine that thread->throttle_trigger is now_us - 10 000, latency_limit is 5 000 and throttle_delay is 100 How many times you would call processXactStats in this while loop? Around 100 times to catch up. And each time it would do st->cnt++ Yep. The "true" argument tells the stats that the transaction was skipped, though. It just counting late transaction that could not be processed. And this while loop is inside for(;;) in which as I said above we can do st->cnt++ not more than once. I see no logic here. The logic is that at most one *real* transaction is actually performed in the for, but there may be any number of "skipped" (unexecuted) ones, which is fine. PS This is a fast reply. May be it will make things clear fast wither for me or for you. I will carefully answer your full letter tomorrow (I hope nothing will prevent me from doing it) Today was a holiday in France. Tomorrow is not. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 8 мая 2017 22:08:51 пользователь Fabien COELHO написал: > >while (thread->throttle_trigger < now_us - > >latency_limit && > > > > /* with -t, do not overshoot */ > > (nxacts <= 0 || st->cnt < nxacts)) > > ... > > >if (nxacts > 0 && st->cnt >= nxacts) > >{ > > > >st->state = CSTATE_FINISHED; > >break; > > > >} > > st->cnt -- number of transactions finished successed or failed, right? one iteration of for(;;) is for one transaction or really less. Right? We can't process two tansactions in one iteration of this loop. So we can't increase st->cnt more then once during one iteration? So let's look at the while loop: while (thread->throttle_trigger < now_us - latency_limit && /* with -t, do not overshoot */ (nxacts <= 0 || st->cnt < nxacts)) { processXactStats(thread, st, &now, true, agg); /* next rendez-vous */ wait = getPoissonRand(thread, throttle_delay); thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } Let's imagine that thread->throttle_trigger is now_us - 10 000, latency_limit is 5 000 and throttle_delay is 100 How many times you would call processXactStats in this while loop? And each time it would do st->cnt++ And this while loop is inside for(;;) in which as I said above we can do st- >cnt++ not more than once. I see no logic here. PS This is a fast reply. May be it will make things clear fast wither for me or for you. I will carefully answer your full letter tomorrow (I hope nothing will prevent me from doing it) -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
Hello Alvaro, Here is a v3, with less files. I cannot say I find it better, but it still works. The "command_likes" function has been renamed "command_checks". Do parts of this need to be backpatched? I would not bother too much about backpatching. I notice that you're patching pgbench.c, probably to fix some bug(s); The bug fix part is about small issues that I noticed while writing extensive tests. Probably nobody would have noticed otherwise for some time. is the idea that we would backpatch all the new tests on whatever old branches need the bugfixes too? If so, how far back do the fixes need to go? I'd say 9.6. There has been quite some changes and significant restructuring on pgbench wrt to prior versions. ISTM TestLib::command_checks() needs a comment explaining what it does. Its API seems pretty opaque. Ok. That I can do. I'm wondering about Windows portability that I cannot check. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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 Herrerahttps://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
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..749b16d 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,9 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + 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 */ @@ -2053,6 +2055,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2372,7 +2380,7 @@ doCustom(TState *thread, CState *st, StatsData *agg) per_script_stats || use_log) processXactStats(thread, st, &now, false, agg); else - thread->stats.cnt++; + thread->stats.cnt++, st->cnt++; if (is_connect) { @@ -2381,7 +2389,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2530,6 +2537,7 @@ processXactStats(TState *thread, CState *st, instr_time *now, lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,8 +2547,12 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + /* just count */ thread->stats.cnt++; + /* client stat is just counting */ + st->cnt ++; + if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3118,7 +3130,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) /* Save line */ my_command->line = expr_scanner_get_substring(sstate, start_offset, - end_offset); + end_offset + 1); if (pg_strcasecmp(my_command->argv[0], "sleep") == 0) { @@ -3509,7 +3521,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time, { printf("number of transactions per client: %d\n", nxacts); printf("number of transactions actually processed: " INT64_FORMAT "/%d\n", - total->cnt, nxacts * nclients); + total->cnt - total->skipped, nxacts * nclients); } else { @@ -3525,12 +3537,12 @@ printResults(TState *threads, StatsData *total, instr_time total_time, if (throttle_delay && latency_limit) printf("number of transactions skipped: " INT64_FORMAT " (%.3f %%)\n", total->skipped, - 100.0 * total->skipped / (total->skipped + total->cnt)); + 100.0 * total->skipped / total->cnt); if (latency_limit) printf("number of transactions above the %.1f ms latency limit: %d (%.3f %%)\n", latency_limit / 1000.0, latency_late, - 100.0 * latency_late / (total->skipped + total->cnt)); + 100.0 * latency_late / total->cnt); if (throttle_delay || progress || latency_limit) printSimpleStats("latency", &total->latency); @@ -3580,7 +3592,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time, printf(" - number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n", sql_script[i].stats.skipped, 100.0 * sql_script[i].stats.skipped / - (sql_script[i].stats.skipped + sql_script[i].stats.cnt)); + sql_script[i].stats.cnt); if (num_scripts > 1) printSimpleStats(" - latency", &sql_script[i].stats.latency); @@ -4106,6 +4118,12 @@ main(int argc, char **argv) exit(1); } + if (progress_timestamp && progress <= 0) + { + fprintf(stderr, "--progress-timestamp is allowed only under --progress\n"); + exit(1); + } + /* * save main process id in the global variable because process id will be
Re: [HACKERS] pgbench tap tests & minor fixes
Hello Robert, 1. This patch makes assorted cosmetic and non-cosmetic changes to pgbench.c. That is not expected for a testing patch. Indeed, cosmetic changes should be avoided. If those changes need to be made because they are bug fixes or whatever, Yep, this is the case, minor bugs, plus comment I added to make clear things I had to guess when doing the debug. There are thread & clients stats, and they were not well managed. then they should be committed separately. Fine with me. Note that the added tests check that the bugs/issues are fixed. That would suggest (1) apply the pretty small bug fixes to pgbench and (2) add the test, in order to avoid adding non working tests, or having to change test afterwards. A bunch of them look cosmetic and could be left out or all committed together, according to the committer's discretion, but the functional ones shouldn't just be randomly included into a commit that says "add TAP tests for pgbench". The only real cosmectic one is the "." added to the comment. Others are useful comments which helped debug. 2. I agree that the way the expression evaluation tests are structured, with lots of small files, is not great. The problem with it in my view is not so much that it creates a lot of small files, but that you end up with half of the test definition in 001_pgbench.pl and the other half spread across all of those small files. Yep. It'd be easy to end up with those things getting out of sync (small files that aren't in @errors, for example) Hmmm. They are not expected to change, mostly new tests and files should be added when new features are added. and isn't real evident on a quick glance how those files actually get used. Sure. This is also more or less true of existing tests and has not been a problem before. I think it would be better to move the expected output into @errors instead of having a separate file for it in each case. The files do not contain the "expected output", they are the pgbench scripts to run through pgbench with the -f option. The problem I see with the alternative is to have the contents of plenty pgbench scripts (sql comments + sql commands + backslash commands) stored in the middle of a perl script making it pretty unreadable, having to write and remove files on each test, having problems with running a given test again if it fails because the needed files are not there and have to be thought for in the middle of the perl script or not have been cleaned up by the test script which is not very clean... So between "not great" and "quite bad", I chose "not great". I can do "quite bad", but I would prefer to avoid it:-) 3. The comment for check_pgbench_logs() is just "... with logs", which, at least to me, is not at all clear. Indeed. 4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl. Now, those file names seem completely uninformative to me. I can rename them. "001_pgbench.pl" is just the pre-existing one that I just edited. The "basic" is a new one with basic option error tests, replicating what is done in other TAP tests. would provide about the same amount of information; I think we can do better. Maybe call it 002_without_server or something; that actually explains the distinction. Ok. Why not. 5. I don't think adding something like command_likes() is a problem particularly; it's similar to command_like() which we have already got, and seems like a reasonable extension of the same idea. But I don't like the fact that the code looks quite different, Indeed, I put comments:-) Otherwise it does not do the same thing. and it seems like it might be better to just extend command_like() and command_fails_like to each allow $expected_stdout to optionally be an array of patterns that are tested in turn rather than just a single pattern. That seems better than adding another very similar function. It is not so similar: I want to test (1) precise exit status, (2) one or range of re on stdout, and (3) one or range of re on stderr. The available commands just allow to check stdout once if status is ok, OR stderr if status is not ok. No functions check for the precise status, no functions check for a range of re, no functions checks for both stdout & stderr, or only in special cases (help, version, invalid option...). Basically what I do does not fit any function prototype cleanly, so adding a new functions looked like the right approach. Probably a better name could be thought of. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
Hello Nikolay, - I agree to add a generic command TestLib & a wrapper in PostgresNode, instead of having pgbench specific things in the later, then call them from pgbench test script. - I still think that moving the pgbench scripts inside the test script is a bad idea (tm). My sum up is the following: I see my job as a reviewer is to tell "I've read the code, and I am sure it is good". I'm ok with that. Does not mean I cannot argue if I happen to disagree on a point. I can tell this about this code, if: - There is no pgbench specific staff in src/test/perl. Or there should be _really_big_ reason for it. ISTM that v2 I sent does not contain any pgbench specific code. However It contains new generic code to check for status and list of re on stdout & stderr. - All the testing is done via calls of TestLib.pm functions. May be these functions should be improved somehow. May be there should be some warper around them. But not direct IPC::Run::run call. There is no call to IPC out of TestLib.pm in v2 I sent. - All the pgbench scripts are stored in one file. 36 files are not acceptable. I would include them in the test script itself. May be it can be done in other ways. But not 36 less then 100 byte files in source code tree... I will write an ugly stuff if it is require to pass this patch, but I'm unconvinced that this is a good idea. What it is issue with having 36 small files in a directory? Pg source tree currently contains about 79 under 100 bytes files related to the insufficient test it is running, so this did not seem to be an issue in the past. May be I am wrong. I am not a guru. But then somebody else should say "I've read the code, and I am sure it is good" instead of me. And it would be his responsibility then. But if you ask me, issues listed above are very important, and until they are solved I can not tell "the code is good", and I see no way to persuade me. May be just ask somebody else... Of all the issues you list, 2/3 are already fixed in the v2 I sent attached to the mail you are responding to, and I actually think that the last point is a bad idea, which I can implement and be sad about. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
Hello, Nope. TestLib does not know about PostgresNode, the idea is rather that PostgresNode knows and wraps around TestLib when needed. Actually as I look at this part, I feeling an urge to rewrite this code, and change it so, that all command_like calls were called in a context of certain node, but it is subject for another patch. In this patch it would be good to use TestLib functions as they are now, or with minimum modifications. I do not understand. In the v2 I sent ISTM that I did exactly as it is done for other test functions: - the test function itself is in TestLib - PostgresNode wraps around it to provide the necessary connection information which it is holding. Maybe a better pattern could be thought of, but at least now my addition is consistent with pre-existing code. Now I can add a command_likes which allows to manage status, stdout and stderr and add that in TestLib & PostgresNode. This is good idea too, I still do not understand why do you want to add it to PostgresNode. There is a command_like function in TestLib and a method of the same name in PostgresNode to provide the function with connections informations. And name command_likes -- a very bad solution, as it can easily be confused with command_like. That is a good point. What other name do you suggest? If it is possible to do it backward compatible, I would try to improve command_like, so it can check all the results. If it is not, I would give this function a name that brings no confusion. The problem I have with the pre-existing functions is that they do a partial job: whether status is ok nor not, whether stdout contains something XOR whether stderr contains something. I want to do all that repeatedly, hence the enhanced version which checks all three with list of patterns. Now maybe I can extend the existing "command_like" to check for extra arguments, to get the expected status and manage list of patterns for both stdout & stderr instead of a single regex, but for me this is somehow a distinct function. Hmmm. I thought of that but was very unconvinced by the approach: pgbench basically always requires a file, so what the stuff was doing was writting the script into a file, checking for possible errors when doing so, then unlinking the file and checking for errors again after the run. I think I was wrong about deleting file after tests are finished. Better keep them. Hmmm... Then what is the point not to have them as files to begin with? Not to have them in source code tree in git. I do not see that as a problem, the point of git is to manage file contents anyway. Now, as I said, I will write unreadable code if required, I will only be sad about it. The rest would be in the answer to another sum up letter. Ok. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
On Mon, Apr 24, 2017 at 4:45 PM, Nikolay Shaplov wrote: > I can tell this about this code, if: > > - There is no pgbench specific staff in src/test/perl. Or there should be > _really_big_ reason for it. > > - All the testing is done via calls of TestLib.pm functions. May be these > functions should be improved somehow. May be there should be some warper > around them. But not direct IPC::Run::run call. > > - All the pgbench scripts are stored in one file. 36 files are not acceptable. > I would include them in the test script itself. May be it can be done in other > ways. But not 36 less then 100 byte files in source code tree... > > > May be I am wrong. I am not a guru. But then somebody else should say "I've > read the code, and I am sure it is good" instead of me. And it would be his > responsibility then. But if you ask me, issues listed above are very > important, and until they are solved I can not tell "the code is good", and I > see no way to persuade me. May be just ask somebody else... A few things that I notice: 1. This patch makes assorted cosmetic and non-cosmetic changes to pgbench.c. That is not expected for a testing patch. If those changes need to be made because they are bug fixes or whatever, then they should be committed separately. A bunch of them look cosmetic and could be left out or all committed together, according to the committer's discretion, but the functional ones shouldn't just be randomly included into a commit that says "add TAP tests for pgbench". 2. I agree that the way the expression evaluation tests are structured, with lots of small files, is not great. The problem with it in my view is not so much that it creates a lot of small files, but that you end up with half of the test definition in 001_pgbench.pl and the other half spread across all of those small files. It'd be easy to end up with those things getting out of sync (small files that aren't in @errors, for example) and isn't real evident on a quick glance how those files actually get used. I think it would be better to move the expected output into @errors instead of having a separate file for it in each case. 3. The comment for check_pgbench_logs() is just "... with logs", which, at least to me, is not at all clear. 4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl. Now, those file names seem completely uninformative to me. I assume that if the pgbench directory has tests in a file called "pgbench", that's either because there is only one file of tests, or because it contains the most basic tests. But then we have another file called "basic". So basically these could be called "foo" and "bar" and it would provide about the same amount of information; I think we can do better. Maybe call it 002_without_server or something; that actually explains the distinction. 5. I don't think adding something like command_likes() is a problem particularly; it's similar to command_like() which we have already got, and seems like a reasonable extension of the same idea. But I don't like the fact that the code looks quite different, and it seems like it might be better to just extend command_like() and command_fails_like to each allow $expected_stdout to optionally be an array of patterns that are tested in turn rather than just a single pattern. That seems better than adding another very similar function. I generally support this effort to improve test coverage of pgbench. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 24 апреля 2017 09:01:18 пользователь Fabien COELHO написал: > > To sum up: > > > > - I agree to add a generic command TestLib & a wrapper in PostgresNode, > > > > instead of having pgbench specific things in the later, then call > > them from pgbench test script. > > > > - I still think that moving the pgbench scripts inside the test script > > > > is a bad idea (tm). My sum up is the following: I see my job as a reviewer is to tell "I've read the code, and I am sure it is good". I can tell this about this code, if: - There is no pgbench specific staff in src/test/perl. Or there should be _really_big_ reason for it. - All the testing is done via calls of TestLib.pm functions. May be these functions should be improved somehow. May be there should be some warper around them. But not direct IPC::Run::run call. - All the pgbench scripts are stored in one file. 36 files are not acceptable. I would include them in the test script itself. May be it can be done in other ways. But not 36 less then 100 byte files in source code tree... May be I am wrong. I am not a guru. But then somebody else should say "I've read the code, and I am sure it is good" instead of me. And it would be his responsibility then. But if you ask me, issues listed above are very important, and until they are solved I can not tell "the code is good", and I see no way to persuade me. May be just ask somebody else... -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 23 апреля 2017 22:02:25 пользователь Fabien COELHO написал: > Hello Nikolay, > > >> Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, > >> it > >> is not to test pgbench itself. Pgbench allows to run some programmable > >> clients in parallel very easily, which cannot be done simply otherwise. I > >> think that having it there would encourage such uses. > > > > Since none of us is Tom Lane, I think we have no moral right to encourage > > somebody doing somebody in postgres. > > I do not get it. > > > People do what they think better to do. > > Probably. > > >> [...] abstraction. For me, all pg standard executables could have their > >> methods in PostgresNode.pm. > > > > you are speaking about > > local $ENV{PGPORT} = $self->port; > > ? > > Yes. My point is that this is the internal stuff of PostgresNode and that > it should stay inside that class. The point of the class is to organize > postgres instances for testing, including client-server connections. From > an object oriented point of view, the method to identify the server could > vary, thus the testing part should not need to know, unless what is tested > is this connection variations, hence it make sense to have it there. > > > Why do you need it here after all? Lots of TAP tests for bin utilities > > runs > > them using command_like function from TestLib.pm and need no setting > > $ENV{PGPORT}. > > The test I've seen that do not need it do not connect to the server. > In order to connect to the server they get through variants from > PostgresNode which set the variables before calling the TestLib function. > > > Is pgbench so special? If it is so, may be it is good reason to > > fix utilities from TestLib.pm, so they can take port from PostgresNode. > > Nope. TestLib does not know about PostgresNode, the idea is rather that > PostgresNode knows and wraps around TestLib when needed. Actually as I look at this part, I feeling an urge to rewrite this code, and change it so, that all command_like calls were called in a context of certain node, but it is subject for another patch. In this patch it would be good to use TestLib functions as they are now, or with minimum modifications. > > If in these tests we can use command_like instead of pgbench_likes it > > would increase maintainability of the code. > > "command_like" variants do not look precisely at all results (status, > stdout, stderr) and are limited to what they check (eg one regex at a > time). Another thing I do not like is that they expect commands as a list > of pieces, which is not very readable. checking all this things sounds as a good idea. > > Now I can add a command_likes which allows to manage status, stdout and > stderr and add that in TestLib & PostgresNode . This is good idea too, I still do not understand why do you want to add it to PostgresNode. And name command_likes -- a very bad solution, as it can easily be confused with command_like. If it is possible to do it backward compatible, I would try to improve command_like, so it can check all the results. If it is not, I would give this function a name that brings no confusion. > >>> I think all the data from this file should be somehow imported into > >>> 001_pgbench.pl and these files should be created only when tests is > >>> running, and then removed when testing is done. > >> > >> Hmmm. I thought of that but was very unconvinced by the approach: pgbench > >> basically always requires a file, so what the stuff was doing was > >> writting > >> the script into a file, checking for possible errors when doing so, then > >> unlinking the file and checking for errors again after the run. > > > > I think I was wrong about deleting file after tests are finished. Better > > keep them. > > Hmmm... Then what is the point not to have them as files to begin with? Not to have them in source code tree in git. The rest would be in the answer to another sum up letter. -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
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.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..2128418 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,9 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + 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 */ @@ -2053,6 +2055,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2366,13 +2374,13 @@ doCustom(TState *thread, CState *st, StatsData *agg) /* * transaction finished: calculate latency and log the - * transaction + * transaction. */ if (progress || throttle_delay || latency_limit || per_script_stats || use_log) processXactStats(thread, st, &now, false, agg); else - thread->stats.cnt++; + thread->stats.cnt++, st->cnt++; if (is_connect) { @@ -2381,7 +2389,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2530,6 +2537,7 @@ processXactStats(TState *thread, CState *st, instr_time *now, lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,8 +2547,12 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + /* just count */ thread->stats.cnt++; + /* client stat is just counting */ + st->cnt ++; + if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3118,7 +3130,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) /* Save line */ my_command->line = expr_scanner_get_substring(sstate, start_offset, - end_offset); + end_offset + 1); if (pg_strcasecmp(my_command->argv[0], "sleep") == 0) { @@ -3509,7 +3521,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time, { printf("number of transactions per client: %d\n", nxacts); printf("number of transactions actually processed: " INT64_FORMAT "/%d\n", - total->cnt, nxacts * nclients); + total->cnt - total->skipped, nxacts * nclients); } else { @@ -3525,12 +3537,12 @@ printResults(TState *threads, StatsData *total, instr_time total_time, if (throttle_delay && latency_limit) printf("number of transactions skipped: " INT64_FORMAT " (%.3f %%)\n", total->skipped, - 100.0 * total->skipped / (total->skipped + total->cnt)); + 100.0 * total->skipped / total->cnt); if (latency_limit) printf("number of transactions above the %.1f ms latency limit: %d (%.3f %%)\n", latency_limit / 1000.0, latency_late, - 100.0 * latency_late / (total->skipped + total->cnt)); + 100.0 * latency_late / total->cnt); if (throttle_delay || progress || latency_limit) printSimpleStats("latency", &total->latency); @@ -3580,7 +3592,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time, printf(" - number of transactions skipped: " INT64_FORMAT " (%.3f%%)\n", sql_script[i].stats.skipped, 100.0 * sql_script[i].stats.skipped / - (sql_script[i].stats.skipped + sql_s
Re: [HACKERS] pgbench tap tests & minor fixes
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 qui
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 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 furt
Re: [HACKERS] pgbench tap tests & minor fixes
Hello Nikolay, Hi! Since I used to be good at perl, I like tests, and I've dealt with postgres TAP tests before, I think I can review you patch. If it is OK for everyone. I think that all good wills are welcome, especially concerning code reviews. For now I've just gave this patch a quick look through... But nevertheless I have something to comment: The attached patch: (1) extends the existing perl tap test infrastructure with methods to test pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes" which allows to check for expectations. I do not think it is good idea adding this functions to the PostgresNode.pm. I thought it was:-) They are pgbench specific. Sure. I do not think anybody will need them outside of pgbench tests. Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it is not to test pgbench itself. Pgbench allows to run some programmable clients in parallel very easily, which cannot be done simply otherwise. I think that having it there would encourage such uses. Another point is that the client needs informations held as attributes in the node in order to connect to the server. Having it outside would mean picking the attributes inside, which is pretty unclean as it breaks the abstraction. For me, all pg standard executables could have their methods in PostgresNode.pm. Finally, it does not cost anything to have it there. Generally, these functions as they are now, should be placed is separate .pm file. like it was done in src/test/ssl/ I put them there because it already manages both terminal client (psql) & server side things (initdb, postgres...). Initially pgbench was a "contrib" but it has been moved as a standard client a few versions ago, so for me it seemed logical to have the support there. May be you should move some code into some kind of helpers and keep them in PostgresNode.pm. Hmmm. I can put a "run any client" function with the same effect and pass an additional argument to tell that pgbench should be run, but this looks quite contrived for no special reason. Or write universal functions that can be used for testing any postgres console tool. Then they can be placed in PostgresNode.pm There are already "psql"-specific functions... Why not pgbench specific ones? (3) add a lot of new very small tests so that coverage jumps from very low to over 90% for source files. [...] What tool did you use to calculate the coverage? I followed the documentated recipee: https://www.postgresql.org/docs/devel/static/regress-coverage.html Lots of small test looks good at first glance, except the point that adding lots of small files with pgbench scripts is not great idea. This makes code tree too overloaded with no practical reason. I am speaking of src/bin/pgbench/t/scripts/001_0* files. I think all the data from this file should be somehow imported into 001_pgbench.pl and these files should be created only when tests is running, and then removed when testing is done. Hmmm. I thought of that but was very unconvinced by the approach: pgbench basically always requires a file, so what the stuff was doing was writting the script into a file, checking for possible errors when doing so, then unlinking the file and checking for errors again after the run. Then you have to do some escaping the the pgbench script themselves, and the perl script becomes pretty horrible and unreadable with plenty of perl, SQL, backslash commands in strings... Finally, if the script is inside the perl script it makes it hard to run the test outside of it when a problem is found, so it is a pain. I think that job for creating and removing these files should be moved to pgbench and pgbench_likes. But there is more then one way to do it. ;-) Hmmm. See above. That's what I've noticed so far... I will give this patch more attentive look soon. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes
В письме от 17 апреля 2017 14:51:45 пользователь Fabien COELHO написал: > When developping new features for pgbench, I usually write some tests > which are lost when the feature is committed. Given that I have submitted > some more features and that part of pgbench code may be considered for > sharing with pgsql, I think that improving the abysmal state of tests > would be a good move. Hi! Since I used to be good at perl, I like tests, and I've dealt with postgres TAP tests before, I think I can review you patch. If it is OK for everyone. For now I've just gave this patch a quick look through... But nevertheless I have something to comment: > The attached patch: > > (1) extends the existing perl tap test infrastructure with methods to test > pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes" > which allows to check for expectations. I do not think it is good idea adding this functions to the PostgresNode.pm. They are pgbench specific. I do not think anybody will need them outside of pgbench tests. Generally, these functions as they are now, should be placed is separate .pm file. like it was done in src/test/ssl/ May be you should move some code into some kind of helpers and keep them in PostgresNode.pm. Or write universal functions that can be used for testing any postgres console tool. Then they can be placed in PostgresNode.pm > (3) add a lot of new very small tests so that coverage jumps from very low > to over 90% for source files. I think that derived files (exprparse.c, > exprscan.c) should be removed from coverage analysis. > > Previous coverage status: > > exprparse.y 0.0 % 0 / 770.0 %0 / 8 > exprscan.l 0.0 % 0 / 102 0.0 %0 / 8 > pgbench.c 28.3 % 485 / 1716 43.1 % 28 / 65 > > New status: > > exprparse.y 96.1 %73 / 76 100.0 % 8 / 8 > exprscan.l 92.8 %90 / 97 100.0 % 8 / 8 > pgbench.c 90.4 % 1542 / 1705 96.9 % 63 / 65 > > The test runtime is about doubled on my laptop, which is not too bad given > the coverage achieved. What tool did you use to calculate the coverage? Lots of small test looks good at first glance, except the point that adding lots of small files with pgbench scripts is not great idea. This makes code tree too overloaded with no practical reason. I am speaking of src/bin/pgbench/t/scripts/001_0* files. I think all the data from this file should be somehow imported into 001_pgbench.pl and these files should be created only when tests is running, and then removed when testing is done. I think that job for creating and removing these files should be moved to pgbench and pgbench_likes. But there is more then one way to do it. ;-) That's what I've noticed so far... I will give this patch more attentive look soon. -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[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.y0.0 % 0 / 770.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.y96.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.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index ae36247..2128418 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -229,7 +229,7 @@ typedef struct SimpleStats typedef struct StatsData { time_t start_time; /* interval start time, for aggregates */ - int64 cnt; /* number of transactions */ + int64 cnt; /* number of transactions, including skipped */ int64 skipped; /* number of transactions skipped under --rate * and --latency-limit */ SimpleStats latency; @@ -329,7 +329,7 @@ typedef struct bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */ /* per client collected stats */ - int64 cnt; /* transaction count */ + int64 cnt; /* client transaction count, for -t */ int ecnt; /* error count */ } CState; @@ -2045,7 +2045,9 @@ doCustom(TState *thread, CState *st, StatsData *agg) 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) + 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 */ @@ -2053,6 +2055,12 @@ doCustom(TState *thread, CState *st, StatsData *agg) thread->throttle_trigger += wait; st->txn_scheduled = thread->throttle_trigger; } + + if (nxacts > 0 && st->cnt >= nxacts) + { + st->state = CSTATE_FINISHED; + break; + } } st->state = CSTATE_THROTTLE; @@ -2366,13 +2374,13 @@ doCustom(TState *thread, CState *st, StatsData *agg) /* * transaction finished: calculate latency and log the - * transaction + * transaction. */ if (progress || throttle_delay || latency_limit || per_script_stats || use_log) processXactStats(thread, st, &now, false, agg); else - thread->stats.cnt++; + thread->stats.cnt++, st->cnt++; if (is_connect) { @@ -2381,7 +2389,6 @@ doCustom(TState *thread, CState *st, StatsData *agg) INSTR_TIME_SET_ZERO(now); } -++st->cnt; if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) { /* exit success */ @@ -2530,6 +2537,7 @@ processXactStats(TState *thread, CState *st, instr_time *now, lag = INSTR_TIME_GET_MICROSEC(st->txn_begin) - st->txn_scheduled; } + /* thread stats */ if (progress || throttle_delay || latency_limit) { accumStats(&thread->stats, skipped, latency, lag); @@ -2539,8 +2547,12 @@ processXactStats(TState *thread, CState *st, instr_time *now, thread->latency_late++; } else + /* just count */ thread->stats.cnt++; + /* client stat is just counting */ + st->cnt ++; + if (use_log) doLog(thread, st, agg, skipped, latency, lag); @@ -3118,7 +3130,7 @@ process_backslash_command(PsqlScanState sstate, const char *source) /* Save li