Обсуждение: BUG #17767: psql: tab-completion causes warnings when standard_conforming_strings = off

The following bug has been logged on the website:

Bug reference:      17767
Logged by:          Takuya Aramaki
Email address:      takaram71@gmail.com
PostgreSQL version: 15.1
Operating system:   Debian bullseye
Description:

Hello,

I unexpectedly got some warning messages when using tab-completion of
psql.
I only face this issue with psql 15, not with v14.

Step to reproduce:
1. Set standard_conforming_strings = off
2. Type `\d _` and press Tab key

Actual result:
Got `WARNING:  nonstandard use of \\ in a string literal`

Expected result:
No warning messages

Below is my console log when I reproduced this issue with the official
docker image.

~~~
$ docker run --rm --name postgres -e POSTGRES_PASSWORD=pass -d
postgres:15.1
67361a066d40c8f98af6e21028839a2b6a9852dceaaf3d01c3e8c06ffae91d2e
$ docker exec -it postgres psql -h localhost -U postgres
psql (15.1 (Debian 15.1-1.pgdg110+1))
Type "help" for help.

postgres=# SET standard_conforming_strings = off;
SET
postgres=# \d _WARNING:  nonstandard use of \\ in a string literal
LINE 1: ...FROM pg_catalog.pg_class c WHERE (c.relname) LIKE '\\_%' AND...
                                                             ^
HINT:  Use the escape string syntax for backslashes, e.g., E'\\'.
WARNING:  nonstandard use of \\ in a string literal
LINE 3: ...OM pg_catalog.pg_namespace n WHERE n.nspname LIKE '\\_%' AND...
                                                             ^
HINT:  Use the escape string syntax for backslashes, e.g., E'\\'.
~~~

Thank you.

PG Bug reporting form <noreply@postgresql.org> writes:
> Step to reproduce:
> 1. Set standard_conforming_strings = off
> 2. Type `\d _` and press Tab key

> Actual result:
> Got `WARNING:  nonstandard use of \\ in a string literal`

Yeah, that should be coded a bit more robustly.  Will fix, thanks
for the report!

            regards, tom lane

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> Step to reproduce:
>> 1. Set standard_conforming_strings = off
>> 2. Type `\d _` and press Tab key

>> Actual result:
>> Got `WARNING:  nonstandard use of \\ in a string literal`

> Yeah, that should be coded a bit more robustly.  Will fix, thanks
> for the report!

Actually, that seems to be considerably more easily said than done.
I'd thought it'd only require a localized fix, but the issue is
that tab-complete.c is depending on PQescapeStringConn which
only cares about generating a legal literal, not about avoiding
the escape_string_warning warning.  To fix, we'd need to

(1) modify what tab-complete.c's escape_string() does, which isn't hard;

(2) modify every query string that incorporates an escape_string result,
along the lines of

-" WHERE d.datname LIKE '%s' "\
+" WHERE d.datname LIKE E'%s' "\

It might be better to change to

+" WHERE d.datname LIKE %s "\

and make escape_string() responsible for adding the E and quotes,
so that it's less likely someone forgets the E.  In any case,
step (2) is going to be enormously invasive to tab-complete.c,
and there'd be substantial risk of introducing new bugs.

This problem has been there a long time, though it doesn't manifest
in exactly this way before v15 changed tab-complete's implementation
of name matching.  In v14, I can get it to happen with

\d a\b<TAB>

which is less likely to be something somebody would try.

I'm having a hard time getting excited about making such a change,
TBH.  Why is it that you are running with
standard_conforming_strings = off and escape_string_warning = on
anyway?  If you haven't yet converted your apps to support
standard-conforming strings, you probably aren't intent on
doing so in the near future, so you might as well turn off
escape_string_warning.  We last worried about silencing such
warnings in our client programs more than a dozen years ago;
I'm not sure we should still be worried in 2023.

            regards, tom lane

At Wed, 01 Feb 2023 15:01:34 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> I'm having a hard time getting excited about making such a change,
> TBH.  Why is it that you are running with
> standard_conforming_strings = off and escape_string_warning = on
> anyway?  If you haven't yet converted your apps to support
> standard-conforming strings, you probably aren't intent on
> doing so in the near future, so you might as well turn off
> escape_string_warning.  We last worried about silencing such
> warnings in our client programs more than a dozen years ago;
> I'm not sure we should still be worried in 2023.

I personally fine with the current behavior for the same reason as you
raised. We could enclose completion queries by "BEGIN; SET LOCAL
standard_... = on;" and "COMMIT;" in exec_query but I think you don't
like that (and me neither).

If we don't make that change, it might be good to add a note to the
documentation for standard_conforming_strings something like "Note
that setting this to off on a psql session can cause tab-completion
emit WARNING when command lines containing backslashes.", but even
that may be too noisy against the benefit..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Wed, 01 Feb 2023 15:01:34 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
>> I'm having a hard time getting excited about making such a change,
>> TBH.  Why is it that you are running with
>> standard_conforming_strings = off and escape_string_warning = on
>> anyway?  If you haven't yet converted your apps to support
>> standard-conforming strings, you probably aren't intent on
>> doing so in the near future, so you might as well turn off
>> escape_string_warning.  We last worried about silencing such
>> warnings in our client programs more than a dozen years ago;
>> I'm not sure we should still be worried in 2023.

> I personally fine with the current behavior for the same reason as you
> raised. We could enclose completion queries by "BEGIN; SET LOCAL
> standard_... = on;" and "COMMIT;" in exec_query but I think you don't
> like that (and me neither).

Yeah ... also that's problematic if we're in a transaction, especially
an already-failed one.

Eventually I would like to remove the standard_conforming_strings GUC
altogether, primarily for the reason given at the top of gram.y:

 *      In general, nothing in this file should initiate database accesses
 *      nor depend on changeable state (such as SET variables).  If you do
 *      database accesses, your code will fail when we have aborted the
 *      current transaction and are just parsing commands to find the next
 *      ROLLBACK or COMMIT.  If you make use of SET variables, then you
 *      will do the wrong thing in multi-query strings like this:
 *            SET constraint_exclusion TO off; SELECT * FROM foo;
 *      because the entire string is parsed by gram.y before the SET gets
 *      executed.  Anything that depends on the database or changeable state
 *      should be handled during parse analysis so that it happens at the
 *      right time not the wrong time.

Now, I don't foresee that actually happening any time soon (and every
report of somebody still using standard_conforming_strings = off
pushes out my idea of when it could happen).  But perhaps allowing
minor annoyances like this to go unfixed would start to light a
fire under people to get off of that setting.

            regards, tom lane

At Thu, 02 Feb 2023 00:51:07 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > I personally fine with the current behavior for the same reason as you
> > raised. We could enclose completion queries by "BEGIN; SET LOCAL
> > standard_... = on;" and "COMMIT;" in exec_query but I think you don't
> > like that (and me neither).
> 
> Yeah ... also that's problematic if we're in a transaction, especially
> an already-failed one.
(Oops!)

> Eventually I would like to remove the standard_conforming_strings GUC
> altogether, primarily for the reason given at the top of gram.y:
> 
>  *      In general, nothing in this file should initiate database accesses
>  *      nor depend on changeable state (such as SET variables).  If you do
>  *      database accesses, your code will fail when we have aborted the
>  *      current transaction and are just parsing commands to find the next
>  *      ROLLBACK or COMMIT.  If you make use of SET variables, then you
>  *      will do the wrong thing in multi-query strings like this:
>  *            SET constraint_exclusion TO off; SELECT * FROM foo;
>  *      because the entire string is parsed by gram.y before the SET gets
>  *      executed.  Anything that depends on the database or changeable state
>  *      should be handled during parse analysis so that it happens at the
>  *      right time not the wrong time.
> 
> Now, I don't foresee that actually happening any time soon (and every
> report of somebody still using standard_conforming_strings = off
> pushes out my idea of when it could happen).  But perhaps allowing
> minor annoyances like this to go unfixed would start to light a
> fire under people to get off of that setting.

