Re: [HACKERS] psql \copy warning

Patch applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:
>
> I have developed an updated patch that:
>
> o turns off escape_string_warning in pg_dumpall.c
> o optionally use E'' for \password (undocumented option?)
> o honor standard_conforming-strings for \copy (but not
> support literal E'' strings)
> o optionally use E'' for \d commands
> o turn off escape_string_warning for createdb, createuser,
> droplang
>
> I agree someday we might want to turn off escape_string_warning, but I
> think we should leave it on as long as possible as it is still pointing
> out escape problem areas in the code.
>
> ---------------------------------------------------------------------------
>
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > > Right. I think the question is whether we want all psql strings to
> > > > accept backslashes, and hence not support E'' at all for psql commands.
> > > > I figured that made the most sense.
> > >
> > > I'm not convinced. Wouldn't it be better if psql commands track the
> > > backend syntax? With standard_conforming_strings on, there will be two
> > > ways to tell COPY you want a tab as a delimiter:
> > > DELIMITER '<actual tab char>'
> > > DELIMITER E'\t'
> > > and in particular this will NOT do that:
> > > DELIMITER '\t'
> >
> > Well, I think it a little more confusing that just \copy. What about \d
> > and \set uses of backslashes. Do they honor standard_conforming_strings
> > too? I assume you are saying they should.
> >
> > > If we keep '\t' as meaning tab in the \copy syntax then I think we're
> > > going to cause confusion in the long run. I think we should fix \copy
> > > and related psql backslash commands to accept E'\t', and make sure that
> > > the behavior is the same as the connected backend depending on what its
> > > standard_conforming_strings setting is.
> >
> > OK, though this is going to mean that examples in the psql manual page
> > are going to be different for different standard_conforming_strings
> > settings:
> >
> > testdb=> \set content '\'' `cat my_file.txt` '\''
> > testdb=> INSERT INTO my_table VALUES (:content);
> >
> > psql doesn't know '''' is about doubling single quotes in a string,
> > though \copy does. The major problem, I think, is that psql often
> > follows the shell rules, rather than the SQL rules for most things.
> >
> > > There is a secondary, largely cosmetic question of whether psql should
> > > attempt to prevent you from seeing escape_string_warning messages.
> > > I personally have come to the conclusion that escape_string_warning is
> > > probably not going to be on by default anyway ;-), and hence it's not
> > > worth going to great extremes to prevent this, particularly if it breaks
> > > the ability to use psql against pre-8.1 servers.
> >
> > It does break backward compatibility.
> >
> > --
> > Bruce Momjian http://candle.pha.pa.us
> > EnterpriseDB http://www.enterprisedb.com
> >
> > + If your life is a hard drive, Christ can be your backup. +
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 4: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
>
> --
> Bruce Momjian http://candle.pha.pa.us
> EnterpriseDB http://www.enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +

> Index: src/bin/pg_dump/pg_dumpall.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v
> retrieving revision 1.77
> diff -c -c -r1.77 pg_dumpall.c
> *** src/bin/pg_dump/pg_dumpall.c 28 May 2006 21:13:54 -0000 1.77
> --- src/bin/pg_dump/pg_dumpall.c 29 May 2006 20:41:01 -0000
> ***************
> *** 338,343 ****
> --- 338,345 ----
> printf("SET client_encoding = '%s';\n",
> pg_encoding_to_char(encoding));
> printf("SET standard_conforming_strings = %s;\n", std_strings);
> + if (strcmp(std_strings, "off") == 0)
> + printf("SET escape_string_warning = 'off';\n");
> printf("\n");
>
> /* Dump roles (users) */
> Index: src/bin/psql/command.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
> retrieving revision 1.166
> diff -c -c -r1.166 command.c
> *** src/bin/psql/command.c 2 Apr 2006 20:08:22 -0000 1.166
> --- src/bin/psql/command.c 29 May 2006 20:41:02 -0000
> ***************
> *** 681,688 ****
> PGresult *res;
>
> initPQExpBuffer(&buf);
> ! printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD '%s';",
> ! fmtId(user), encrypted_password);
> res = PSQLexec(buf.data, false);
> termPQExpBuffer(&buf);
> if (!res)
> --- 681,689 ----
> PGresult *res;
>
> initPQExpBuffer(&buf);
> ! printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD %c'%s';",
> ! fmtId(user), NEED_E_STR(encrypted_password),
> ! encrypted_password);
> res = PSQLexec(buf.data, false);
> termPQExpBuffer(&buf);
> if (!res)
> Index: src/bin/psql/common.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/psql/common.h,v
> retrieving revision 1.47
> diff -c -c -r1.47 common.h
> *** src/bin/psql/common.h 6 Mar 2006 19:49:20 -0000 1.47
> --- src/bin/psql/common.h 29 May 2006 20:41:03 -0000
> ***************
> *** 23,28 ****
> --- 23,34 ----
> #define atooid(x) ((Oid) strtoul((x), NULL, 10))
>
> /*
> + * We use this to prefix strings with E'' that we know are already safe,
> + * so we don't get an escape_string_warning.
> + */
> + #define NEED_E_STR(str) ((strchr(str, '\\') && !standard_strings()) ? ESCAPE_STRING_SYNTAX : ' ')
> +
> + /*
> * Safer versions of some standard C library functions. If an
> * out-of-memory condition occurs, these functions will bail out
> * safely; therefore, their return value is guaranteed to be non-NULL.
> Index: src/bin/psql/copy.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/psql/copy.c,v
> retrieving revision 1.61
> diff -c -c -r1.61 copy.c
> *** src/bin/psql/copy.c 26 May 2006 19:51:29 -0000 1.61
> --- src/bin/psql/copy.c 29 May 2006 20:41:03 -0000
> ***************
> *** 216,222 ****
> goto error;
>
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', true, pset.encoding);
> if (!token)
> goto error;
>
> --- 216,222 ----
> goto error;
>
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', true, pset.encoding);
> if (!token)
> goto error;
>
> ***************
> *** 255,261 ****
> if (token && pg_strcasecmp(token, "delimiters") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (!token)
> goto error;
> result->delim = pg_strdup(token);
> --- 255,261 ----
> if (token && pg_strcasecmp(token, "delimiters") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (!token)
> goto error;
> result->delim = pg_strdup(token);
> ***************
> *** 290,299 ****
> else if (pg_strcasecmp(token, "delimiter") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token)
> result->delim = pg_strdup(token);
> else
> --- 290,299 ----
> else if (pg_strcasecmp(token, "delimiter") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token)
> result->delim = pg_strdup(token);
> else
> ***************
> *** 302,311 ****
> else if (pg_strcasecmp(token, "null") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token)
> result->null = pg_strdup(token);
> else
> --- 302,311 ----
> else if (pg_strcasecmp(token, "null") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token)
> result->null = pg_strdup(token);
> else
> ***************
> *** 314,323 ****
> else if (pg_strcasecmp(token, "quote") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token)
> result->quote = pg_strdup(token);
> else
> --- 314,323 ----
> else if (pg_strcasecmp(token, "quote") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token)
> result->quote = pg_strdup(token);
> else
> ***************
> *** 326,335 ****
> else if (pg_strcasecmp(token, "escape") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! '\\', false, pset.encoding);
> if (token)
> result->escape = pg_strdup(token);
> else
> --- 326,335 ----
> else if (pg_strcasecmp(token, "escape") == 0)
> {
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token && pg_strcasecmp(token, "as") == 0)
> token = strtokx(NULL, whitespace, NULL, "'",
> ! standard_strings() ? 0 : '\\', false, pset.encoding);
> if (token)
> result->escape = pg_strdup(token);
> else
> ***************
> *** 462,481 ****
> if (options->delim)
> {
> if (options->delim[0] == '\'')
> ! appendPQExpBuffer(&query, " USING DELIMITERS %s",
> ! options->delim);
> else
> ! appendPQExpBuffer(&query, " USING DELIMITERS '%s'",
> ! options->delim);
> }
>
> /* There is no backward-compatible CSV syntax */
> if (options->null)
> {
> if (options->null[0] == '\'')
> ! appendPQExpBuffer(&query, " WITH NULL AS %s", options->null);
> else
> ! appendPQExpBuffer(&query, " WITH NULL AS '%s'", options->null);
> }
>
> if (options->csv_mode)
> --- 462,483 ----
> if (options->delim)
> {
> if (options->delim[0] == '\'')
> ! appendPQExpBuffer(&query, " USING DELIMITERS %c%s",
> ! NEED_E_STR(options->delim), options->delim);
> else
> ! appendPQExpBuffer(&query, " USING DELIMITERS %c'%s'",
> ! NEED_E_STR(options->delim), options->delim);
> }
>
> /* There is no backward-compatible CSV syntax */
> if (options->null)
> {
> if (options->null[0] == '\'')
> ! appendPQExpBuffer(&query, " WITH NULL AS %c%s",
> ! NEED_E_STR(options->null), options->null);
> else
> ! appendPQExpBuffer(&query, " WITH NULL AS %c'%s'",
> ! NEED_E_STR(options->null), options->null);
> }
>
> if (options->csv_mode)
> ***************
> *** 487,503 ****
> if (options->quote)
> {
> if (options->quote[0] == '\'')
> ! appendPQExpBuffer(&query, " QUOTE AS %s", options->quote);
> else
> ! appendPQExpBuffer(&query, " QUOTE AS '%s'", options->quote);
> }
>
> if (options->escape)
> {
> if (options->escape[0] == '\'')
> ! appendPQExpBuffer(&query, " ESCAPE AS %s", options->escape);
> else
> ! appendPQExpBuffer(&query, " ESCAPE AS '%s'", options->escape);
> }
>
> if (options->force_quote_list)
> --- 489,509 ----
> if (options->quote)
> {
> if (options->quote[0] == '\'')
> ! appendPQExpBuffer(&query, " QUOTE AS %c%s",
> ! NEED_E_STR(options->quote), options->quote);
> else
> ! appendPQExpBuffer(&query, " QUOTE AS %c'%s'",
> ! NEED_E_STR(options->quote), options->quote);
> }
>
> if (options->escape)
> {
> if (options->escape[0] == '\'')
> ! appendPQExpBuffer(&query, " ESCAPE AS %c%s",
> ! NEED_E_STR(options->escape), options->escape);
> else
> ! appendPQExpBuffer(&query, " ESCAPE AS %c'%s'",
> ! NEED_E_STR(options->escape), options->escape);
> }
>
> if (options->force_quote_list)
> Index: src/bin/psql/describe.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/psql/describe.c,v
> retrieving revision 1.137
> diff -c -c -r1.137 describe.c
> *** src/bin/psql/describe.c 28 May 2006 21:13:54 -0000 1.137
> --- src/bin/psql/describe.c 29 May 2006 20:41:04 -0000
> ***************
> *** 1907,1920 ****
> --- 1907,1923 ----
> if (altnamevar)
> {
> appendPQExpBuffer(buf, "(%s ~ ", namevar);
> + appendPQExpBufferChar(buf, NEED_E_STR(namebuf.data));
> appendStringLiteralConn(buf, namebuf.data, pset.db);
> appendPQExpBuffer(buf, "\n OR %s ~ ", altnamevar);
> + appendPQExpBufferChar(buf, NEED_E_STR(namebuf.data));
> appendStringLiteralConn(buf, namebuf.data, pset.db);
> appendPQExpBuffer(buf, ")\n");
> }
> else
> {
> appendPQExpBuffer(buf, "%s ~ ", namevar);
> + appendPQExpBufferChar(buf, NEED_E_STR(namebuf.data));
> appendStringLiteralConn(buf, namebuf.data, pset.db);
> appendPQExpBufferChar(buf, '\n');
> }
> ***************
> *** 1938,1943 ****
> --- 1941,1947 ----
> {
> WHEREAND();
> appendPQExpBuffer(buf, "%s ~ ", schemavar);
> + appendPQExpBufferChar(buf, NEED_E_STR(schemabuf.data));
> appendStringLiteralConn(buf, schemabuf.data, pset.db);
> appendPQExpBufferChar(buf, '\n');
> }
> Index: src/bin/scripts/createdb.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/scripts/createdb.c,v
> retrieving revision 1.19
> diff -c -c -r1.19 createdb.c
> *** src/bin/scripts/createdb.c 29 May 2006 19:52:46 -0000 1.19
> --- src/bin/scripts/createdb.c 29 May 2006 20:41:08 -0000
> ***************
> *** 185,190 ****
> --- 185,192 ----
> {
> conn = connectDatabase(dbname, host, port, username, password, progname);
>
> + executeCommand(conn, "SET escape_string_warning TO 'off'", progname, false);
> +
> printfPQExpBuffer(&sql, "COMMENT ON DATABASE %s IS ", fmtId(dbname));
> appendStringLiteralConn(&sql, comment, conn);
> appendPQExpBuffer(&sql, ";\n");
> Index: src/bin/scripts/createuser.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/scripts/createuser.c,v
> retrieving revision 1.30
> diff -c -c -r1.30 createuser.c
> *** src/bin/scripts/createuser.c 29 May 2006 19:52:46 -0000 1.30
> --- src/bin/scripts/createuser.c 29 May 2006 20:41:08 -0000
> ***************
> *** 243,248 ****
> --- 243,250 ----
> printfPQExpBuffer(&sql, "CREATE ROLE %s", fmtId(newuser));
> if (newpassword)
> {
> + executeCommand(conn, "SET escape_string_warning TO 'off'", progname, false);
> +
> if (encrypted == TRI_YES)
> appendPQExpBuffer(&sql, " ENCRYPTED");
> if (encrypted == TRI_NO)
> Index: src/bin/scripts/droplang.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/scripts/droplang.c,v
> retrieving revision 1.20
> diff -c -c -r1.20 droplang.c
> *** src/bin/scripts/droplang.c 29 May 2006 19:52:46 -0000 1.20
> --- src/bin/scripts/droplang.c 29 May 2006 20:41:09 -0000
> ***************
> *** 176,183 ****
> * Force schema search path to be just pg_catalog, so that we don't have
> * to be paranoid about search paths below.
> */
> ! executeCommand(conn, "SET search_path = pg_catalog;",
> ! progname, echo);
>
> /*
> * Make sure the language is installed and find the OIDs of the handler
> --- 176,182 ----
> * Force schema search path to be just pg_catalog, so that we don't have
> * to be paranoid about search paths below.
> */
> ! executeCommand(conn, "SET search_path = pg_catalog;", progname, echo);
>
> /*
> * Make sure the language is installed and find the OIDs of the handler

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +