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

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

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

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

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

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

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

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

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

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.

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

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

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,

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).

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

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

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

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.

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

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

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

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

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

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

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

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

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.

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, > > > > -

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, -

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

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

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

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

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 +++

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,

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

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

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

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

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

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

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.

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

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

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 >