That seems like the right decision for to-be-obsolete features.  Or,
we can explicitly mar that variables as OBSOLETE then rip off it a few
years later.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2023-Feb-02, Kyotaro Horiguchi wrote:

> > Now, I don't foresee that actually happening any time soon (and every
> > report of somebody still using standard_conforming_strings = off
> > pushes out my idea of when it could happen).  But perhaps allowing
> > minor annoyances like this to go unfixed would start to light a
> > fire under people to get off of that setting.
> 
> That seems like the right decision for to-be-obsolete features.  Or,
> we can explicitly mar that variables as OBSOLETE then rip off it a few
> years later.

Hmm, really?  We could just remove it for 16; people will still be able
to use the warnings mode in 15 for several years, so if they want to
upgrade to 16+ they will have plenty of time to update their apps.

If we decide to keep it, then ISTM we should find a way to make this tab
completion thingy work silently.  But I'd rather we didn't spend any
time on it.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, really?  We could just remove it for 16; people will still be able
> to use the warnings mode in 15 for several years, so if they want to
> upgrade to 16+ they will have plenty of time to update their apps.

I think removing standard_conforming_strings = off might be a
bridge too far, even yet.  Or were you speaking of removing
escape_string_warning?  I could get behind that perhaps.
Making it default to off could be an even easier sell.

            regards, tom lane

On 2023-Feb-06, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Hmm, really?  We could just remove it for 16; people will still be able
> > to use the warnings mode in 15 for several years, so if they want to
> > upgrade to 16+ they will have plenty of time to update their apps.
> 
> I think removing standard_conforming_strings = off might be a
> bridge too far, even yet.  Or were you speaking of removing
> escape_string_warning?  I could get behind that perhaps.
> Making it default to off could be an even easier sell.

I was thinking we'd remove them together.  Anybody who is running
standard_conforming_strings=off will need the warning so that they can
find the places they need to touch in order to migrate.  Keeping the
ability to run nonstandard strings but without the ability to have the
warnings would be dangerous, because then there's no easy way to
upgrade.

So, if we want to keep standard_conforming_strings=off, then by all
means let's keep the warning too.

(I agree BTW with the idea that running psql with non-standard strings
and the warnings enabled is not something that we need to support
specifically.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Feb-06, Tom Lane wrote:
>> I think removing standard_conforming_strings = off might be a
>> bridge too far, even yet.  Or were you speaking of removing
>> escape_string_warning?  I could get behind that perhaps.
>> Making it default to off could be an even easier sell.

> I was thinking we'd remove them together.  Anybody who is running
> standard_conforming_strings=off will need the warning so that they can
> find the places they need to touch in order to migrate.  Keeping the
> ability to run nonstandard strings but without the ability to have the
> warnings would be dangerous, because then there's no easy way to
> upgrade.

Yeah, that's true.  So then the question is do we have any desire
to kill off standard_conforming_strings=off altogether?

You could certainly make an argument that doing so would be a net
security improvement, because it's likely that by now there are a
ton of applications that aren't careful with backslashes and will
have SQL-injection hazards if run under standard_conforming_strings=off.
Whether that argument will placate the people who don't want to
change their existing s_c_s=off-dependent apps, I dunno.

> (I agree BTW with the idea that running psql with non-standard strings
> and the warnings enabled is not something that we need to support
> specifically.)

Yeah, just changing the e_s_w default to "off" might be easiest.

            regards, tom lane

On 2023-Feb-06, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

> > (I agree BTW with the idea that running psql with non-standard strings
> > and the warnings enabled is not something that we need to support
> > specifically.)
> 
> Yeah, just changing the e_s_w default to "off" might be easiest.

Let's do a quick consult in pgsql-hackers to do that now.  It seems
we're right on time for 16.  (Actually, is -hackers the right audience?)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/