Re: pgbench - doCustom cleanup

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: "Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: pgbench - doCustom cleanup
Date: 2018-11-19 23:02:45
Message-ID: 20181119230245.h3lghyuggsyavmke@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2018-Nov-17, Fabien COELHO wrote:

> > Attached is a v3, where I have updated inaccurate comments.
> Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e

Attached v5. I thought that separating the part that executes the
command was an obvious readability improvement. Tests still pass,
though maybe I broke something inadvertently. (I started by thinking
"does this block require a 'fall-through' comment?" The 600 line
function is pretty hard to read with the multiple messy switches and
conditionals; split into 400 + 200 subroutine it's nicer to reason

Do we really use the word "move" to talk about state changes? It sounds
very odd to me. I would change that to "transition" -- would anybody
object to that? (Not changed in v5.)

On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
macro -- consider this:
if (foo)
Which "if" is the else now attached to? Now maybe the C standard has an
answer for that (I don't know what it is), but it's hard to read and
likely the compiler will complain anyway. I wrapped it in "do { }
while(0)" as is customary.

Álvaro Herrera
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
pgbench-state-change-5.patch text/x-diff 25.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2018-11-20 00:04:55 Re: Buildfarm failures for hash indexes: buffer leaks
Previous Message Andres Freund 2018-11-19 22:38:04 Re: Buildfarm failures for hash indexes: buffer leaks