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.

Sign up for GitHub

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

Jump to bottom

release-21.1: assorted pgwire and sql shell backports #66130

Merged
merged 3 commits into from
Jun 16, 2021
Merged

release-21.1: assorted pgwire and sql shell backports #66130

merged 3 commits into from
Jun 16, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Jun 7, 2021

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

@knz knz requested a review from rafiss June 7, 2021 13:13
@knz knz requested a review from a team as a code owner June 7, 2021 13:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

rafiss and others added 3 commits June 8, 2021 15:01
@rafiss @knz
pgwire: only show TZ minutes offset if non-zero
2a21e5c 
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.
@rafiss @knz
cli: format time zones and floating points to match SQL
f8cc409 
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.
@knz
cli/sql: properly format common array types
aba5a3d 
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 backport21.1-65063-66079 branch from b93cbb5 to aba5a3d Compare June 8, 2021 13:01
@rafiss
Copy link
Collaborator

rafiss commented Jun 8, 2021

i'll let @otan weigh in since he was opposed to the tz offset change being backported (#65341 (review)) -- but we can revisit this discussion

@rafiss rafiss requested a review from otan June 8, 2021 19:28
@otan
Copy link
Contributor

otan commented Jun 8, 2021

Yeah I think it's too big a change for a minor version (if we think it fits, why not every version before as well?).

otan
otan reviewed Jun 8, 2021
View reviewed changes
pkg/cli/sql_util.go
@@ -1134,6 +1145,11 @@ func formatVal(
if colType == "FLOAT4" {
width = 32
}
if math.IsInf(t, 1) {
return "Infinity"
Copy link
Contributor

Choose a reason for hiding this comment

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

my concern is anyone relying on this right now on a major version who wants a quick patch to some other problem but this breaks something else unexpectedly.

@rafiss
Copy link
Collaborator

rafiss commented Jun 8, 2021 •
edited
Loading

I'd be in favor of backporting to every (supported) version before as well -- that's just v21.1 and v20.2.

to me it doesn't seem that risky, since we are conforming closer to the pg spec/implementation, and we already know that all tools expect this new adjusted format. is the concern that people may have implemented custom CRDB drivers that don't work with Postgres at all?

@knz
Copy link
Contributor Author

knz commented Jun 9, 2021 •
edited
Loading

I'll let you two figure out whether to backport the TZ/float changes in #65063.

I chose to backport it because otherwise my other PR #66079 did not apply cleanly. If we're unsure about the TZ changes, I can do a bit more work to backport #66079 without it.

Just let me know what is best.

@knz
Copy link
Contributor Author

knz commented Jun 15, 2021

would love an opinion on the above

@otan otan changed the title release-21.1: asoorted pgwire and sql shell backports release-21.1: assorted pgwire and sql shell backports Jun 15, 2021
otan
otan approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

hmm, i guess the likelihood of someone having a really bad time with this is very slim.
since the rebase is effort too, and we're backporting it all the way down, i'm content with this being backported.

@knz
Copy link
Contributor Author

knz commented Jun 16, 2021

ok cool, I'll send the backport PR for 20.2 too then.

Cheers

@knz knz merged commit 12b59c6 into cockroachdb:release-21.1 Jun 16, 2021
@knz knz deleted the backport21.1-65063-66079 branch June 16, 2021 10:50
@knz knz mentioned this pull request Jun 16, 2021
Merged
@rafiss rafiss mentioned this pull request Jun 17, 2021
Closed
This was referenced Aug 13, 2021
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otan otan otan approved these changes

@rafiss rafiss Awaiting requested review from rafiss

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knz @cockroach-teamcity @rafiss @otan