Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 3/28/20 5:27 AM, Fabien COELHO wrote: Hello Tom, Thanks for your feedback, I'd be rather unclear about what the actual feedback is, though. I'd interpret it as "pg does not care much about code coverage". Most clients are in the red on coverage.postgresql.org. I'd like pgbench at least

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Tom, Thanks for your feedback, I'd be rather unclear about what the actual feedback is, though. I'd interpret it as "pg does not care much about code coverage". Most clients are in the red on coverage.postgresql.org. I'd like pgbench at least to be in the green, but it does not look

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Fabien COELHO writes: >> It does not look like the remainder of this patch is going to be committed >> and I don't think it makes sense to keep moving the patch indefinitely. >> Unless something changes by the end of this CF I'll mark it Returned With >> Feedback. > I'd be rather unclear about

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

This patch is registered in 2020-01, but the last message in the thread seems to be from 2019/05/23. The patch itself seems to be OK (it applies fine etc.) What do we need to get it over the line, instead of just moving it to the next one CF over and over? It does not look like the remainder

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hi, This patch is registered in 2020-01, but the last message in the thread seems to be from 2019/05/23. The patch itself seems to be OK (it applies fine etc.) What do we need to get it over the line, instead of just moving it to the next one CF over and over? regards -- Tomas Vondra

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

V11 is just a rebase after the reindentation patches. Indeed, yet again, I forgot the attachement:-( I stared at the new test case for a while, and I must say it looks very cryptic. It's not exactly this patch's fault - all the pgbench tests are cryptic - Perl is cryptic. Regexprs are

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, Indeed, yet again, I forgot the attachement:-( I stared at the new test case for a while, and I must say it looks very cryptic. It's not exactly this patch's fault - all the pgbench tests are cryptic - Perl is cryptic. Regexprs are cryptic. but I think we need to do

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, I stared at the new test case for a while, and I must say it looks very cryptic. It's not exactly this patch's fault - all the pgbench tests are cryptic - Perl is cryptic. Regexprs are cryptic. but I think we need to do something about that before adding any more tests. I'm

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 25/03/2019 20:13, Fabien COELHO wrote: Attached is the remainder of the patch rebased to current head. I stared at the new test case for a while, and I must say it looks very cryptic. It's not exactly this patch's fault - all the pgbench tests are cryptic - but I think we need to do

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

I pushed the refactoring now, to extract the progress reporting to a separate function. That seems like an improvement independently from the rest of the patch. Sure. I'll take another look at the rest, probably tomorrow. Ok. Attached is the remainder of the patch rebased to current

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 10/03/2019 13:26, Fabien COELHO wrote: Attached is a rebase. I pushed the refactoring now, to extract the progress reporting to a separate function. That seems like an improvement independently from the rest of the patch. I'll take another look at the rest, probably tomorrow. - Heikki

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Attached is a rebase. -- Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 5df54a8e57..1db6b75823 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -6032,6 +6032,96 @@ main(int argc, char **argv) return exit_code; } +/* display the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Tom, Unfortunately, there was no activity over the last few commitfests and the proposed patch pgbench-tap-progress-6 can't be applied anymore without conflicts. Fabien, what are your plans about it, could you please post a rebased version? Here it is. I'm confused about the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Fabien COELHO writes: >> Unfortunately, there was no activity over the last few commitfests and the >> proposed patch pgbench-tap-progress-6 can't be applied anymore without >> conflicts. Fabien, what are your plans about it, could you please post a >> rebased version? > Here it is. I'm

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On Sat, Nov 24, 2018 at 05:58:38PM +0100, Fabien COELHO wrote: >> Unfortunately, there was no activity over the last few commitfests and the >> proposed patch pgbench-tap-progress-6 can't be applied anymore without >> conflicts. Fabien, what are your plans about it, could you please post a >>

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Unfortunately, there was no activity over the last few commitfests and the proposed patch pgbench-tap-progress-6 can't be applied anymore without conflicts. Fabien, what are your plans about it, could you please post a rebased version? Here it is. -- Fabien.diff --git

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

> On Thu, Jul 19, 2018 at 2:24 AM Fabien COELHO wrote: > > > Hello Heikki, > > >> [...] > >> So threadRun() would not have the opportunity to stop the scheduled > >> transaction, even if beyond the end of run, because it would not have got > >> out of doCustom, in the case I outlined above. > > >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, [...] So threadRun() would not have the opportunity to stop the scheduled transaction, even if beyond the end of run, because it would not have got out of doCustom, in the case I outlined above. I see. Instead of moving to FINISHED state, then, we could stay in THROTTLE state,

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 18/07/18 23:29, Fabien COELHO wrote: Hmm. How about we just remove this special case from doCustom(): case CSTATE_START_THROTTLE: // ... if (duration > 0 && st->txn_scheduled > end_time) { st->state = CSTATE_FINISHED; break; } That way, we let the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hmm. How about we just remove this special case from doCustom(): case CSTATE_START_THROTTLE: // ... if (duration > 0 && st->txn_scheduled > end_time) { st->state = CSTATE_FINISHED; break; } That way, we let the client go into CSTATE_THROTTLE state, even

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 18/07/18 22:56, Fabien COELHO wrote: Hello Heikki, Yep. The attached version does only the tailing stuff under -R and not all threads were stopped on errors, with comments to tell about the why. Hmm. How about we just remove this special case from doCustom(): case

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, Yep. The attached version does only the tailing stuff under -R and not all threads were stopped on errors, with comments to tell about the why. Hmm. How about we just remove this special case from doCustom(): case CSTATE_START_THROTTLE: // ... if (duration > 0 &&

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 18/07/18 16:01, Fabien COELHO wrote: I don't think you want to wait in that situation. I think we should wait at the end only if there some threads still alive, with nothing to do only because of --rate. Yep. The attached version does only the tailing stuff under -R and not all threads were

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, I did that in the attached version: no more environment variable hack, and no execution shortcut even if there is nothing to do. I also had to reproduce the progress logic to keep on printing report of (no) progress in this tailing phase. On second thoughts, there's one

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 18/07/18 01:43, Fabien COELHO wrote: The more reasonable alternative could be to always last 2 seconds under -T 2, even if the execution can be shorten because there is nothing to do at all, i.e. remove the environment-based condition but keep the sleep. That sounds reasonable. It's a bit

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, [...] Let's keep it that way. I think the only change we need to make in the logic is to check at the end, if *any* progress reports at all have been printed, and print one if not. Ok, this simplifies the condition. And do that only when the -P option is smaller than the -T

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 12/07/18 21:27, Fabien COELHO wrote: For the testing, we just need to make sure that at least one progress report is always printed, if -P is used. Right? Yep. That is the first condition above the last_report is set to thread_start meaning that there has been no report. So where does the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Indeed… but then throttling would not be tested:-) The point of the test is to exercise all time-related options, including throttling with a reasonable small value. Ok. I don't think that's really worthwhile. If we add some code that only runs in testing, then we're not really testing the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

I don't understand the 0.5 second rule. For the tests, we only need to ensure that at least one progress report is printed, right? [...] I still don't understand. Let's look at the code: if (progress && thread->tid == 0) { ... if (last_report == thread_start || now -

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

On 12/07/18 19:00, Fabien COELHO wrote: How pgbenchs prints a progress if none were printed, or if the last progress was over 0.5 seconds ago, so as to have kind of a catchup in the end. I don't understand the 0.5 second rule. For the tests, we only need to ensure that at least one progress

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Hello Heikki, Thanks for having a look at this small patch which aim at improving pgbench coverage. How pgbenchs prints a progress if none were printed, or if the last progress was over 0.5 seconds ago, so as to have kind of a catchup in the end. I don't understand the 0.5 second rule.

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Minor rebase after vpath fix (e94f2bc809a0c684185666f19d81f6496e732a3a). -- Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 5c07dd9..1fd2451 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -5163,6 +5163,96 @@ main(int argc, char **argv)

Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)

Fabien COELHO writes: > Attached is an attempt at improving the situation: I took a quick look at this and can't really convince myself that it improves our situation. The fact that it prints nothing if the runtime is outside of a tightly constrained range seems likely to