Re: Trailing semicolons in psql patch

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

How about a patch to strip semicolons in psql? I *know*
some of you have done this before:

\d mytable;

(Did not find any relation named "mytable;")

Since there is no reason to have a table named "mytable;"
why not just have psql do the smart thing and silently
strip the trailing semicolons? The attached patch only addresses
a few backslash commands as a proof of concept, but some others
could probably use it without harm. The patch affects:

\C
\c
\d
\e
\i
\o
\s
\z

Greg Sabino Mullane
greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200109271811

P.S. Re: microtiming: EXPLAIN ANALYZE sounds interesting, and as
soon as cvs is fixed so that anoncvs/postgresql works, I'll take
a look at it. :)

-----BEGIN PGP SIGNATURE-----
Comment: http://www.turnstep.com/pgp.html

iQA/AwUBO7OkzrybkGcUlkrIEQIPrgCgurQBAqZLt6m5JvbKOJyVIOx3ZQAAn0Wg
KD097oaniENYAeikbSM7LdZg
=erHC
-----END PGP SIGNATURE-----

I like it. Several people have complained because they forget and add
the semicolon. I like that you made it per-command so commands that
need to see the semicolon see it. I will change it slightly to use
true/false rather than 0/1.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> How about a patch to strip semicolons in psql? I *know*
> some of you have done this before:
>
> \d mytable;
>
> (Did not find any relation named "mytable;")
>
> Since there is no reason to have a table named "mytable;"
> why not just have psql do the smart thing and silently
> strip the trailing semicolons? The attached patch only addresses
> a few backslash commands as a proof of concept, but some others
> could probably use it without harm. The patch affects:
>
> \C
> \c
> \d
> \e
> \i
> \o
> \s
> \z
>
> Greg Sabino Mullane
> greg(at)turnstep(dot)com
> PGP Key: 0x14964AC8 200109271811
>
> P.S. Re: microtiming: EXPLAIN ANALYZE sounds interesting, and as
> soon as cvs is fixed so that anoncvs/postgresql works, I'll take
> a look at it. :)
>
>
> -----BEGIN PGP SIGNATURE-----
> Comment: http://www.turnstep.com/pgp.html
>
> iQA/AwUBO7OkzrybkGcUlkrIEQIPrgCgurQBAqZLt6m5JvbKOJyVIOx3ZQAAn0Wg
> KD097oaniENYAeikbSM7LdZg
> =erHC
> -----END PGP SIGNATURE-----

> *** ./src/bin/psql/command.c.orig Tue Sep 25 13:21:50 2001
> --- ./src/bin/psql/command.c Tue Sep 25 18:06:37 2001
> ***************
> *** 55,61 ****
> {
> OT_NORMAL, OT_SQLID, OT_FILEPIPE
> };
> ! static char *scan_option(char **string, enum option_type type, char *quote);
> static char *unescape(const unsigned char *source, size_t len);
>
> static bool do_edit(const char *filename_arg, PQExpBuffer query_buf);
> --- 55,61 ----
> {
> OT_NORMAL, OT_SQLID, OT_FILEPIPE
> };
> ! static char *scan_option(char **string, enum option_type type, char *quote, bool semicolon);
> static char *unescape(const unsigned char *source, size_t len);
>
> static bool do_edit(const char *filename_arg, PQExpBuffer query_buf);
> ***************
> *** 216,222 ****
> /* \C -- override table title (formerly change HTML caption) */
> else if (strcmp(cmd, "C") == 0)
> {
> ! char *opt = scan_option(&string, OT_NORMAL, NULL);
>
> success = do_pset("title", opt, &pset.popt, quiet);
> free(opt);
> --- 216,222 ----
> /* \C -- override table title (formerly change HTML caption) */
> else if (strcmp(cmd, "C") == 0)
> {
> ! char *opt = scan_option(&string, OT_NORMAL, NULL, 1);
>
> success = do_pset("title", opt, &pset.popt, quiet);
> free(opt);
> ***************
> *** 238,245 ****
> char opt1q,
> opt2q;
>
> ! opt1 = scan_option(&string, OT_NORMAL, &opt1q);
> ! opt2 = scan_option(&string, OT_NORMAL, &opt2q);
>
> if (opt2)
> /* gave username */
> --- 238,245 ----
> char opt1q,
> opt2q;
>
> ! opt1 = scan_option(&string, OT_NORMAL, &opt1q, 1);
> ! opt2 = scan_option(&string, OT_NORMAL, &opt2q, 1);
>
> if (opt2)
> /* gave username */
> ***************
> *** 273,280 ****
> {
> char *name;
> bool show_verbose;
>
> - name = scan_option(&string, OT_SQLID, NULL);
> show_verbose = strchr(cmd, '+') ? true : false;
>
> switch (cmd[1])
> --- 273,280 ----
> {
> char *name;
> bool show_verbose;
> + name = scan_option(&string, OT_SQLID, NULL, 1);
>
> show_verbose = strchr(cmd, '+') ? true : false;
>
> switch (cmd[1])
> ***************
> *** 337,343 ****
> }
> else
> {
> ! fname = scan_option(&string, OT_NORMAL, NULL);
> status = do_edit(fname, query_buf) ? CMD_NEWEDIT : CMD_ERROR;
> free(fname);
> }
> --- 337,343 ----
> }
> else
> {
> ! fname = scan_option(&string, OT_NORMAL, NULL, 1);
> status = do_edit(fname, query_buf) ? CMD_NEWEDIT : CMD_ERROR;
> free(fname);
> }
> ***************
> *** 356,363 ****
> fout = pset.queryFout;
> else
> fout = stdout;
> !
> ! while ((value = scan_option(&string, OT_NORMAL, &quoted)))
> {
> if (!quoted && strcmp(value, "-n") == 0)
> no_newline = true;
> --- 356,363 ----
> fout = pset.queryFout;
> else
> fout = stdout;
> !
> ! while ((value = scan_option(&string, OT_NORMAL, &quoted, 0)))
> {
> if (!quoted && strcmp(value, "-n") == 0)
> no_newline = true;
> ***************
> *** 378,384 ****
> /* \encoding -- set/show client side encoding */
> else if (strcmp(cmd, "encoding") == 0)
> {
> ! char *encoding = scan_option(&string, OT_NORMAL, NULL);
>
> if (!encoding)
> /* show encoding */
> --- 378,384 ----
> /* \encoding -- set/show client side encoding */
> else if (strcmp(cmd, "encoding") == 0)
> {
> ! char *encoding = scan_option(&string, OT_NORMAL, NULL, 0);
>
> if (!encoding)
> /* show encoding */
> ***************
> *** 406,412 ****
> /* \f -- change field separator */
> else if (strcmp(cmd, "f") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL);
>
> success = do_pset("fieldsep", fname, &pset.popt, quiet);
> free(fname);
> --- 406,412 ----
> /* \f -- change field separator */
> else if (strcmp(cmd, "f") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL, 0);
>
> success = do_pset("fieldsep", fname, &pset.popt, quiet);
> free(fname);
> ***************
> *** 415,421 ****
> /* \g means send query */
> else if (strcmp(cmd, "g") == 0)
> {
> ! char *fname = scan_option(&string, OT_FILEPIPE, NULL);
>
> if (!fname)
> pset.gfname = NULL;
> --- 415,421 ----
> /* \g means send query */
> else if (strcmp(cmd, "g") == 0)
> {
> ! char *fname = scan_option(&string, OT_FILEPIPE, NULL, 0);
>
> if (!fname)
> pset.gfname = NULL;
> ***************
> *** 447,453 ****
> /* \i is include file */
> else if (strcmp(cmd, "i") == 0 || strcmp(cmd, "include") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL);
>
> if (!fname)
> {
> --- 447,453 ----
> /* \i is include file */
> else if (strcmp(cmd, "i") == 0 || strcmp(cmd, "include") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL, 1);
>
> if (!fname)
> {
> ***************
> *** 475,482 ****
> char *opt1,
> *opt2;
>
> ! opt1 = scan_option(&string, OT_NORMAL, NULL);
> ! opt2 = scan_option(&string, OT_NORMAL, NULL);
>
> if (strcmp(cmd + 3, "export") == 0)
> {
> --- 475,482 ----
> char *opt1,
> *opt2;
>
> ! opt1 = scan_option(&string, OT_NORMAL, NULL, 1);
> ! opt2 = scan_option(&string, OT_NORMAL, NULL, 1);
>
> if (strcmp(cmd + 3, "export") == 0)
> {
> ***************
> *** 528,534 ****
> /* \o -- set query output */
> else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
> {
> ! char *fname = scan_option(&string, OT_FILEPIPE, NULL);
>
> success = setQFout(fname);
> free(fname);
> --- 528,534 ----
> /* \o -- set query output */
> else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
> {
> ! char *fname = scan_option(&string, OT_FILEPIPE, NULL, 1);
>
> success = setQFout(fname);
> free(fname);
> ***************
> *** 547,554 ****
> /* \pset -- set printing parameters */
> else if (strcmp(cmd, "pset") == 0)
> {
> ! char *opt0 = scan_option(&string, OT_NORMAL, NULL);
> ! char *opt1 = scan_option(&string, OT_NORMAL, NULL);
>
> if (!opt0)
> {
> --- 547,554 ----
> /* \pset -- set printing parameters */
> else if (strcmp(cmd, "pset") == 0)
> {
> ! char *opt0 = scan_option(&string, OT_NORMAL, NULL, 0);
> ! char *opt1 = scan_option(&string, OT_NORMAL, NULL, 0);
>
> if (!opt0)
> {
> ***************
> *** 577,583 ****
> /* \s save history in a file or show it on the screen */
> else if (strcmp(cmd, "s") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL);
>
> success = saveHistory(fname ? fname : "/dev/tty");
>
> --- 577,583 ----
> /* \s save history in a file or show it on the screen */
> else if (strcmp(cmd, "s") == 0)
> {
> ! char *fname = scan_option(&string, OT_NORMAL, NULL, 1);
>
> success = saveHistory(fname ? fname : "/dev/tty");
>
> ***************
> *** 589,595 ****
> /* \set -- generalized set variable/option command */
> else if (strcmp(cmd, "set") == 0)
> {
> ! char *opt0 = scan_option(&string, OT_NORMAL, NULL);
>
> if (!opt0)
> {
> --- 589,595 ----
> /* \set -- generalized set variable/option command */
> else if (strcmp(cmd, "set") == 0)
> {
> ! char *opt0 = scan_option(&string, OT_NORMAL, NULL, 0);
>
> if (!opt0)
> {
> ***************
> *** 614,624 ****
> char *newval = NULL;
> char *opt;
>
> ! opt = scan_option(&string, OT_NORMAL, NULL);
> newval = xstrdup(opt ? opt : "");
> free(opt);
>
> ! while ((opt = scan_option(&string, OT_NORMAL, NULL)))
> {
> newval = realloc(newval, strlen(newval) + strlen(opt) + 1);
> if (!newval)
> --- 614,624 ----
> char *newval = NULL;
> char *opt;
>
> ! opt = scan_option(&string, OT_NORMAL, NULL, 0);
> newval = xstrdup(opt ? opt : "");
> free(opt);
>
> ! while ((opt = scan_option(&string, OT_NORMAL, NULL, 0)))
> {
> newval = realloc(newval, strlen(newval) + strlen(opt) + 1);
> if (!newval)
> ***************
> *** 648,654 ****
> /* \T -- define html <table ...> attributes */
> else if (strcmp(cmd, "T") == 0)
> {
> ! char *value = scan_option(&string, OT_NORMAL, NULL);
>
> success = do_pset("tableattr", value, &pset.popt, quiet);
> free(value);
> --- 648,654 ----
> /* \T -- define html <table ...> attributes */
> else if (strcmp(cmd, "T") == 0)
> {
> ! char *value = scan_option(&string, OT_NORMAL, NULL, 0);
>
> success = do_pset("tableattr", value, &pset.popt, quiet);
> free(value);
> ***************
> *** 657,663 ****
> /* \unset */
> else if (strcmp(cmd, "unset") == 0)
> {
> ! char *opt = scan_option(&string, OT_NORMAL, NULL);
>
> if (!opt)
> {
> --- 657,663 ----
> /* \unset */
> else if (strcmp(cmd, "unset") == 0)
> {
> ! char *opt = scan_option(&string, OT_NORMAL, NULL, 0);
>
> if (!opt)
> {
> ***************
> *** 686,692 ****
> }
> else
> {
> ! fname = scan_option(&string, OT_FILEPIPE, NULL);
>
> if (!fname)
> {
> --- 686,692 ----
> }
> else
> {
> ! fname = scan_option(&string, OT_FILEPIPE, NULL, 1);
>
> if (!fname)
> {
> ***************
> *** 741,747 ****
> /* \z -- list table rights (grant/revoke) */
> else if (strcmp(cmd, "z") == 0)
> {
> ! char *opt = scan_option(&string, OT_SQLID, NULL);
>
> success = permissionsList(opt);
> free(opt);
> --- 741,747 ----
> /* \z -- list table rights (grant/revoke) */
> else if (strcmp(cmd, "z") == 0)
> {
> ! char *opt = scan_option(&string, OT_SQLID, NULL, 1);
>
> success = permissionsList(opt);
> free(opt);
> ***************
> *** 772,778 ****
> char *value;
>
> fprintf(stderr, "+ optstr = |%s|\n", options_string);
> ! while ((value = scan_option(&string, OT_NORMAL, NULL)))
> {
> fprintf(stderr, "+ opt(%d) = |%s|\n", i++, value);
> free(value);
> --- 772,778 ----
> char *value;
>
> fprintf(stderr, "+ optstr = |%s|\n", options_string);
> ! while ((value = scan_option(&string, OT_NORMAL, NULL, 1)))
> {
> fprintf(stderr, "+ opt(%d) = |%s|\n", i++, value);
> free(value);
> ***************
> *** 787,793 ****
> status = CMD_ERROR;
>
> /* eat the rest of the options string */
> ! while ((val = scan_option(&string, OT_NORMAL, NULL)))
> {
> if (status != CMD_UNKNOWN)
> psql_error("\\%s: extra argument '%s' ignored\n", cmd, val);
> --- 787,793 ----
> status = CMD_ERROR;
>
> /* eat the rest of the options string */
> ! while ((val = scan_option(&string, OT_NORMAL, NULL, 0)))
> {
> if (status != CMD_UNKNOWN)
> psql_error("\\%s: extra argument '%s' ignored\n", cmd, val);
> ***************
> *** 806,812 ****
> * scan_option()
> */
> static char *
> ! scan_option(char **string, enum option_type type, char *quote)
> {
> unsigned int pos = 0;
> char *options_string;
> --- 806,812 ----
> * scan_option()
> */
> static char *
> ! scan_option(char **string, enum option_type type, char *quote, bool semicolon)
> {
> unsigned int pos = 0;
> char *options_string;
> ***************
> *** 822,827 ****
> --- 822,835 ----
> /* skip leading whitespace */
> pos += strspn(options_string + pos, " \t\n\r");
>
> +
> + /* Strip any trailing semi-colons for some types */
> + if (semicolon) {
> + int i;
> + for (i = strlen(options_string)-1; i && options_string[i]==';'; i--);
> + if (i<strlen(options_string)-1) options_string[i+1]='\0';
> + }
> +
> switch (options_string[pos])
> {
>
> ***************
> *** 863,868 ****
> --- 871,877 ----
> * If this is expected to be an SQL identifier like option
> * then we strip out the double quotes
> */
> +
> if (type == OT_SQLID)
> {
> unsigned int k,
> ***************
> *** 894,900 ****
> *string = options_string + jj + 1;
> if (quote)
> *quote = '"';
> !
> return return_val;
> }
>
> --- 903,909 ----
> *string = options_string + jj + 1;
> if (quote)
> *quote = '"';
> !
> return return_val;
> }
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

greg(at)turnstep(dot)com writes:

> Since there is no reason to have a table named "mytable;"
> why not just have psql do the smart thing and silently
> strip the trailing semicolons?

"Stripping" semicolons is one thing, accepting semicolons as command
separators is another. Your patch seems to handle the case of

\d mytable;

but it doesn't handle any of

\d mytable; \d yourtable
\d mytable;<space>
\d mytable; select * from mytable;

Anything short of that plus...

> The attached patch only addresses
> a few backslash commands as a proof of concept,

...is just going to create some other confusion in place of the current
one.

--
Peter Eisentraut peter_e(at)gmx(dot)net http://funkturm.homeip.net/~peter

> greg(at)turnstep(dot)com writes:
>
> > Since there is no reason to have a table named "mytable;"
> > why not just have psql do the smart thing and silently
> > strip the trailing semicolons?
>
> "Stripping" semicolons is one thing, accepting semicolons as command
> separators is another. Your patch seems to handle the case of
>
> \d mytable;
>
> but it doesn't handle any of
>
> \d mytable; \d yourtable
> \d mytable;<space>
> \d mytable; select * from mytable;
>
> Anything short of that plus...
>
> > The attached patch only addresses
> > a few backslash commands as a proof of concept,
>
> ...is just going to create some other confusion in place of the current
> one.

My assumption is that the semicolons used with backslash commands are
just knee-jerk adding of semicolons to the end, not attempts to actually
string backslash commands togeter. His patch does catch those cases.
The basic issue is that I think we can conclude that any trailing
semicolon on a line with a backslash command can be dropped. In fact, I
wouldn't want to catch any of the items you listed above. Let them fail.

We all do this by accident, don't we? I know I do.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The basic issue is that I think we can conclude that any trailing
> semicolon on a line with a backslash command can be dropped.

I agree with this premise ... but stated that way, the patch does
entirely the wrong thing, because it's stripping trailing semis at
the word level not the line level. I think it's a lot more surprising
to drop the semi in

\x foo; bar

than when it's actually at the end of the line.

How about stripping trailing semis (perhaps whitespace too?) before
the line is broken into words?

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> I agree with this premise ... but stated that way, the patch does
> entirely the wrong thing, because it's stripping trailing semis at
> the word level not the line level. I think it's a lot more surprising
> to drop the semi in
>
> \x foo; bar
>
> than when it's actually at the end of the line.

Ah...so what we want then is for all of these cases to work:

\d foo \d bar
\d foo; \d bar
\d foo; \d bar;
\d foo; \d bar

Makes sense...I never combine backslash commands on the same line,
but it is possible. In that case, how about just moving this section:

if (semicolon) {
int i;
for (i = strlen(options_string)-1; i && options_string[i]==';'; i--);
if (i<strlen(options_string)-1) options_string[i+1]='\0';
}

to right after here, where we find a "normal word"?

strncpy(return_val, &options_string[pos], token_end);
return_val[token_end] = 0;

(and changing options_string to return_val)

that should allow us to catch all the cases above, if I am
understanding the objection above correctly.

Greg Sabino Mullane
greg(at)turnstep(dot)com PGP Key: 0x14964AC8 200110011738

-----BEGIN PGP SIGNATURE-----
Comment: http://www.turnstep.com/pgp.html

iQA/AwUBO7jijLybkGcUlkrIEQIMWwCfYDNNqC/UYKKKaqj5MYrEcg2bGhcAoPUP
cqeF4yXAuPedUpQPLVFsw1lO
=4dGu
-----END PGP SIGNATURE-----

OK, any chance of getting an updated patch. Your patch email is at:

http://candle.pha.pa.us/cgi-bin/pgpatches

> > I agree with this premise ... but stated that way, the patch does
> > entirely the wrong thing, because it's stripping trailing semis at
> > the word level not the line level. I think it's a lot more surprising
> > to drop the semi in
> >
> > \x foo; bar
> >
> > than when it's actually at the end of the line.
>
> Ah...so what we want then is for all of these cases to work:
>
> \d foo \d bar
> \d foo; \d bar
> \d foo; \d bar;
> \d foo; \d bar
>
> Makes sense...I never combine backslash commands on the same line,
> but it is possible. In that case, how about just moving this section:
>
> if (semicolon) {
> int i;
> for (i = strlen(options_string)-1; i && options_string[i]==';'; i--);
> if (i<strlen(options_string)-1) options_string[i+1]='\0';
> }
>
> to right after here, where we find a "normal word"?
>
> strncpy(return_val, &options_string[pos], token_end);
> return_val[token_end] = 0;
>
> (and changing options_string to return_val)
>
> that should allow us to catch all the cases above, if I am
> understanding the objection above correctly.
>
> Greg Sabino Mullane
> greg(at)turnstep(dot)com PGP Key: 0x14964AC8 200110011738
-- End of PGP signed section.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026