pgbench exit code

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

pgbench exit code

Peter Eisentraut-6
In pgbench, when an error occurs during a benchmark run (SQL error,
connection error, etc.) it just prints an error message and then
proceeds to print the run summary normally and exits with status 0.  So
this is quite inconvenient, especially from a script.

The attached patch changes this so that it exits with status 1 and also
prints a line below the summary advising that the results are incomplete.

The test suite had, curiously, encoded the previous behavior, checking
for exit status 0 vs 1 in various error situations.  In this patch, I
have just updated the expected results (which are all "1" now), but
perhaps we should remove listing that individually and just check for
nonzero in all cases somewhere centrally.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-pgbench-Report-errors-during-run-better.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Fabien COELHO-3

Hello Peter,

> In pgbench, when an error occurs during a benchmark run (SQL error,
> connection error, etc.) it just prints an error message and then
> proceeds to print the run summary normally and exits with status 0.  So
> this is quite inconvenient, especially from a script.

Yep. I'm fine with changing this behavior... last time I suggested
something like that, or probably something more drastic like existing
immediately when there is little sense to go on, it was not approved, some
people want the report anyway.

Your approach of not changing the output too much but changing the exit
status and adding a warning may get through more easily.

Note that there is some logic in distinguishing between different type of
errors (before the bench start vs the bench has started), so I'd suggest
that the exit status should be different.

> The attached patch changes this so that it exits with status 1 and also
> prints a line below the summary advising that the results are incomplete.
>
> The test suite had, curiously, encoded the previous behavior,

Sure, the point of the tests is to check for unwanted regressions. It does
not mean that the behavior is ideal for all use cases.

Patch applies cleanly, compiles, global & local "make check" are ok.

The abort is by a client, but the code seems to only check the first
client of a thread. ISTM that if the second or later client abort it may
not be detected? Probably an intermediate aggregation at the thread level
is needed, or maybe a global variable, or as errors are counted somewhere,
it may be enough just to check that the count is non zero?

  -               [ $status ? qr{^$} : qr{processed: 0/1} ],
  +               [],


The patch removes a check that there was an output and that no
transactions where processed. ISTM it should be kept. If a different exit
status is chosen on abort, that would allow to keep it easily.

Probably there should be some documentation changes as well?

Note that the patch probably interferes in small ways with Marina's patch
for retrying on serialization or deadlock errors, see:

  https://commitfest.postgresql.org/18/1645/

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Fabien COELHO-3

>> The attached patch changes this so that it exits with status 1 and also
>> prints a line below the summary advising that the results are incomplete.

BTW, I have added this patch to the next CF:

  https://commitfest.postgresql.org/19/1751/

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Michael Paquier-2
On Fri, Aug 10, 2018 at 01:50:49PM +0200, Fabien COELHO wrote:
> BTW, I have added this patch to the next CF:
>
> https://commitfest.postgresql.org/19/1751/

The latest patch does not apply anymore.  Peter, could you rebase it?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Michael Paquier-2
On Mon, Oct 01, 2018 at 11:29:19AM +0900, Michael Paquier wrote:
> On Fri, Aug 10, 2018 at 01:50:49PM +0200, Fabien COELHO wrote:
>> BTW, I have added this patch to the next CF:
>>
>> https://commitfest.postgresql.org/19/1751/
>
> The latest patch does not apply anymore.  Peter, could you rebase it?

On top of that, I just noticed that the patch has been waiting for input
from the author for a couple of weeks, so the thing is returned with
feedback for now.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Peter Eisentraut-6
In reply to this post by Fabien COELHO-3
On 10/08/2018 01:41, Fabien COELHO wrote:
> Your approach of not changing the output too much but changing the exit
> status and adding a warning may get through more easily.
>
> Note that there is some logic in distinguishing between different type of
> errors (before the bench start vs the bench has started), so I'd suggest
> that the exit status should be different.

done

> The abort is by a client, but the code seems to only check the first
> client of a thread. ISTM that if the second or later client abort it may
> not be detected? Probably an intermediate aggregation at the thread level
> is needed, or maybe a global variable, or as errors are counted somewhere,
> it may be enough just to check that the count is non zero?

fixed

>   -               [ $status ? qr{^$} : qr{processed: 0/1} ],
>   +               [],
>
> The patch removes a check that there was an output and that no
> transactions where processed. ISTM it should be kept. If a different exit
> status is chosen on abort, that would allow to keep it easily.

fixed

> Probably there should be some documentation changes as well?

done

New patched attached.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v2-0001-pgbench-Report-errors-during-run-better.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Fabien COELHO-3

Hello Peter,

>> The abort is by a client, but the code seems to only check the first
>> client of a thread. ISTM that if the second or later client abort it may
>> not be detected? Probably an intermediate aggregation at the thread level
>> is needed, or maybe a global variable, or as errors are counted somewhere,
>> it may be enough just to check that the count is non zero?
>
> fixed

With an aggregation. Fine with me.

>> The patch removes a check that there was an output and that no
>> transactions where processed. ISTM it should be kept. If a different exit
>> status is chosen on abort, that would allow to keep it easily.
>
> fixed

Ok.

>> Probably there should be some documentation changes as well?
>
> done

Ok.

Patch applies cleanly, compiles, global & local "make check" are ok.
Doc build is okay.

I noticed "for (int i"… pgbench is welcome to C99:-)

No further remarks. I turned the patch as ready on the CF app.

Attached a file I used for some manual tests.

--
Fabien.

bad-sql-proba.sql (112 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench exit code

Peter Eisentraut-6
On 12/10/2018 21:22, Fabien COELHO wrote:
> No further remarks. I turned the patch as ready on the CF app.

Committed, thanks.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services