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@candle.pha.pa.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. +