Re: [PATCH] add long options to pgbench (submission 1)

This is mostly for reference to the next commitfest.

This very minor patch adds a corresponding long option to all short (one
letter) options of pgbench. In particular for connection options there is
now --host --username --port options similar to the "psql" client.

While I was at developing some small extensions to pgbench, ISTM that I
could do that without much effort.

Note that I'm not so sure about whether to chose singular or plural long
option names.

--
Fabien.

On Thu, May 2, 2013 at 1:59 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> This is mostly for reference to the next commitfest.
>
> This very minor patch adds a corresponding long option to all short (one
> letter) options of pgbench. In particular for connection options there is
> now --host --username --port options similar to the "psql" client.
>
> While I was at developing some small extensions to pgbench, ISTM that I
> could do that without much effort.

I don't really have an opinion on whether this is worth doing, but
we'd probably want to update all of our client utilities, not just
pgbench, if we did.

> Note that I'm not so sure about whether to chose singular or plural long
> option names.

I've wondered that myself.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello Robert,

>> This very minor patch adds a corresponding long option to all short
>> (one letter) options of pgbench. [...]
>
> I don't really have an opinion on whether this is worth doing, but we'd
> probably want to update all of our client utilities, not just pgbench,
> if we did.

The current status is that "official" clients already have systematic long
options. I have checked: psql, pg_dump, pg_dumpall, pg_restore,
pg_basebackup, createdb, createuser, createlang. Possibly other commands
in contrib do not have long option.

As for the rational, when I type interactively I tend to use short
options, but in a script I like long options so that I may not need to
look them up in the documentation too often when reading the script later.
The other rational is that adding long options is cheap and
straightforward.

--
Fabien.

New submission which put long options in alphabetical order, which seems
more logical.

This is for reference to the next commitfest.

--
Fabien.

> New submission which put long options in alphabetical order, which seems
> more logical.
>
> This is for reference to the next commitfest.

When I applied your patch appeared the following messages:

$ patch -p1 < /tmp/pgbench-longopts.patch
patching file contrib/pgbench/pgbench.c
Hunk #1 succeeded at 2109 (offset 29 lines).
patching file doc/src/sgml/pgbench.sgml
Hunk #2 succeeded at 172 (offset 12 lines).
Hunk #3 FAILED at 170.
Hunk #4 succeeded at 193 (offset 11 lines).
Hunk #5 FAILED at 199.
Hunk #13 succeeded at 344 (offset -33 lines).
Hunk #14 succeeded at 367 (offset -33 lines).
Hunk #15 succeeded at 382 (offset -33 lines).
Hunk #16 succeeded at 395 (offset -33 lines).
Hunk #17 succeeded at 407 (offset -33 lines).
Hunk #18 succeeded at 422 (offset -33 lines).
Hunk #19 succeeded at 432 (offset -33 lines).
Hunk #20 succeeded at 442 (offset -33 lines).
Hunk #21 succeeded at 454 (offset -33 lines).
2 out of 24 hunks FAILED -- saving rejects to file
doc/src/sgml/pgbench.sgml.rej

Please fix that and re-send the patch.

Regards,

-----
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
--
View this message in context: http://postgresql.1045698.n5.nabble.com/PATCH-add-long-options-to-pgbench-submission-1-tp5754115p5760039.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

> Please fix that and re-send the patch.

Find attached diff wrt current master.

--
Fabien.

On Thu, Jun 20, 2013 at 9:59 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:

>
> Please fix that and re-send the patch.
>>
>
> Find attached diff wrt current master.
>
>
Thanks.

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Thu, Jun 20, 2013 at 9:17 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Thu, Jun 20, 2013 at 9:59 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>>> Please fix that and re-send the patch.
>> Find attached diff wrt current master.
> Thanks.

I would like to solicit opinions on whether this is a good idea. I
understand that the patch author thinks it's a good idea, and I don't
have a strong position either way. But I want to hear what other
people think.

Anyone have an opinion?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-06-25 12:11:06 -0400, Robert Haas wrote:
> On Thu, Jun 20, 2013 at 9:17 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> > On Thu, Jun 20, 2013 at 9:59 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> >>> Please fix that and re-send the patch.
> >> Find attached diff wrt current master.
> > Thanks.
>
> I would like to solicit opinions on whether this is a good idea. I
> understand that the patch author thinks it's a good idea, and I don't
> have a strong position either way. But I want to hear what other
> people think.
>
> Anyone have an opinion?

+1. When writing scripts I like to use the long options because that
makes it easier to understand some time later. I don't really see a
reason not to do this.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas escribió:
> On Thu, Jun 20, 2013 at 9:17 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> > On Thu, Jun 20, 2013 at 9:59 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> >>> Please fix that and re-send the patch.
> >> Find attached diff wrt current master.
> > Thanks.
>
> I would like to solicit opinions on whether this is a good idea. I
> understand that the patch author thinks it's a good idea, and I don't
> have a strong position either way. But I want to hear what other
> people think.

+1 from me on the general idea. Didn't check the actual patch.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I would like to solicit opinions on whether this is a good idea. I
> understand that the patch author thinks it's a good idea, and I don't
> have a strong position either way. But I want to hear what other
> people think.

If it makes pgbench more consistent with psql's command line options,
it seems reasonable to me.

regards, tom lane

On Tue, Jun 25, 2013 at 12:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I would like to solicit opinions on whether this is a good idea. I
>> understand that the patch author thinks it's a good idea, and I don't
>> have a strong position either way. But I want to hear what other
>> people think.
>
> If it makes pgbench more consistent with psql's command line options,
> it seems reasonable to me.

OK, I think it does that. So that's three votes in favor and none
opposed. I think I'd like to quibble with some of the names a bit,
though. The patch adds --fill-factor, but I think we should spell it
without the hyphen: --fillfactor. I think --quiet-log should be
spelled --quiet. I think --connect for each connection is not very
descriptive; maybe --connection-per-transaction or something, although
that's kind of long. I think -M should be aliased to --protocol, not
--query-mode. --skip-some-update is incorrectly pluralized; if that's
what we're going to use, it should be --skip-some-updates.
Alternatively, we could use --update-large-tables-only, which might
make the intent more clear.

On another note, it doesn't look like this updates the output of
pgbench --help, which seems important.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> I think I'd like to quibble with some of the names a bit, though.

That is a good idea, because I'm not a native English speaker and I was
not so sure for many options.

> The patch adds --fill-factor, but I think we should spell it
> without the hyphen: --fillfactor.

Fine with me.

> I think --quiet-log should be spelled --quiet.

ISTM that --quiet usually means "not verbose on stdout", so I added log
because this was specific to the log output, and that there may be a need
for a --quiet option for stdout at some time.

> I think --connect for each connection is not very descriptive; maybe
> --connection-per-transaction or something, although that's kind of long.

Yes, I think that it is too long. You have to type them!

What about '--reconnect'?

> I think -M should be aliased to --protocol, not --query-mode.

Fine with me.

> --skip-some-update is incorrectly pluralized; if that's
> what we're going to use, it should be --skip-some-updates.

Indeed.

> Alternatively, we could use --update-large-tables-only, which might
> make the intent more clear.

Yep, but quite long.

> On another note, it doesn't look like this updates the output of
> pgbench --help, which seems important.

Indeed, it should.

Please find attached a v4 which takes into account most of your comments,
but "very very long" option names.

--
Fabien.

On Tue, Jun 25, 2013 at 3:09 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>> I think --quiet-log should be spelled --quiet.
>
> ISTM that --quiet usually means "not verbose on stdout", so I added log
> because this was specific to the log output, and that there may be a need
> for a --quiet option for stdout at some time.

The output that is quieted by -q is not the log output produced by
--log; it's the regular progress output on stdout/stderr.

So I changed that, and committed this, with some further cosmetic
changes. I made the formatting of the help message more like psql's
help message, including adjusting pgbench to start the description of
each option in the same column that psql does. This got rid of a lot
of line breaks and IMHO makes the output of pgbench --help quite a bit
more readable. I made stylistic adjustments to the documentation
portion of the patch as well, again to match the markup used for psql.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 27, 2013 at 10:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jun 25, 2013 at 3:09 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>>> I think --quiet-log should be spelled --quiet.
>>
>> ISTM that --quiet usually means "not verbose on stdout", so I added log
>> because this was specific to the log output, and that there may be a need
>> for a --quiet option for stdout at some time.
>
> The output that is quieted by -q is not the log output produced by
> --log; it's the regular progress output on stdout/stderr.
>
> So I changed that, and committed this, with some further cosmetic
> changes. I made the formatting of the help message more like psql's
> help message, including adjusting pgbench to start the description of
> each option in the same column that psql does. This got rid of a lot
> of line breaks and IMHO makes the output of pgbench --help quite a bit
> more readable. I made stylistic adjustments to the documentation
> portion of the patch as well, again to match the markup used for psql.

In help messages:

+ " -s NUM, --scale NUM scaling factor\n"

This should be "-s, --scale=NUM" for the sake of consistency with other
options.

Regards,

--
Fujii Masao

On Thu, Jun 27, 2013 at 10:24 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Jun 27, 2013 at 10:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Jun 25, 2013 at 3:09 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>>>> I think --quiet-log should be spelled --quiet.
>>>
>>> ISTM that --quiet usually means "not verbose on stdout", so I added log
>>> because this was specific to the log output, and that there may be a need
>>> for a --quiet option for stdout at some time.
>>
>> The output that is quieted by -q is not the log output produced by
>> --log; it's the regular progress output on stdout/stderr.
>>
>> So I changed that, and committed this, with some further cosmetic
>> changes. I made the formatting of the help message more like psql's
>> help message, including adjusting pgbench to start the description of
>> each option in the same column that psql does. This got rid of a lot
>> of line breaks and IMHO makes the output of pgbench --help quite a bit
>> more readable. I made stylistic adjustments to the documentation
>> portion of the patch as well, again to match the markup used for psql.
>
> In help messages:
>
> + " -s NUM, --scale NUM scaling factor\n"
>
> This should be "-s, --scale=NUM" for the sake of consistency with other
> options.

Woops, missed that one. Fixed, thanks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> [...] So I changed that, and committed this, with some further cosmetic
> changes. [...]

Thanks for the commit & the style improvements.

For the text formatting, I tried to keep the screen width under 80
characters, because if the line is too wide it is harder to read as the
eye may loose the alignment. But being homogeneous with other commands is
fine with me as well.

--
Fabien.

Fabien COELHO escribió:

> For the text formatting, I tried to keep the screen width under 80
> characters, because if the line is too wide it is harder to read as
> the eye may loose the alignment. But being homogeneous with other
> commands is fine with me as well.

The format chosen by Robert fits in 80 columns and looks very good to
me. Thanks to both.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services