Skip to content

Decode text results defensively when the client encoding cannot decode them (SQL_ASCII) - #1628

Open
DiegoDAF wants to merge 1 commit into
dbcli:mainfrom
DiegoDAF:pr-sql-ascii-clean
Open

Decode text results defensively when the client encoding cannot decode them (SQL_ASCII)#1628
DiegoDAF wants to merge 1 commit into
dbcli:mainfrom
DiegoDAF:pr-sql-ascii-clean

Conversation

@DiegoDAF

@DiegoDAF DiegoDAF commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #1518. Fixes #1484.

When the client encoding cannot decode a text value (most commonly SQL_ASCII), psycopg returns text columns as raw bytes instead of str. Three code paths were passing those values through unguarded, producing the crashes reported in #1518 and #1484:

  • The socket directory query result is used to build the prompt, so a Unix socket connection raised TypeError: expected str, not bytes.
  • The show time zone result is printed in the startup message, showing a b'...'-prefixed value.
  • The function metadata rows feed the background completion refresher; parsing their bytes with parse_defaults() killed the refresh thread with TypeError: can only concatenate str (not "int") to str.

The fix decodes those values defensively when they come back as bytes, with the same guard the completion metadata fix for #1405 already uses (decode("utf-8", "replace")): a _decode_if_bytes() helper, plus a _decode_row() variant for the function metadata rows, which contain array columns.

Deliberately narrow in scope: function_definition(), search_path(), schemata() and databases() also return text and are still unguarded, but they are not involved in the reported crashes. Guarding them can be a follow-up if anyone hits them.

Reproduced against a real SQL_ASCII cluster (3396 catalog functions; all metadata fields come back as plain str after the fix). 6 unit tests: 3 fail without the fix, 3 pin the already-working str path so it stays unchanged.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install).
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

Sorry about the missing checklist, that was me writing the description by hand instead of starting from the template.

The third box is honestly unchecked, with a small finding attached: .pre-commit-config.yaml pins ruff-pre-commit at v0.11.7, while [testenv:style] in tox.ini installs ruff unpinned (0.15.x today). The two disagree on formatting, so running the hook reformats code that the CI style job then wants reformatted back. I ran ruff check and ruff format matching the CI instead, both clean. Happy to open a separate PR bumping the pre-commit rev if you would like them to agree.

function metadata

When the client encoding cannot decode a text value (most commonly
SQL_ASCII), psycopg returns text columns as raw bytes. Decode those
values defensively, using the same guard as the completion metadata
fix for dbcli#1405:

- get_socket_directory() and get_timezone() now decode their results,
  so the prompt no longer raises a TypeError on Unix socket
  connections and the timezone startup message no longer shows a
  b'...' value.
- functions() now decodes every metadata row before yielding, so the
  background completion refresh no longer dies in parse_defaults with
  a TypeError.

Closes dbcli#1484 and dbcli#1518. Related: dbcli#1405.
@j-bennet

j-bennet commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

@DiegoDAF nit: the PR checklist may seem boilerplate, but we prefer to have it. It gives us some confidence that the contributor familiarized themselves with our requirements.

@j-bennet

j-bennet commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Instead of decoding bytes, why don't we set client_encoding to utf8 on connect?

@dbaty

dbaty commented Sep 4, 2026

Copy link
Copy Markdown
Member

I tried a different approach a few weeks/months ago, in #1629. It's not fully tested but I think it goes in a better direction than forcefully and blindly converting bytes to strings.

@DiegoDAF

DiegoDAF commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Good question, and thanks @dbaty for #1629, I had not seen it. I set up a real SQL_ASCII cluster to compare the two approaches rather than argue from theory, and the results are worth sharing because they point at a combination rather than a winner.

The catch is that client_encoding=utf8 only helps when the bytes happen to be valid UTF-8. With SQL_ASCII the server performs no conversion, so invalid sequences reach the client untouched and the server rejects the query outright.

Setup: initdb -E SQL_ASCII, one table and one column holding the latin-1 byte 0xF1 (Muñoz), plus a table whose identifier contains the same byte.

Reading the data:

today                  b'Mu\xf1oz'                      -> bytes, pgcli raises
client_encoding=utf8   CharacterNotInRepertoire      -> "invalid byte sequence for encoding UTF8: 0xf1 0x6f 0x7a"
decode(errors=replace) 'Mu�oz'                      -> degraded but readable

Reading identifiers from the catalog (this is issue #1405's case):

today                  [b'clientes', b'Mu\xf1oz_tbl']
client_encoding=utf8   CharacterNotInRepertoire      -> the catalog query itself fails, so completions break entirely
decode(errors=replace) ['clientes', 'Mu�oz_tbl']

So on a SQL_ASCII database that actually holds non-UTF-8 bytes, forcing the encoding turns a display problem into "the query fails", including the metadata queries that drive autocompletion.

That said, #1629 is clearly better where it applies. On a SQL_ASCII database whose content happens to be pure ASCII, which I suspect is most of the affected users, it returns real str with no replacement characters and no lossy decode, and it covers every path at once instead of the three this PR guards.

They are not really competing, then. #1629 gets the encoding right when the data allows it; the defensive decode is what keeps pgcli usable when it does not. If you like, I can rework this PR into that shape: override_client_encoding as the primary mechanism, falling back to the defensive decode when the server raises CharacterNotInRepertoire. Happy to do it either on top of #1629 or as a revision here, whichever you prefer, and equally happy to close this one if you would rather take #1629 alone first and treat the fallback as a follow-up.

I can also contribute the SQL_ASCII fixture and tests from this comparison to whichever PR you keep, since #1629 mentions automated tests are lacking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants