On Fri, Aug 24, 2018 at 01:32:47PM +0900, Tatsuro Yamada wrote: > I revised the patch and created new tap tests.
Thanks, I have looked at the patch set. And here are some notes about issues found while reviewing. For oid2name: - Documentation had some typos, --tablename was used instead of --table. - Using plural forms for some options makes more sense to me, like --indexes, --system-objects, --tablespaces - I have tweaked the description of -H in usage(). - An '=' was missing for --filenode - I have reordered the options in alphabetical order. - Added some basic tap tests, and added correct handling of the tests in oid2name/Makefile. - Inclusion of getopt_long.h has been missing, this would have caused failures on Windows. - getopt_long() has been reordered to be alphabetical. Same thing for long_options[]. This has the advantage to ease the review and code read. - Some formatting issues with option docs, leading to some noise diffs. For vacuumlo: - Some typos in documentation. - Documentation format was rather weird for some options, so I made the whole consistent. - I agree with the option names you put. - Reorganization of the options. - Added basic TAP tests, and fixed Makefile. - default clause was missing from the option set. Attached are updated patches, which I would be fine to commit. What do you think? -- Michael
From 6f2a0931ddf9ab1b742d0bbc6bac3930d982b60b Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 27 Aug 2018 19:00:51 +0900 Subject: [PATCH 1/2] Rework option set of oid2name oid2name has done little effort to keep an interface consistent with other binary utilities: - -H was used instead of -h/-host. This option is now marked as deprecated, still its output is accepted to be backward-compatible. - -P has been removed from the code, and was still documented. - All options gain long aliases, making connection options more similar to other binaries. - Document environment variables which could be used: PGHOST, PGPORT and PGUSER. A basic set of TAP tests is added on the way. Author: Tatsuro Yamada Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/c7e7f25c-1747-cd0f-9335-390bc97b2...@lab.ntt.co.jp --- contrib/oid2name/.gitignore | 2 + contrib/oid2name/Makefile | 6 ++ contrib/oid2name/oid2name.c | 124 ++++++++++++++++++-------------- contrib/oid2name/t/001_basic.pl | 12 ++++ doc/src/sgml/oid2name.sgml | 81 +++++++++++++++------ 5 files changed, 151 insertions(+), 74 deletions(-) create mode 100644 contrib/oid2name/t/001_basic.pl diff --git a/contrib/oid2name/.gitignore b/contrib/oid2name/.gitignore index fdefde108d..0410fb7afa 100644 --- a/contrib/oid2name/.gitignore +++ b/contrib/oid2name/.gitignore @@ -1 +1,3 @@ /oid2name + +/tmp_check/ diff --git a/contrib/oid2name/Makefile b/contrib/oid2name/Makefile index 3eef8f60be..77b72880f5 100644 --- a/contrib/oid2name/Makefile +++ b/contrib/oid2name/Makefile @@ -19,3 +19,9 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c index 63e360c4c5..aa122ca0e9 100644 --- a/contrib/oid2name/oid2name.c +++ b/contrib/oid2name/oid2name.c @@ -14,6 +14,8 @@ #include "fe_utils/connect.h" #include "libpq-fe.h" #include "pg_getopt.h" +#include "getopt_long.h" + /* an extensible array to keep track of elements to show */ typedef struct @@ -60,8 +62,28 @@ void sql_exec_dumpalltbspc(PGconn *, struct options *); void get_opts(int argc, char **argv, struct options *my_opts) { + static struct option long_options[] = { + {"dbname", required_argument, NULL, 'd'}, + {"host", required_argument, NULL, 'h'}, + {"host", required_argument, NULL, 'H'}, /* deprecated */ + {"filenode", required_argument, NULL, 'f'}, + {"indexes", no_argument, NULL, 'i'}, + {"oid", required_argument, NULL, 'o'}, + {"port", required_argument, NULL, 'p'}, + {"quiet", no_argument, NULL, 'q'}, + {"tablespaces", no_argument, NULL, 's'}, + {"system-objects", no_argument, NULL, 'S'}, + {"table", required_argument, NULL, 't'}, + {"username", required_argument, NULL, 'U'}, + {"version", no_argument, NULL, 'V'}, + {"extended", no_argument, NULL, 'x'}, + {"help", no_argument, NULL, '?'}, + {NULL, 0, NULL, 0} + }; + int c; const char *progname; + int optindex; progname = get_progname(argv[0]); @@ -93,7 +115,7 @@ get_opts(int argc, char **argv, struct options *my_opts) } /* get opts */ - while ((c = getopt(argc, argv, "H:p:U:d:t:o:f:qSxish")) != -1) + while ((c = getopt_long(argc, argv, "d:f:h:H:io:p:qsSt:U:x", long_options, &optindex)) != -1) { switch (c) { @@ -102,54 +124,35 @@ get_opts(int argc, char **argv, struct options *my_opts) my_opts->dbname = pg_strdup(optarg); break; - /* specify one tablename to show */ - case 't': - add_one_elt(optarg, my_opts->tables); - break; - - /* specify one Oid to show */ - case 'o': - add_one_elt(optarg, my_opts->oids); - break; - /* specify one filenode to show */ case 'f': add_one_elt(optarg, my_opts->filenodes); break; - /* don't show headers */ - case 'q': - my_opts->quiet = true; - break; - /* host to connect to */ - case 'H': + case 'H': /* deprecated */ + case 'h': my_opts->hostname = pg_strdup(optarg); break; - /* port to connect to on remote host */ - case 'p': - my_opts->port = pg_strdup(optarg); - break; - - /* username */ - case 'U': - my_opts->username = pg_strdup(optarg); - break; - - /* display system tables */ - case 'S': - my_opts->systables = true; - break; - /* also display indexes */ case 'i': my_opts->indexes = true; break; - /* display extra columns */ - case 'x': - my_opts->extended = true; + /* specify one Oid to show */ + case 'o': + add_one_elt(optarg, my_opts->oids); + break; + + /* port to connect to on remote host */ + case 'p': + my_opts->port = pg_strdup(optarg); + break; + + /* don't show headers */ + case 'q': + my_opts->quiet = true; break; /* dump tablespaces only */ @@ -157,9 +160,24 @@ get_opts(int argc, char **argv, struct options *my_opts) my_opts->tablespaces = true; break; - case 'h': - help(progname); - exit(0); + /* display system tables */ + case 'S': + my_opts->systables = true; + break; + + /* specify one tablename to show */ + case 't': + add_one_elt(optarg, my_opts->tables); + break; + + /* username */ + case 'U': + my_opts->username = pg_strdup(optarg); + break; + + /* display extra columns */ + case 'x': + my_opts->extended = true; break; default: @@ -176,20 +194,22 @@ help(const char *progname) "Usage:\n" " %s [OPTION]...\n" "\nOptions:\n" - " -d DBNAME database to connect to\n" - " -f FILENODE show info for table with given file node\n" - " -H HOSTNAME database server host or socket directory\n" - " -i show indexes and sequences too\n" - " -o OID show info for table with given OID\n" - " -p PORT database server port number\n" - " -q quiet (don't show headers)\n" - " -s show all tablespaces\n" - " -S show system objects too\n" - " -t TABLE show info for named table\n" - " -U NAME connect as specified database user\n" - " -V, --version output version information, then exit\n" - " -x extended (show additional columns)\n" - " -?, --help show this help, then exit\n" + " -f, --filenode=FILENODE show info for table with given file node\n" + " -i, --indexes show indexes and sequences too\n" + " -o, --oid=OID show info for table with given OID\n" + " -q, --quiet quiet (don't show headers)\n" + " -s, --tablespaces show all tablespaces\n" + " -S, --system-objects show system objects too\n" + " -t, --table=TABLE show info for named table\n" + " -V, --version output version information, then exit\n" + " -x, --extended extended (show additional columns)\n" + " -?, --help show this help, then exit\n" + "\nConnection options:\n" + " -d, --dbname=DBNAME database to connect to\n" + " -h, --host=HOSTNAME database server host or socket directory\n" + " -H same as -h, deprecated option\n" + " -p, --port=PORT database server port number\n" + " -U, --username=USERNAME connect as specified database user\n" "\nThe default action is to show all database OIDs.\n\n" "Report bugs to <pgsql-b...@postgresql.org>.\n", progname, progname); diff --git a/contrib/oid2name/t/001_basic.pl b/contrib/oid2name/t/001_basic.pl new file mode 100644 index 0000000000..fa2c5743f6 --- /dev/null +++ b/contrib/oid2name/t/001_basic.pl @@ -0,0 +1,12 @@ +use strict; +use warnings; + +use TestLib; +use Test::More tests => 8; + +######################################### +# Basic checks + +program_help_ok('oid2name'); +program_version_ok('oid2name'); +program_options_handling_ok('oid2name'); diff --git a/doc/src/sgml/oid2name.sgml b/doc/src/sgml/oid2name.sgml index dd875281c8..536682ac5a 100644 --- a/doc/src/sgml/oid2name.sgml +++ b/doc/src/sgml/oid2name.sgml @@ -60,41 +60,48 @@ <variablelist> <varlistentry> - <term><option>-f</option> <replaceable>filenode</replaceable></term> - <listitem><para>show info for table with filenode <replaceable>filenode</replaceable></para></listitem> + <term><option>-f <replaceable class="parameter">filenode</replaceable></option></term> + <term><option>--filenode=<replaceable class="parameter">filenode</replaceable></option></term> + <listitem><para>show info for table with filenode <replaceable>filenode</replaceable>.</para></listitem> </varlistentry> <varlistentry> <term><option>-i</option></term> - <listitem><para>include indexes and sequences in the listing</para></listitem> + <term><option>--indexes</option></term> + <listitem><para>include indexes and sequences in the listing.</para></listitem> </varlistentry> <varlistentry> - <term><option>-o</option> <replaceable>oid</replaceable></term> - <listitem><para>show info for table with OID <replaceable>oid</replaceable></para></listitem> + <term><option>-o <replaceable class="parameter">oid</replaceable></option></term> + <term><option>--oid=<replaceable class="parameter">oid</replaceable></option></term> + <listitem><para>show info for table with OID <replaceable>oid</replaceable>.</para></listitem> </varlistentry> <varlistentry> <term><option>-q</option></term> - <listitem><para>omit headers (useful for scripting)</para></listitem> + <term><option>--quiet</option></term> + <listitem><para>omit headers (useful for scripting).</para></listitem> </varlistentry> <varlistentry> <term><option>-s</option></term> - <listitem><para>show tablespace OIDs</para></listitem> + <term><option>--tablespaces</option></term> + <listitem><para>show tablespace OIDs.</para></listitem> </varlistentry> <varlistentry> <term><option>-S</option></term> + <term><option>--system-objects</option></term> <listitem><para>include system objects (those in <option>information_schema</option>, <option>pg_toast</option> - and <option>pg_catalog</option> schemas) + and <option>pg_catalog</option> schemas). </para></listitem> </varlistentry> <varlistentry> - <term><option>-t</option> <replaceable>tablename_pattern</replaceable></term> - <listitem><para>show info for table(s) matching <replaceable>tablename_pattern</replaceable></para></listitem> + <term><option>-t <replaceable class="parameter">tablename_pattern</replaceable></option></term> + <term><option>--table=<replaceable class="parameter">tablename_pattern</replaceable></option></term> + <listitem><para>show info for table(s) matching <replaceable class="parameter">tablename_pattern</replaceable>.</para></listitem> </varlistentry> <varlistentry> @@ -109,8 +116,9 @@ <varlistentry> <term><option>-x</option></term> + <term><option>--extended</option></term> <listitem><para>display more information about each object shown: tablespace name, - schema name, and OID + schema name, and OID. </para></listitem> </varlistentry> @@ -133,29 +141,34 @@ <variablelist> <varlistentry> - <term><option>-d</option> <replaceable>database</replaceable></term> - <listitem><para>database to connect to</para></listitem> + <term><option>-d <replaceable class="parameter">database</replaceable></option></term> + <term><option>--dbname=<replaceable class="parameter">database</replaceable></option></term> + <listitem><para>database to connect to.</para></listitem> </varlistentry> <varlistentry> - <term><option>-H</option> <replaceable>host</replaceable></term> - <listitem><para>database server's host</para></listitem> + <term><option>-h <replaceable class="parameter">host</replaceable></option></term> + <term><option>--host=<replaceable class="parameter">host</replaceable></option></term> + <listitem><para>database server's host.</para></listitem> </varlistentry> <varlistentry> - <term><option>-p</option> <replaceable>port</replaceable></term> - <listitem><para>database server's port</para></listitem> + <term><option>-H <replaceable class="parameter">host</replaceable></option></term> + <listitem><para>database server's host. Use of this parameter is + <emphasis>deprecated</emphasis> as of + <productname>PostgreSQL</productname> 12.</para></listitem> </varlistentry> <varlistentry> - <term><option>-U</option> <replaceable>username</replaceable></term> - <listitem><para>user name to connect as</para></listitem> + <term><option>-p <replaceable class="parameter">port</replaceable></option></term> + <term><option>--port=<replaceable class="parameter">port</replaceable></option></term> + <listitem><para>database server's port.</para></listitem> </varlistentry> <varlistentry> - <term><option>-P</option> <replaceable>password</replaceable></term> - <listitem><para>password (deprecated — putting this on the command line - is a security hazard)</para></listitem> + <term><option>-U <replaceable class="parameter">username</replaceable></option></term> + <term><option>--username=<replaceable class="parameter">username</replaceable></option></term> + <listitem><para>user name to connect as.</para></listitem> </varlistentry> </variablelist> @@ -188,6 +201,30 @@ </para> </refsect1> + <refsect1> + <title>Environment</title> + + <variablelist> + <varlistentry> + <term><envar>PGHOST</envar></term> + <term><envar>PGPORT</envar></term> + <term><envar>PGUSER</envar></term> + + <listitem> + <para> + Default connection parameters. + </para> + </listitem> + </varlistentry> + </variablelist> + + <para> + This utility, like most other <productname>PostgreSQL</productname> utilities, + also uses the environment variables supported by <application>libpq</application> + (see <xref linkend="libpq-envars"/>). + </para> + </refsect1> + <refsect1> <title>Notes</title> -- 2.18.0
From f7e5840ec52c44cad3f9b9dd0ceccfe14135c628 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 27 Aug 2018 19:01:13 +0900 Subject: [PATCH 2/2] Rework option set of vacuumlo Like oid2name, vacuumlo has been lacking consistency with other utilities for its options: - Connection options gain long aliases. - Document environment variables which could be used: PGHOST, PGPORT and PGUSER. Documentation and code is reordered to be more consistent. A basic set of TAP tests has been added while on it. Author: Tatsuro Yamada Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/c7e7f25c-1747-cd0f-9335-390bc97b2...@lab.ntt.co.jp --- contrib/vacuumlo/.gitignore | 2 + contrib/vacuumlo/Makefile | 6 +++ contrib/vacuumlo/t/001_basic.pl | 9 ++++ contrib/vacuumlo/vacuumlo.c | 82 ++++++++++++++++++--------------- doc/src/sgml/vacuumlo.sgml | 39 ++++++++++++++-- 5 files changed, 98 insertions(+), 40 deletions(-) create mode 100644 contrib/vacuumlo/t/001_basic.pl diff --git a/contrib/vacuumlo/.gitignore b/contrib/vacuumlo/.gitignore index 07f6ab4fd7..f3f0ce3d80 100644 --- a/contrib/vacuumlo/.gitignore +++ b/contrib/vacuumlo/.gitignore @@ -1 +1,3 @@ /vacuumlo + +/tmp_check/ diff --git a/contrib/vacuumlo/Makefile b/contrib/vacuumlo/Makefile index 71106ff69c..06c5f43f1b 100644 --- a/contrib/vacuumlo/Makefile +++ b/contrib/vacuumlo/Makefile @@ -19,3 +19,9 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/contrib/vacuumlo/t/001_basic.pl b/contrib/vacuumlo/t/001_basic.pl new file mode 100644 index 0000000000..2bfb6ce17d --- /dev/null +++ b/contrib/vacuumlo/t/001_basic.pl @@ -0,0 +1,9 @@ +use strict; +use warnings; + +use TestLib; +use Test::More tests => 8; + +program_help_ok('vacuumlo'); +program_version_ok('vacuumlo'); +program_options_handling_ok('vacuumlo'); diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c index 7eb474ca3e..26040115a9 100644 --- a/contrib/vacuumlo/vacuumlo.c +++ b/contrib/vacuumlo/vacuumlo.c @@ -26,6 +26,7 @@ #include "fe_utils/connect.h" #include "libpq-fe.h" #include "pg_getopt.h" +#include "getopt_long.h" #define BUFSIZE 1024 @@ -434,17 +435,17 @@ usage(const char *progname) printf("%s removes unreferenced large objects from databases.\n\n", progname); printf("Usage:\n %s [OPTION]... DBNAME...\n\n", progname); printf("Options:\n"); - printf(" -l LIMIT commit after removing each LIMIT large objects\n"); - printf(" -n don't remove large objects, just show what would be done\n"); - printf(" -v write a lot of progress messages\n"); - printf(" -V, --version output version information, then exit\n"); - printf(" -?, --help show this help, then exit\n"); + printf(" -l, --limit=LIMIT commit after removing each LIMIT large objects\n"); + printf(" -n, --dry-run don't remove large objects, just show what would be done\n"); + printf(" -v, --verbose write a lot of progress messages\n"); + printf(" -V, --version output version information, then exit\n"); + printf(" -?, --help show this help, then exit\n"); printf("\nConnection options:\n"); - printf(" -h HOSTNAME database server host or socket directory\n"); - printf(" -p PORT database server port\n"); - printf(" -U USERNAME user name to connect as\n"); - printf(" -w never prompt for password\n"); - printf(" -W force password prompt\n"); + printf(" -h, --host=HOSTNAME database server host or socket directory\n"); + printf(" -p, --port=PORT database server port\n"); + printf(" -U, --username=USERNAME user name to connect as\n"); + printf(" -w, --no-password never prompt for password\n"); + printf(" -W, --password force password prompt\n"); printf("\n"); printf("Report bugs to <pgsql-b...@postgresql.org>.\n"); } @@ -453,11 +454,26 @@ usage(const char *progname) int main(int argc, char **argv) { + static struct option long_options[] = { + {"host", required_argument, NULL, 'h'}, + {"limit", required_argument, NULL, 'l'}, + {"port", required_argument, NULL, 'p'}, + {"dry-run", no_argument, NULL, 'n'}, + {"username", required_argument, NULL, 'U'}, + {"verbose", no_argument, NULL, 'v'}, + {"version", no_argument, NULL, 'V'}, + {"no-password", no_argument, NULL, 'w'}, + {"password", no_argument, NULL, 'W'}, + {"help", no_argument, NULL, '?'}, + {NULL, 0, NULL, 0} + }; + int rc = 0; struct _param param; int c; int port; const char *progname; + int optindex; progname = get_progname(argv[0]); @@ -486,25 +502,12 @@ main(int argc, char **argv) } } - while (1) + while((c = getopt_long(argc, argv, "h:l:np:U:vwW", long_options, &optindex)) != -1) { - c = getopt(argc, argv, "h:l:U:p:vnwW"); - if (c == -1) - break; - switch (c) { - case '?': - fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); - exit(1); - case ':': - exit(1); - case 'v': - param.verbose = 1; - break; - case 'n': - param.dry_run = 1; - param.verbose = 1; + case 'h': + param.pg_host = pg_strdup(optarg); break; case 'l': param.transaction_limit = strtol(optarg, NULL, 10); @@ -516,14 +519,9 @@ main(int argc, char **argv) exit(1); } break; - case 'U': - param.pg_user = pg_strdup(optarg); - break; - case 'w': - param.pg_prompt = TRI_NO; - break; - case 'W': - param.pg_prompt = TRI_YES; + case 'n': + param.dry_run = 1; + param.verbose = 1; break; case 'p': port = strtol(optarg, NULL, 10); @@ -534,9 +532,21 @@ main(int argc, char **argv) } param.pg_port = pg_strdup(optarg); break; - case 'h': - param.pg_host = pg_strdup(optarg); + case 'U': + param.pg_user = pg_strdup(optarg); break; + case 'v': + param.verbose = 1; + break; + case 'w': + param.pg_prompt = TRI_NO; + break; + case 'W': + param.pg_prompt = TRI_YES; + break; + default: + fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); + exit(1); } } diff --git a/doc/src/sgml/vacuumlo.sgml b/doc/src/sgml/vacuumlo.sgml index 0b4dfc2b17..0b57a77af4 100644 --- a/doc/src/sgml/vacuumlo.sgml +++ b/doc/src/sgml/vacuumlo.sgml @@ -55,7 +55,8 @@ <variablelist> <varlistentry> - <term><option>-l</option> <replaceable>limit</replaceable></term> + <term><option>-l <replaceable class="parameter">limit</replaceable></option></term> + <term><option>--limit=<replaceable class="parameter">limit</replaceable></option></term> <listitem> <para> Remove no more than <replaceable>limit</replaceable> large objects per @@ -69,6 +70,7 @@ <varlistentry> <term><option>-n</option></term> + <term><option>--dry-run</option></term> <listitem> <para>Don't remove anything, just show what would be done.</para> </listitem> @@ -76,6 +78,7 @@ <varlistentry> <term><option>-v</option></term> + <term><option>--verbose</option></term> <listitem> <para>Write a lot of progress messages.</para> </listitem> @@ -110,21 +113,24 @@ <variablelist> <varlistentry> - <term><option>-h</option> <replaceable>hostname</replaceable></term> + <term><option>-h <replaceable class="parameter">host</replaceable></option></term> + <term><option>--host=<replaceable class="parameter">host</replaceable></option></term> <listitem> <para>Database server's host.</para> </listitem> </varlistentry> <varlistentry> - <term><option>-p</option> <replaceable>port</replaceable></term> + <term><option>-p <replaceable>port</replaceable></option></term> + <term><option>--port=<replaceable class="parameter">port</replaceable></option></term> <listitem> <para>Database server's port.</para> </listitem> </varlistentry> <varlistentry> - <term><option>-U</option> <replaceable>username</replaceable></term> + <term><option>-U <replaceable>username</replaceable></option></term> + <term><option>--username=<replaceable class="parameter">username</replaceable></option></term> <listitem> <para>User name to connect as.</para> </listitem> @@ -146,6 +152,7 @@ <varlistentry> <term><option>-W</option></term> + <term><option>--password</option></term> <listitem> <para> Force <application>vacuumlo</application> to prompt for a @@ -167,6 +174,30 @@ </para> </refsect1> + <refsect1> + <title>Environment</title> + + <variablelist> + <varlistentry> + <term><envar>PGHOST</envar></term> + <term><envar>PGPORT</envar></term> + <term><envar>PGUSER</envar></term> + + <listitem> + <para> + Default connection parameters. + </para> + </listitem> + </varlistentry> + </variablelist> + + <para> + This utility, like most other <productname>PostgreSQL</productname> utilities, + also uses the environment variables supported by <application>libpq</application> + (see <xref linkend="libpq-envars"/>). + </para> + </refsect1> + <refsect1> <title>Notes</title> -- 2.18.0
signature.asc
Description: PGP signature
