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

tree: correctly format the "unknown" oid for pg compatibility #118587

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

DrewKimball
Copy link
Collaborator

@DrewKimball DrewKimball commented Feb 1, 2024

The REGTYPE, REGPROC, REGCLASS, and REGNAMESPACE types are all
in the oid type family, but unlike OID, display - instead of 0 when
the value is zero. Previously, this was handled by setting the name field
of DOid to -, but the handling was incomplete. This commit removes the
old special cases, and instead adds special handling to the functions that
parse an oid from a string, as well as the DOid.Format() method. This way,
correctly formatting the "unknown" oid doesn't rely on remembering to set
the name field.

In addition, this commit modifies some of the random datum generation
logic to create types other than the canonical T_oid, like T_regrole.

Fixes #118273

Release note: None

@DrewKimball DrewKimball requested a review from a team as a code owner February 1, 2024 18:06
@DrewKimball DrewKimball requested review from rytaft and removed request for a team February 1, 2024 18:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice find!!

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rytaft)

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @rytaft)

@DrewKimball
Copy link
Collaborator Author

TFTRs!

I ended up having to make some changes to the places where we generate datums for tests, as well.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

@DrewKimball DrewKimball force-pushed the oid branch 2 times, most recently from 1b3abd8 to b10edcb Compare February 29, 2024 05:41
@DrewKimball DrewKimball requested review from a team as code owners February 29, 2024 05:41
@DrewKimball DrewKimball changed the title keyside,valueside: correctly set name for 0 oid tree: correctly format the "unknown" oid for pg compatibility Feb 29, 2024
@DrewKimball
Copy link
Collaborator Author

After wrestling with tests for a bit, I've changed the approach. I removed the old (incomplete) handling of "unknown" oids. We now only output the - when either printing text results to the client, or when casting to a string. There are also two locations where we parse an oid from a string; those correctly handle - as well.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 5 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @mgartner, @michae2, and @rytaft)


pkg/sql/logictest/testdata/logic_test/pgoidtype line 702 at r3 (raw file):

INSERT INTO t118273 VALUES (0, 0);

skipif config local-mixed-23.1

nit: I think you can mention multiple configs to skip on a single line.

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.

nice!

Reviewed 1 of 5 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @mgartner, @michae2, and @rytaft)

Copy link
Collaborator Author

@DrewKimball DrewKimball 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 (and 3 stale) (waiting on @mgartner, @michae2, @rafiss, @rytaft, and @yuzefovich)


pkg/sql/logictest/testdata/logic_test/pgoidtype line 702 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think you can mention multiple configs to skip on a single line.

TIL, thanks.

@yuzefovich
Copy link
Member

@DrewKimball should we push it over the finish line? We just got another failure like this over in #122817

@DrewKimball
Copy link
Collaborator Author

Sorry, I lost track of this one. I'll rebase.

The `REGTYPE`, `REGPROC`, `REGCLASS`, and `REGNAMESPACE` types are all
in the oid type family, but unlike `OID`, display `-` instead of `0` when
the value is zero. Previously, this was handled by setting the `name` field
of `DOid` to `-`, but the handling was incomplete. This commit removes the
old special cases, and instead adds special handling to the functions that
parse an oid from a string, as well as the `DOid.Format()` method. This way,
correctly formatting the "unknown" oid doesn't rely on remembering to set
the `name` field.

In addition, this commit modifies some of the random datum generation
logic to create types other than the canonical `T_oid`, like `T_regrole`.

Fixes cockroachdb#118273

Release note: None
@DrewKimball
Copy link
Collaborator Author

TFTRs!

bors r+

@craig craig bot merged commit 32e46c2 into cockroachdb:master Apr 30, 2024
22 checks passed
@DrewKimball DrewKimball deleted the oid branch April 30, 2024 09:14
@yuzefovich
Copy link
Member

blathers backport release-24.1

Copy link

blathers-crl bot commented Apr 30, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from a4b6234 to blathers/backport-release-24.1-118587: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-24.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@yuzefovich
Copy link
Member

blathers backport release-23.2

Copy link

blathers-crl bot commented Apr 30, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from a4b6234 to blathers/backport-release-23.2-118587: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-23.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner
Copy link
Collaborator

blathers backport release-23.1

Copy link

blathers-crl bot commented Aug 15, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from a4b6234 to blathers/backport-release-23.1-118587: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-23.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

roachtest: [- replaced with 0] unoptimized-query-oracle/disable-rules=all/rand-tables failed
6 participants