Thread: Re: [HACKERS] psql \copy warning
Jeremy Drake wrote: > I use the \copy command from psql to load data into postgres. I was > fiddling with setting up a database on a HEAD build, and I got the > following new warning > > testy=# \copy episodes from 'episodes.data' with delimiter as '\t' > WARNING: nonstandard use of escape in a string literal > LINE 1: COPY episodes FROM STDIN USING DELIMITERS '\t' > ^ > HINT: Use the escape string syntax for escapes, e.g., E'\r\n'. > > > I figured that this is the new standards conforming strings feature, and I > guess I should get used to the new escape syntax. So I tried the hint > > testy=# \copy episodes FROM 'episodes.data' with delimiter as E'\t' > \copy: parse error at "'\t'" > > So is it just me, or is this decidedly non-helpful? I assume someone > missed this place for the new syntax tweaks? Interesting bug report. The basic question is whether \copy should follow the quoting rules of the SQL server, or of psql. Most psql arguments have backslashes enabled, so I am thinking \copy should as well, and not match COPY if standard_compliant_strings is on. The attached patch fixes the warning you received by adding E'' strings to the \copy arguments, and adds it for the other backslash commands like \d. I don't think we want a psql escape control setting, nor do we want to remove backslash controls from psql commands (they are not SQL standard). -- 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/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 28 May 2006 03:22:10 -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 28 May 2006 03:22:10 -0000 *************** *** 22,27 **** --- 22,29 ---- #define atooid(x) ((Oid) strtoul((x), NULL, 10)) + #define NEED_E_STR(str) (strchr(str, '\\') ? ESCAPE_STRING_SYNTAX : ' ') + /* * Safer versions of some standard C library functions. If an * out-of-memory condition occurs, these functions will bail out 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 28 May 2006 03:22:10 -0000 *************** *** 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.136 diff -c -c -r1.136 describe.c *** src/bin/psql/describe.c 28 May 2006 02:27:08 -0000 1.136 --- src/bin/psql/describe.c 28 May 2006 03:22:11 -0000 *************** *** 1902,1915 **** WHEREAND(); if (altnamevar) appendPQExpBuffer(buf, ! "(%s ~ '^%s'\n" ! " OR %s ~ '^%s')\n", ! namevar, namebuf.data, ! altnamevar, namebuf.data); else appendPQExpBuffer(buf, ! "%s ~ '^%s'\n", ! namevar, namebuf.data); } } --- 1902,1917 ---- WHEREAND(); if (altnamevar) appendPQExpBuffer(buf, ! "(%s ~ %c'^%s'\n" ! " OR %s ~ %c'^%s')\n", ! namevar, NEED_E_STR(namebuf.data), ! namebuf.data, altnamevar, ! NEED_E_STR(namebuf.data), namebuf.data); else appendPQExpBuffer(buf, ! "%s ~ %c'^%s'\n", ! namevar, NEED_E_STR(namebuf.data), ! namebuf.data); } } *************** *** 1926,1933 **** if (schemabuf.data[0] && schemavar) { WHEREAND(); ! appendPQExpBuffer(buf, "%s ~ '^%s'\n", ! schemavar, schemabuf.data); } } else --- 1928,1936 ---- if (schemabuf.data[0] && schemavar) { WHEREAND(); ! appendPQExpBuffer(buf, "%s ~ %c'^%s'\n", ! schemavar, NEED_E_STR(schemabuf.data), ! schemabuf.data); } } else
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The attached patch fixes the warning you received by adding E'' strings
> to the \copy arguments, and adds it for the other backslash commands
> like \d.
You missed the point entirely Bruce. The problem is that \copy's
argument parsing won't accept an option specified as E'\t' --- I believe
it is seeing that as two arguments instead of one.
The patch you propose addresses a completely different issue, which is
whether we are going to E-ify all our utilities so they don't trigger
the escape_string_warning patch. I don't think that that is the right
direction to go in. In fact, based on what I was doing this afternoon,
my feeling is that 8.2 will not ship with escape_string_warning turned
on by default. It's a good tool for testing code when you're trying to
move the code over to standard conforming strings, but it's just too
noisy for code that in point of fact is already fixed.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The attached patch fixes the warning you received by adding E'' strings
> to the \copy arguments, and adds it for the other backslash commands
> like \d.
Further comment on this: I don't think we want all these places
individually making this sort of decision. What they should all be
doing is using appendStringLiteralConn to generate the properly-quoted
literal. (I fixed this already in describe.c, but not in those other
places.)
Once we've got that done, we could argue about whether appendStringLiteral
ought to prepend an E to silence escape_string_warning. I'd still vote
no, but at least it would be a single place to change and not N of 'em.
What's more, each place that is generating a variable literal without
using appendStringLiteral or PQescapeString is at least potentially
vulnerable to encoding issues, and so we should probably convert them
anyway.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The attached patch fixes the warning you received by adding E'' strings > > to the \copy arguments, and adds it for the other backslash commands > > like \d. > > You missed the point entirely Bruce. The problem is that \copy's > argument parsing won't accept an option specified as E'\t' --- I believe > it is seeing that as two arguments instead of one. 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. > The patch you propose addresses a completely different issue, which is > whether we are going to E-ify all our utilities so they don't trigger > the escape_string_warning patch. I don't think that that is the right > direction to go in. In fact, based on what I was doing this afternoon, > my feeling is that 8.2 will not ship with escape_string_warning turned > on by default. It's a good tool for testing code when you're trying to > move the code over to standard conforming strings, but it's just too > noisy for code that in point of fact is already fixed. Well, we can remove the change I made to ruleutils.c::get_const_expr() because you are right, we are setting standard_conforming_strings in pg_dump, so we know the value and E'' is not necessary. We might also want to turn off escape_string_warning in pg_dump as well. As far as psql, we could check standard_conforming_strings and just use the proper escaping without using E'', assuming escape_string_warning is off. escape_string_warning was really designed as a portability tool, so if we don't want to use it for our utilities, that is fine, as long as we are checking standard_conforming_strings. If we don't set escape_string_warning on for a release, will we be able to turn on standard_conforming_strings? I just don't know. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The attached patch fixes the warning you received by adding E'' strings > > to the \copy arguments, and adds it for the other backslash commands > > like \d. > > Further comment on this: I don't think we want all these places > individually making this sort of decision. What they should all be > doing is using appendStringLiteralConn to generate the properly-quoted > literal. (I fixed this already in describe.c, but not in those other > places.) Yes, I think so. > Once we've got that done, we could argue about whether appendStringLiteral > ought to prepend an E to silence escape_string_warning. I'd still vote > no, but at least it would be a single place to change and not N of 'em. > What's more, each place that is generating a variable literal without > using appendStringLiteral or PQescapeString is at least potentially > vulnerable to encoding issues, and so we should probably convert them > anyway. True. See the email I just sent about escape_string_warning. FYI, we are finding these places because of escape_string_warning. Do we trust users to turn that on and test before standard_conforming_strings becomes true. One big problem is that people are having to use E'' if they want to keep using backslashes, even if they have already tested standard_conforming_strings. One nice thing about E'' is that it works no matter what the value of standard_conforming_strings is. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
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'
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.
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.
regards, tom lane
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. +
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
Currently, psql single-quote argument strings can only embed single quotes as \', not ''. This is because while the main psqlscan.l loop understands '', the subsections used for psql arguments, xslasharg, doesn't. This patch attempts to fix that. However, I am not sure it is done right because I am not using the xe and xq state values, like the main psql scanner code. I assume we can not use them because we are already in xslasharg, and can't add another state here. The unusual thing is that in my testing it worked anyway. --------------------------------------------------------------------------- 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: doc/src/sgml/ref/psql-ref.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v retrieving revision 1.162 diff -c -c -r1.162 psql-ref.sgml *** doc/src/sgml/ref/psql-ref.sgml 26 May 2006 19:51:29 -0000 1.162 --- doc/src/sgml/ref/psql-ref.sgml 29 May 2006 17:49:43 -0000 *************** *** 2262,2268 **** copy the contents of a file into a table column. First load the file into a variable and then proceed as above. <programlisting> ! testdb=> <userinput>\set content '\'' `cat my_file.txt` '\''</userinput> testdb=> <userinput>INSERT INTO my_table VALUES (:content);</userinput> </programlisting> One possible problem with this approach is that <filename>my_file.txt</filename> --- 2262,2268 ---- copy the contents of a file into a table column. First load the file into a variable and then proceed as above. <programlisting> ! testdb=> <userinput>\set content '''' `cat my_file.txt` ''''</userinput> testdb=> <userinput>INSERT INTO my_table VALUES (:content);</userinput> </programlisting> One possible problem with this approach is that <filename>my_file.txt</filename> *************** *** 2270,2283 **** they don't cause a syntax error when the second line is processed. This could be done with the program <command>sed</command>: <programlisting> ! testdb=> <userinput>\set content '\'' `sed -e "s/'/\\\\\\'/g" < my_file.txt` '\''</userinput> </programlisting> Observe the correct number of backslashes (6)! It works this way: After <application>psql</application> has parsed this ! line, it passes <literal>sed -e "s/'/\\\'/g" < my_file.txt</literal> to the shell. The shell will do its own thing inside the double quotes and execute <command>sed</command> with the arguments ! <literal>-e</literal> and <literal>s/'/\\'/g</literal>. When <command>sed</command> parses this it will replace the two backslashes with a single one and then do the substitution. Perhaps at one point you thought it was great that all Unix commands use the --- 2270,2283 ---- they don't cause a syntax error when the second line is processed. This could be done with the program <command>sed</command>: <programlisting> ! testdb=> <userinput>\set content '''' `sed -e "s/'/\\\\''/g" < my_file.txt` ''''</userinput> </programlisting> Observe the correct number of backslashes (6)! It works this way: After <application>psql</application> has parsed this ! line, it passes <literal>sed -e "s/'/\\''/g" < my_file.txt</literal> to the shell. The shell will do its own thing inside the double quotes and execute <command>sed</command> with the arguments ! <literal>-e</literal> and <literal>s/'/''/g</literal>. When <command>sed</command> parses this it will replace the two backslashes with a single one and then do the substitution. Perhaps at one point you thought it was great that all Unix commands use the Index: src/bin/psql/psqlscan.l =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/psqlscan.l,v retrieving revision 1.18 diff -c -c -r1.18 psqlscan.l *** src/bin/psql/psqlscan.l 11 May 2006 19:15:35 -0000 1.18 --- src/bin/psql/psqlscan.l 29 May 2006 17:49:50 -0000 *************** *** 861,866 **** --- 861,868 ---- {quote} { return LEXRES_OK; } + {xqdouble} { emit("'", 1); } + "\\n" { appendPQExpBufferChar(output_buf, '\n'); } "\\t" { appendPQExpBufferChar(output_buf, '\t'); } "\\b" { appendPQExpBufferChar(output_buf, '\b'); }
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. +
Patch applied. Thanks. I see now that a state is not required because we are already in the single-quote string at that point, comment added. --------------------------------------------------------------------------- Bruce Momjian wrote: > > Currently, psql single-quote argument strings can only embed single > quotes as \', not ''. This is because while the main psqlscan.l loop > understands '', the subsections used for psql arguments, xslasharg, > doesn't. This patch attempts to fix that. > > However, I am not sure it is done right because I am not using the xe > and xq state values, like the main psql scanner code. I assume we can > not use them because we are already in xslasharg, and can't add another > state here. > > The unusual thing is that in my testing it worked anyway. > > --------------------------------------------------------------------------- > > 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: doc/src/sgml/ref/psql-ref.sgml > =================================================================== > RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v > retrieving revision 1.162 > diff -c -c -r1.162 psql-ref.sgml > *** doc/src/sgml/ref/psql-ref.sgml 26 May 2006 19:51:29 -0000 1.162 > --- doc/src/sgml/ref/psql-ref.sgml 29 May 2006 17:49:43 -0000 > *************** > *** 2262,2268 **** > copy the contents of a file into a table column. First load the file into a > variable and then proceed as above. > <programlisting> > ! testdb=> <userinput>\set content '\'' `cat my_file.txt` '\''</userinput> > testdb=> <userinput>INSERT INTO my_table VALUES (:content);</userinput> > </programlisting> > One possible problem with this approach is that <filename>my_file.txt</filename> > --- 2262,2268 ---- > copy the contents of a file into a table column. First load the file into a > variable and then proceed as above. > <programlisting> > ! testdb=> <userinput>\set content '''' `cat my_file.txt` ''''</userinput> > testdb=> <userinput>INSERT INTO my_table VALUES (:content);</userinput> > </programlisting> > One possible problem with this approach is that <filename>my_file.txt</filename> > *************** > *** 2270,2283 **** > they don't cause a syntax error when the second line is processed. This > could be done with the program <command>sed</command>: > <programlisting> > ! testdb=> <userinput>\set content '\'' `sed -e "s/'/\\\\\\'/g" < my_file.txt` '\''</userinput> > </programlisting> > Observe the correct number of backslashes (6)! It works > this way: After <application>psql</application> has parsed this > ! line, it passes <literal>sed -e "s/'/\\\'/g" < my_file.txt</literal> > to the shell. The shell will do its own thing inside the double > quotes and execute <command>sed</command> with the arguments > ! <literal>-e</literal> and <literal>s/'/\\'/g</literal>. When > <command>sed</command> parses this it will replace the two > backslashes with a single one and then do the substitution. Perhaps > at one point you thought it was great that all Unix commands use the > --- 2270,2283 ---- > they don't cause a syntax error when the second line is processed. This > could be done with the program <command>sed</command>: > <programlisting> > ! testdb=> <userinput>\set content '''' `sed -e "s/'/\\\\''/g" < my_file.txt` ''''</userinput> > </programlisting> > Observe the correct number of backslashes (6)! It works > this way: After <application>psql</application> has parsed this > ! line, it passes <literal>sed -e "s/'/\\''/g" < my_file.txt</literal> > to the shell. The shell will do its own thing inside the double > quotes and execute <command>sed</command> with the arguments > ! <literal>-e</literal> and <literal>s/'/''/g</literal>. When > <command>sed</command> parses this it will replace the two > backslashes with a single one and then do the substitution. Perhaps > at one point you thought it was great that all Unix commands use the > Index: src/bin/psql/psqlscan.l > =================================================================== > RCS file: /cvsroot/pgsql/src/bin/psql/psqlscan.l,v > retrieving revision 1.18 > diff -c -c -r1.18 psqlscan.l > *** src/bin/psql/psqlscan.l 11 May 2006 19:15:35 -0000 1.18 > --- src/bin/psql/psqlscan.l 29 May 2006 17:49:50 -0000 > *************** > *** 861,866 **** > --- 861,868 ---- > > {quote} { return LEXRES_OK; } > > + {xqdouble} { emit("'", 1); } > + > "\\n" { appendPQExpBufferChar(output_buf, '\n'); } > "\\t" { appendPQExpBufferChar(output_buf, '\t'); } > "\\b" { appendPQExpBufferChar(output_buf, '\b'); } > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> 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.
I find this patch mighty ugly, and hope that most of it can get reverted
before 8.2. The changes you made in describe.c are actively broken ...
didn't you test it? appendStringLiteralConn doesn't know what you did.
I think that a far saner approach would be to make all these places use
appendStringLiteralConn, and to change *only* that routine to throw on
an E at need. That would (a) not be broken, and (b) give us just one
place to change the behavior when it comes time.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> 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. > > I find this patch mighty ugly, and hope that most of it can get reverted > before 8.2. The changes you made in describe.c are actively broken ... > didn't you test it? appendStringLiteralConn doesn't know what you did. I will look into that. Thanks. > I think that a far saner approach would be to make all these places use > appendStringLiteralConn, and to change *only* that routine to throw on > an E at need. That would (a) not be broken, and (b) give us just one > place to change the behavior when it comes time. The problem is that pg_dump uses appendStringLiteralConn() too, and you didn't want pg_dump to emit E'', so you would have to add a boolean to appendStringLiteralConn() to say whether you want an optional E'' and that seems even uglier. -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +