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

colinfo: add version gate for storing pg_lsn types #105391

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

otan
Copy link
Contributor

@otan otan commented Jun 22, 2023

We don't want mixed version clusters storing pg_lsn types, in case they need to rollback / older versions do not understand the type.

Informs #105130

Release note: None

@otan otan requested a review from rafiss June 22, 2023 21:10
@otan otan requested a review from a team as a code owner June 22, 2023 21:10
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan otan force-pushed the mixed_version_testing branch from 62c8e6e to 0643f6f Compare June 22, 2023 21:31
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.

do we also need to block queries that use this data type in a cast? (that's what we had to do for tsquery)

we need to teach the mixed version tests to not use this type too. see 4607a74 and fd09e18

@@ -542,6 +542,9 @@ const (
// that (optionally) embed below-raft admission data.
V23_2_UseACRaftEntryEntryEncodings

// V23_2_PGLogicalReplication gates the use of PGLogicalReplication features.
V23_2_PGLogicalReplication
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no need to make a new cluster version. you can just use clusterversion.V23_2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no V23_2 though, only V23_2Start.

Copy link
Contributor Author

@otan otan Jun 22, 2023

Choose a reason for hiding this comment

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

oh it's not in the const

@otan otan force-pushed the mixed_version_testing branch 2 times, most recently from b7a921f to cd928c1 Compare June 22, 2023 21:56
@otan otan requested a review from a team as a code owner June 22, 2023 21:56
@otan otan requested review from smg260 and renatolabs and removed request for a team June 22, 2023 21:56
@otan otan force-pushed the mixed_version_testing branch from cd928c1 to aad4c19 Compare June 22, 2023 21:57
@otan otan requested a review from rafiss June 22, 2023 21:57
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.

not gonna block this, but see my comment about cast expressions

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan, @renatolabs, and @smg260)


pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed line 2 at r1 (raw file):

# LogicTest: local-mixed-22.2-23.1
# TODO(otan): add tests for mixed 23.1-23.2.

i've got this PR brewing #104994


pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed line 5 at r1 (raw file):


statement error pg_lsn not supported until version 23.2
CREATE TABLE pg_lsn_table(id pg_lsn, val pg_lsn)

i feel like we also need logic for blocking pg_lsn in cast expressions (and a test), otherwise won't distsql get confused in a mixed version cluster?

@otan
Copy link
Contributor Author

otan commented Jun 26, 2023

pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed line 5 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i feel like we also need logic for blocking pg_lsn in cast expressions (and a test), otherwise won't distsql get confused in a mixed version cluster?

i'll add a test; i've never had to add a mixed version test for casts before - not sure there's a great place to do that either

We don't want mixed version clusters storing pg_lsn types, in case they
need to rollback / older versions do not understand the type.

Release note: None
@otan otan force-pushed the mixed_version_testing branch from aad4c19 to ff12251 Compare June 26, 2023 10:39
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 @otan, @renatolabs, and @smg260)


pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed line 5 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

i'll add a test; i've never had to add a mixed version test for casts before - not sure there's a great place to do that either

you can use this same logic test file. what would be the problem with that?

@otan
Copy link
Contributor Author

otan commented Jun 26, 2023

pkg/sql/logictest/testdata/logic_test/pg_lsn_mixed line 5 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you can use this same logic test file. what would be the problem with that?

sorry, i meant adding a version gate for "check you can't ever instantiate a pg_lsn instance anywhere distsql is used".

@otan
Copy link
Contributor Author

otan commented Jun 26, 2023

bors r=rafiss

mergin' for now, will update mixed-version tests later

@rafiss
Copy link
Collaborator

rafiss commented Jun 26, 2023

sorry, i meant adding a version gate for "check you can't ever instantiate a pg_lsn instance anywhere distsql is used".

you don't need to do this. all we'd need is a gate for using pg_lsn in cast expressions. for an example, see the change in pkg/sql/sem/eval/cast.go in #90842

i've added two new items to your tracking issue in #105130

Version gate pg_lsn type in cast expressions
Add pgwire encoding tests for pg_lsn to pkg/cmd/generate-binary/main.go

@craig craig bot merged commit 15d43bb into cockroachdb:master Jun 26, 2023
@craig
Copy link
Contributor

craig bot commented Jun 26, 2023

Build succeeded:

@otan
Copy link
Contributor Author

otan commented Jun 26, 2023

Add pgwire encoding tests for pg_lsn to pkg/cmd/generate-binary/main.go

already there!

@otan
Copy link
Contributor Author

otan commented Jun 26, 2023

oh wait, i changed the json but forgot to commit the generate-binary changes

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.

3 participants