Re: PSQL schema "describe" \dn is not escaping quotes - Mailing list pgsql-hackers
| From | surya poondla |
|---|---|
| Subject | Re: PSQL schema "describe" \dn is not escaping quotes |
| Date | |
| Msg-id | CAOVWO5owc=bd05vqu4-3J5BbKwih8wwB11gFBQ9c5V+qqtOqJA@mail.gmail.com Whole thread |
| In response to | Re: PSQL schema "describe" \dn is not escaping quotes (Peter Smith <smithpb2250@gmail.com>) |
| Responses |
Re: PSQL schema "describe" \dn is not escaping quotes
|
| List | pgsql-hackers |
Hi Peter,
Thanks for v7. It applies cleanly on master, builds without warnings, and make check passes.
The quoting fix itself looks right: both queries now take the actual schema name via appendStringLiteralConn(). I tried names with embedded ', ", \
and ., plus mixed case. They all display correctly with the right publications footer, and \dn "e_sch1" now shows the footer that master silently left out.
I have a couple of comments:
1. Backpatching. Nishant showed the bug goes back to PG15, but v7 also changes what \dn prints whenever a pattern is given:
- each match is now its own "Schema "x"" table instead of one "List of schemas"
- the "(N rows)" footer is gone
- in non-quiet mode, a pattern with no match is now an error
I don't think changes to user-visible output can go into 15–18. Could this be split into two patches? 0001 would be a minimal escaping fix that can be backpatched, e.g. keying the footer off PQgetvalue(res, 0, 0) when exactly one row comes back. 0002 would be the per-schema display, master only, and would probably need its own discussion. psql-ref.sgml also still says matching schemas "are listed", so 0002 would need a doc update.
2. The no-match behaviour doesn't actually match \dt. The comment says "to be same as \dt", but listTables() prints the error and then returns true, while listSchemas() jumps to error_return and returns false. So under ON_ERROR_STOP, \dn nosuch now aborts a script where it used to print an empty table:
$ psql -v ON_ERROR_STOP=1 -c '\dn e_nosuch' -c 'select 1'; echo "exit=$?"
Did not find any schemas named "e_nosuch".
exit=1
$ psql -v ON_ERROR_STOP=1 -c '\dt e_nosuch' -c 'select 1'; echo "exit=$?"
Did not find any tables named "e_nosuch".
?column?
----------
1
(1 row)
exit=0
Also:
- the no-pattern message "Did not find any schemas" has no trailing period, unlike the other messages in describe.c
- in quiet mode a no-match still prints the old empty "List of schemas" table, so there are two output styles depending on -q
- pg_regress runs psql with -q, so none of the new error paths are exercised by the regression tests
3. \dn * now prints a separate table for every schema in the database, pg_catalog, pg_toast and information_schema included. With CSV output, a multi-match pattern now produces several header rows in a row, which would break anything that parses the output:
\pset format csv
\dn e_sch*
Name,Owner
e_sch1,surya
Name,Owner
e_sch2,surya
Name,Owner
e_sch3,surya
This is one more reason to keep the redesign out of the bug fix, or at least call it out explicitly.
4. Minor issues in describeOneSchemaDetails():
- The footer query has no separator after the literal. With ECHO_HIDDEN it shows up as WHERE n.nspname = 'e_sch1'ORDER BY 1;. It works, but a "\n" before ORDER BY would fix it.
- The footer query still uses appendPQExpBuffer() with no format arguments. Jim pointed this out for v1, but v2 only changed the ORDER BY line. appendPQExpBufferStr() would do.
- myopt.topt.default_footer = false is only set inside the sversion >= 150000 branch, so against an older server each schema table still ends with "(1 row)".
5. Tests:
- CREATE PUBLICATION pub_sch_1 FOR TABLES IN SCHEMA SCH_1 is unquoted, so it publishes sch_1, not "SCH_1". The expected output is correct, but a reader will probably assume the opposite. Could you add a comment, or use distinct names?
- There's no coverage for \dn+ with a pattern or for names containing a backslash.
Regards,
Surya Poondla
Thanks for v7. It applies cleanly on master, builds without warnings, and make check passes.
The quoting fix itself looks right: both queries now take the actual schema name via appendStringLiteralConn(). I tried names with embedded ', ", \
and ., plus mixed case. They all display correctly with the right publications footer, and \dn "e_sch1" now shows the footer that master silently left out.
I have a couple of comments:
1. Backpatching. Nishant showed the bug goes back to PG15, but v7 also changes what \dn prints whenever a pattern is given:
- each match is now its own "Schema "x"" table instead of one "List of schemas"
- the "(N rows)" footer is gone
- in non-quiet mode, a pattern with no match is now an error
I don't think changes to user-visible output can go into 15–18. Could this be split into two patches? 0001 would be a minimal escaping fix that can be backpatched, e.g. keying the footer off PQgetvalue(res, 0, 0) when exactly one row comes back. 0002 would be the per-schema display, master only, and would probably need its own discussion. psql-ref.sgml also still says matching schemas "are listed", so 0002 would need a doc update.
2. The no-match behaviour doesn't actually match \dt. The comment says "to be same as \dt", but listTables() prints the error and then returns true, while listSchemas() jumps to error_return and returns false. So under ON_ERROR_STOP, \dn nosuch now aborts a script where it used to print an empty table:
$ psql -v ON_ERROR_STOP=1 -c '\dn e_nosuch' -c 'select 1'; echo "exit=$?"
Did not find any schemas named "e_nosuch".
exit=1
$ psql -v ON_ERROR_STOP=1 -c '\dt e_nosuch' -c 'select 1'; echo "exit=$?"
Did not find any tables named "e_nosuch".
?column?
----------
1
(1 row)
exit=0
Also:
- the no-pattern message "Did not find any schemas" has no trailing period, unlike the other messages in describe.c
- in quiet mode a no-match still prints the old empty "List of schemas" table, so there are two output styles depending on -q
- pg_regress runs psql with -q, so none of the new error paths are exercised by the regression tests
3. \dn * now prints a separate table for every schema in the database, pg_catalog, pg_toast and information_schema included. With CSV output, a multi-match pattern now produces several header rows in a row, which would break anything that parses the output:
\pset format csv
\dn e_sch*
Name,Owner
e_sch1,surya
Name,Owner
e_sch2,surya
Name,Owner
e_sch3,surya
This is one more reason to keep the redesign out of the bug fix, or at least call it out explicitly.
4. Minor issues in describeOneSchemaDetails():
- The footer query has no separator after the literal. With ECHO_HIDDEN it shows up as WHERE n.nspname = 'e_sch1'ORDER BY 1;. It works, but a "\n" before ORDER BY would fix it.
- The footer query still uses appendPQExpBuffer() with no format arguments. Jim pointed this out for v1, but v2 only changed the ORDER BY line. appendPQExpBufferStr() would do.
- myopt.topt.default_footer = false is only set inside the sversion >= 150000 branch, so against an older server each schema table still ends with "(1 row)".
5. Tests:
- CREATE PUBLICATION pub_sch_1 FOR TABLES IN SCHEMA SCH_1 is unquoted, so it publishes sch_1, not "SCH_1". The expected output is correct, but a reader will probably assume the opposite. Could you add a comment, or use distinct names?
- There's no coverage for \dn+ with a pattern or for names containing a backslash.
Regards,
Surya Poondla
pgsql-hackers by date: