Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: does not properly respect the bytea_output setting for displaying BYTEA values #80398

Closed
rafiss opened this issue Apr 22, 2022 · 0 comments · Fixed by #81943
Closed

cli: does not properly respect the bytea_output setting for displaying BYTEA values #80398

rafiss opened this issue Apr 22, 2022 · 0 comments · Fixed by #81943
Labels
A-cli-client CLI commands that pertain to using SQL features C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@rafiss
Copy link
Collaborator

rafiss commented Apr 22, 2022

Describe the problem

The cockroach cli shows BYTEA values in a go-escaped format, which is difficult to work with if you want to use the value by copy/pasting it into a different SQL command.

To Reproduce

Connect to a CRDB cluster using cockroach sql and run:

root@:26257/defaultdb> select descriptor from system.descriptor limit 1;
                                                                      descriptor
-------------------------------------------------------------------------------------------------------------------------------------------------------
  \x129\n\x06system\x10\x01\x1a%\n\r\n\x05admin\x10\x80\x10\x18\x80\x10\n\f\n\x04root\x10\x80\x10\x18\x80\x10\x12\x04node\x18\x02"\x00(\x01@\x00J\x00

Expected behavior
If you connect using psql to the same cluster, you get:

defaultdb=> select descriptor from system.descriptor limit 1;
                                                        descriptor
--------------------------------------------------------------------------------------------------------------------------
 \x12390a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518022200280140004a00

Environment:

Additional context
Using a cast like select descriptor::text from system.descriptor limit 1; does work correctly.

@knz points out that the behavior is a limitation of lib/pq, see:

case []byte:
// Format the bytes as per bytea_output = escape.
//
// We use the "escape" format here because it enables printing
// readable strings as-is -- the default hex format would always
// render as hexadecimal digits. The escape format is also more
// compact.
//
// TODO(knz): this formatting is unfortunate/incorrect, and exists
// only because lib/pq incorrectly interprets the bytes received
// from the server. The proper behavior would be for the driver to
// not interpret the bytes and for us here to print that as-is, so
// that we can let the user see and control the result using
// `bytea_output`.
var buf bytes.Buffer
lexbase.EncodeSQLBytesInner(&buf, string(t))
return buf.String()

Moving the CLI to use pgx could help address this issue.

Jira issue: CRDB-15625

@rafiss rafiss added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-cli-client CLI commands that pertain to using SQL features labels Apr 22, 2022
@rafiss rafiss added this to To do in SQL CLI (demo + sql + workload + dump) via automation Apr 22, 2022
@rafiss rafiss added this to Triage in SQL Sessions - Deprecated via automation Apr 22, 2022
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Apr 22, 2022
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
@craig craig bot closed this as completed in b3589ca Jun 1, 2022
@craig craig bot closed this as completed in #81943 Jun 1, 2022
SQL CLI (demo + sql + workload + dump) automation moved this from To do to Done Jun 1, 2022
SQL Sessions - Deprecated automation moved this from Triage to Done Jun 1, 2022
cucaroach added a commit to cucaroach/cockroach that referenced this issue Oct 10, 2023
In cockroachdb#80398 we fixed the CLI to respect bytea_output
session variable and properly output BYTES columns in \xDEADBEEF format.
Now we don't need the extra hex_ columns. This makes debug.zips smaller
and quicker to create (jobs payloads can get big and so can descriptors
for large tables).

In order not to break cockroach debug decode-proto extend it to look
for and remove the \x prefix when attempting to decode hex.

Fixes: cockroachdb#112032
Epic: none
Release note (cli change): debug zip's no longer include redundant hex_
columns for system table BYTES columns.
craig bot pushed a commit that referenced this issue Oct 10, 2023
112033: cli: remove redundant hex_ columns in debug.zip output r=rickystewart a=cucaroach

In #80398 we fixed the CLI to respect bytea_output
session variable and properly output BYTES columns in \xDEADBEEF format.
Now we don't need the extra hex_ columns. This makes debug.zips smaller
and quicker to create (jobs payloads can get big and so can descriptors
for large tables).

In order not to break cockroach debug decode-proto extend it to look
for and remove the \x prefix when attempting to decode hex.

Fixes: #112032
Epic: none
Release note (cli change): debug zip's no longer include redundant hex_
columns for system table BYTES columns.


112040: sctest: Skip "crdb_internal" entry in comparator testing from logictest corpus r=Xiang-Gu a=Xiang-Gu

Statements collected from `crdb_internal` is of little interest to us
for the purpose of schema changer comparator testings, as it contains
only `crdb_internal` functions. Furthermore, there are statements that
will crash our framework, such as `SELECT crdb_internal.force_panic('foo')`.

This commit therefore adds a check to skip it. This check will also home
any future skipped entries for whatever reason.

Epic: None
Release note: None

112067: sql: fix minor bugs and add tests for procedures r=mgartner a=mgartner

#### sql: add tests for procedures with anonymous arguments

Release note: None

Epic: CRDB-25388

#### sql: fix `CREATE OR REPLACE PROCEDURE` and add tests

This commit fixes a minor bug with `CREATE OR REPLACE PROCEDURE` that
allowed it to replace functions. The bug also allowed
`CREATE OR REPLACE FUNCTION` to replace procedures. Tests for
`CREATE OR REPLACE PROCEDURE` have also been added.

Release note: None

#### sql: fix routine-related error messages

This commit fixes some minor bugs with the error message of
routine-related statements.

Release note: None


112074: optbuilder: move insert fast path test to optbuilder r=msirek a=msirek

This commit moves an insert fast path uniqueness check test case involving a volatile expression to a more applicable BuilderTest.

Epic: CRDB-26290
Informs: #58047

Release note: None

112086: serverutils: use options for SQL connections r=knz a=herkolategan

Previously `ApplicationLayerInterface` contained multiple `SQLConn` method variants for passing connection parameters. Various tests throughout the code use `SQLConn` or `PGUrl` to connect, but the latter does not take into account virtual clusters unless informed.

This PR is meant to enable `SQLConn` to become a standard replacement for `PGUrl` in order to leverage the probabilistic nature of the tests which select between running a second virtual cluster or not.

Numerous tests still use `PGUrl` and pass user, password, or cert details. These tests will soon be updated to start using `SQLConn` and hence should have the same functionality available. This enables the test to not have to deal with querying its probabilistic mode to determine if it should pass a virtual cluster name to `PGUrl`. The `ApplicationLayerInterface` already handles this logic and tests should leverage this.

See also: #111134
Epic: CRDB-31933

Release note: None

112096: optbuilder: move finishBuildLastStmt to routine.go r=mgartner a=mgartner

When routine-related optbuilder logic was moved to a new file,
`routine.go`, the `finishBuildLastStmt` function was missed. This commit
corrects that mistake.

Epic: CRDB-25388

Release note: None


112097: sql/logictest: add CALL statements to drop_procedure r=mgartner a=mgartner

This commit improves the coverage of tests in `drop_procedure` by adding
`CALL` statements to ensure that dropped procedures cannot be executed.

Epic: CRDB-25388

Release note: None


Co-authored-by: Tommy Reilly <treilly@cockroachlabs.com>
Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Mark Sirek <sirek@cockroachlabs.com>
Co-authored-by: Herko Lategan <herko@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli-client CLI commands that pertain to using SQL features C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Development

Successfully merging a pull request may close this issue.

2 participants