Re: pgbench logging broken by time logic changes

On 6/16/21 2:59 PM, Fabien COELHO wrote:
>
> Hello Greg,
>
>> I have a lot of community oriented work backed up behind this right
>> now, so
>> I'm gonna be really honest.  This time rework commit in its current form
>> makes me uncomfortable at this point in the release schedule.  The
>> commit
>> has already fought through two rounds of platform specific bug
>> fixes.  But
>> since the buildfarm doesn't test the logging feature, that whole
>> process is
>> suspect.
>
> Logging is/should going to be fixed.
>
>> My take on the PostgreSQL way to proceed:  this bug exposes that pgbench
>> logging is a feature we finally need to design testing for.
>
> Sure.
>
> The key feedback for me is the usual one: what is not tested does not
> work. Wow:-)


Agreed.


>
> I'm unhappy because I already added tap tests for time-sensitive
> features (-T and others, maybe logging aggregates, cannot remember),
> which have been removed because they could fail under some
> circonstances (eg very very very very slow hosts), or required some
> special handling (a few lines of code) in pgbench, and the net result
> of this is there is not a single test in place for some features:-(


I'm not familiar with exactly what happened in this case, but tests need
to be resilient over a wide range of performance characteristics. One
way around this issue might be to have a way of detecting that it's on a
slow platform and if so either skipping tests (Test::More provides
plenty of support for this) or expecting different results.


>
> There is no problem with proposing tests, the problem is that they are
> accepted, or if they are accepted then that they are not removed at
> the first small issue but rather fixed, or their limitations accepted,
> because testing time-sensitive features is not as simple as testing
> functional features.
>
> Note that currently there is not a single test of psql with autocommit
> off or with "on error rollback". Last time a submitted tap tests to
> raise psql test coverage from 50% to 90%, it was rejected. I'll admit
> that I'm tired arguing that more testing is required, and I'm very
> happy if someone else is ready to try again. Good luck! :-)


:-(


>
>> We need a new buildfarm test and then a march through a full release
>> phase to see how it goes.
>
>> Only then should we start messing with the time logic.  Even if we
>> fixed the source today on both my test platforms, I'd still be
>> nervous that beta 2 could ship and more performance testing could
>> fall over from this modification.  And that's cutting things a little
>> close.
>
> Well, the point beta is to discover bugs not caught by reviews and dev
> tests, fix them, and possibly add tests which would have caught them.
>
> If you revert all features on the first issue in a corner case and put
> it back to the queue, then I do not see why the review and dev tests
> will be much better on the next round, so it does not really help
> moving things forward.
>
> IMHO, the pragmatic approach is to look at fixing first, and maybe
> revert if the problems are deep. I'm not sure this is obviously the
> case here.



It does look like the submitted fix basically reverts the changes w.r.t.
this timestamp logging.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com