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

release-20.2: assorted pgwire and cli/sql backports #66533

Conversation

knz
Copy link
Contributor

@knz knz commented Jun 16, 2021

Release justification: bug fixes in CLI-only functionality.

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

@knz knz requested review from rafiss and otan June 16, 2021 11:21
@knz knz requested a review from a team as a code owner June 16, 2021 11:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Jun 16, 2021

I think I'm missing one PR on account of a pgwire/pgtest test case failing (one that pertains to timestamp)

@knz
Copy link
Contributor Author

knz commented Jun 16, 2021

any idea what PR that could be?

@otan
Copy link
Contributor

otan commented Jun 16, 2021

maybe b450c44 ?

@knz
Copy link
Contributor Author

knz commented Jun 16, 2021

the failure is from encodings.json

@rafiss
Copy link
Collaborator

rafiss commented Jun 16, 2021

it appears that the failing test is now:

--- FAIL: TestUtfName (0.40s)
    test_log_scope.go:158: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestUtfName939491298
    test_log_scope.go:63: use -show-logs to present logs inline
    sql_util_test.go:259: expected output:
          schema_name | table_name | type  | owner | estimated_row_count | locality
        --------------+------------+-------+-------+---------------------+-----------
          public      | żółw       | table | root  |                NULL | NULL
        (1 row)
        
        got:
          schema_name | table_name | type  | owner | estimated_row_count
        --------------+------------+-------+-------+----------------------
          public      | żółw       | table | root  |                   0
        (1 row)

i guess because #57653 is not in 20.2. looks like the test can be fixed to not expect the locality column

@knz
Copy link
Contributor Author

knz commented Jun 16, 2021

ok I'll iterate on this tomorrow or Friday

@otan otan added the X-noremind Bots won't notify about PRs with X-noremind label Jul 7, 2021
@otan otan removed their request for review July 13, 2021 22:31
@rafiss rafiss removed their request for review July 16, 2021 01:05
For casting timestamptz->text, only display the second offset if there
is one for timestamptz, to maintain compatibility with older versions.

Otherwise, for other outputs (e.g. CLI, pgwire), always show the full
zone offset including the seconds. Note the timezone zone offset doesn't
actually fully appear in the CRDB CLI due to lib/pq, but will
successfully display on postgresql connecting to CockroachDB.

Release note (bug fix): Fix a bug where the seconds component of a
zone offset of a timestamptz was not displayed.
@knz knz removed the X-noremind Bots won't notify about PRs with X-noremind label Jul 22, 2021
mrzasa and others added 11 commits July 22, 2021 18:08
Previously internal Name type with non-ASCII characters wwas shown
incorrectly in CLI (e.g. SHOW TABLES, SHOW CONSTRAINTS).
For instance: '\305\274\303\263\305\202w' instead of 'żółw'.
It caused inconvenience while using CLI.

To fix it this patch exposes ColumnTypeDatabaseTypeName in sqlRowsI
interface then uses this function to determine whether returned data is
of a Name type. If it's the case it's casted from []bytes to a string.

Release note (bug fix): Non-ASCII characters in NAME results in
`cockroach sql`  / `demo`,
(e.g. in the results SHOW TABLES, SHOW CONSTRAINTS) are now displayed
without being escaped to octal codes.
Looks like cockroachdb#55071 was too greedy -- looks like (at the very least)
pgjdbc expects the second offset to only be displayed if there is a
second offset. It's easy to do, so I have done so.

Fixed TimeTZ along the way.

Release note (bug fix): Second timezone offsets for TimeTZ now
correctly display over the postgres wire protocol - these were
previously omitted.

Release note (bug fix): Second timezone offsets are only displayed in
the postgres wire protocol for TimestampTZ values. In an earlier patch
for v21.1, this would always display.
There are a few minor differences: dataTypeSize is different for tuples,
and PG does not show seconds offset for times if it is zero.

Release note: None
Release note (bug fix): Integers inside of tuples were not being encoded
properly when using the binary format for retrieving data. This is now
fixed, and the proper integer width is reported.
Release note (bug fix): Blank-padded chars (e.g. CHAR(3)) were not being
encoded correctly when returning results to the client. Now they
correctly include blank-padding when appropriate.
Release note (bug fix): Collated strings were not encoded with the
proper type OID when sending results to the client if the OID was
for the `char` type. This is now fixed.
Prior to this patch, the SQL shell would improperly format
variants of timestamps (time, date) and mistakenly
use the same format for 32-bit and 64-bit float.
This patch corrects this.

Release note (cli change): The SQL shell (`cockroach demo`, `cockroach
sql`) now attempts to better format values that are akin to time/date,
as well as floating-point numbers.
The dscale is defined as number of digits (in base 10)
visible after the decimal separator," so a negative
dscale makes no sense and is not valid.

This was preventing NPGSQL from being able to decode the binary decimals
that CockroachDB was sending.

Release note (bug fix): The binary encoding of decimals no longer will
have negative `dscale` values. This was preventing Npgsql from being
able to read some binary decimals from CockroachDB.
Similar to cockroachdb#57265, this commit matches the PG behavior to
only show the minutes offset if it is non-zero.

Release note (bug fix): Minute timezone offsets are only displayed in
the wire protocol if they are non-zero for TimestampTZ and TimeTZ values.
Previously, they would always display.

Release note (bug fix): Binary TimeTZ values were not being decoded
correctly when being sent as a parameter in the wire protocol. This
is now fixed.
Release note (cli change): The SQL shell now formats times with time
zones so that the minutes and seconds offsets are only shown if they are
non-zero. Also, infinite floating point values are formatted as
`Infinity` rather than `Inf` now.
Release note (bug fix): CockroachDB's SQL shell now properly displays
results of common array types, for example arrays of floats or
arrays of strings.
@knz knz force-pushed the backport20.2-55071-56630-57265-61013-62310-64022-65063-66079 branch from c50495b to 909fc0e Compare July 22, 2021 16:08
@knz
Copy link
Contributor Author

knz commented Jul 22, 2021

Fixed; RFAL

@rafiss rafiss self-requested a review July 22, 2021 16:11
craig bot pushed a commit that referenced this pull request Jul 23, 2021
67937: clisqlexec: simplify a test r=otan a=knz

(this is the master version of a small change in #66533)

This makes the test more robust to future changes in the columns
returned by SHOW.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/sql/pgwire/types.go, line 71 at r12 (raw file):

// It is ignored (and can be nil) for types which do not need padding.
func (b *writeBuffer) writeTextDatum(
	ctx context.Context, d tree.Datum, conv sessiondata.DataConversionConfig, t *types.T,

just a theory, but it's possible the issue here is that conv.Location is not set correctly for the EncodingTest

this wouldn't affect later branches because they contain ab19e5c

@knz
Copy link
Contributor Author

knz commented Apr 10, 2022

wasn't able to make this work.

@knz knz closed this Apr 10, 2022
@knz knz deleted the backport20.2-55071-56630-57265-61013-62310-64022-65063-66079 branch April 10, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants