Date: Thu, 17 Jun 2021 09:24:37 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Thomas Munro <thomas.munro@gmail.com>
cc: Gregory Smith <gregsmithpgsql@gmail.com>,
    PostgreSQL-development <pgsql-hackers@postgresql.org>,
    david.christensen@crunchydata.com
Subject: Re: pgbench logging broken by time logic changes
In-Reply-To: 
 <CA+hUKG+jhatozihmqy4=Ufd0OHwZadEd_vg-_+26hE3f4C3TgQ@mail.gmail.com>
Message-ID: <alpine.DEB.2.22.394.2106170917240.2693553@pseudo>
References: 
 <CAHLJuCW_8Vpcr0=t6O_gozrg3wXXWXZXDioYJd3NhvKriqgpfQ@mail.gmail.com>
 <CA+hUKG+jhatozihmqy4=Ufd0OHwZadEd_vg-_+26hE3f4C3TgQ@mail.gmail.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="2056645051-945376642-1623914677=:2693553"
Archived-At: 
 <https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2106170917240.2693553%40pseudo>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--2056645051-945376642-1623914677=:2693553
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT


Hello Thomas,

>> Before I could get to startup timing I noticed the pgbench logging 
>> output was broken via commit 547f04e7 "Improve time logic": 
>> https://www.postgresql.org/message-id/E1lJqpF-00064e-C6%40gemulon.postgresql.org
>
> It does suck that we broke the logging and that it took 3 months for
> anyone to notice and report it to the list.

Indeed. Well, it also demonstrates that beta are useful.

> Seems like it should be straightforward to fix, though, with fixes 
> already proposed (though I haven't studied them yet, will do).

I think that fixing logging is simple enough, thus a revert is not 
necessary.

>> 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.
>
> It's true that this work produced a few rounds of small portability
> follow-ups: c427de42 (work around strange hacks elsewhere in the tree
> for AIX), 68b34b23 (missing calling convention specifier on Windows),
> and de91c3b9 (adjust pthread missing-function code for threadless
> builds).  These were problems that didn't show up on developer or CI
> systems (including threadless and Windows), and IMHO are typical sorts
> of problems you expect to have to work through when stuff hits the
> build farm, especially when using new system interfaces.  So I don't
> think any of that, on its own, supports reverting anything here.

Yep, the buildfarm is here to catch portability issues, and it does its 
job:-) There is no doubt that logging is has been broken because of lack 
of tests in this area, shame on us. I think it is easy to fix.

> [...] For that reason, I'm not super attached to that new pg_time_usec_t 
> stuff at all, and wouldn't be sad if we reverted that piece.

Well, I was sooo happy to get rid of INSTR_TIME ugly and inefficient 
macros in pgbench… so anything looks better to me.

Note that Michaël is having a look at fixing pgbench logging issues.

-- 
Fabien.
--2056645051-945376642-1623914677=:2693553--