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

descs: legacy schema changes should use a dedicated interface with sane defaults #87753

Closed
postamar opened this issue Sep 9, 2022 · 0 comments · Fixed by #94695
Closed

descs: legacy schema changes should use a dedicated interface with sane defaults #87753

postamar opened this issue Sep 9, 2022 · 0 comments · Fixed by #94695
Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.

Comments

@postamar
Copy link
Contributor

postamar commented Sep 9, 2022 •
edited by cockroach-jira-scripts
Loading

The default behaviour of GetImmutableTableByID and similar methods is to lookup leased descriptors when available and we have to set an AvoidLeased flag explicitly to avoid that. The problem is that when it comes to schema changes, we always want to avoid leased descriptors. We have a CommonLookupFlags() method on resolver.SchemaResolver which does the right thing but it's not enough.

Case in point: in 21.2, the canRemoveEnumValue is buggy because it happily looks up a leased table descriptor. This has since been fixed by #79697. But this class of bug can easily happen again.

Perhaps the solution is to flip the tables and replace AvoidLeased with IncludeLeased and have the optimizer set that flag. It might be less trouble. Alternatively, wrap the descs.Collection behind an interface that the planner can readily use when operating on schema change PlanNodes.

Jira issue: CRDB-19516
@postamar postamar added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Sep 9, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Sep 9, 2022
@postamar postamar added A-schema-catalog Related to the schema descriptors collection and the catalog API in general. and removed T-sql-schema-deprecated Use T-sql-foundations instead labels Sep 9, 2022
@postamar postamar added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Nov 10, 2022
@postamar postamar mentioned this issue Dec 12, 2022
Closed
@postamar postamar mentioned this issue Dec 16, 2022
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 19, 2022
descs: add ByIDGetter and ByNameGetter interfaces
3afba2a 
These pave the way for deprecating the existing Get*ByID and Get*ByName
Collection methods.

Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Dec 20, 2022
descs: add ByIDGetter and ByNameGetter interfaces
a3293f1 
These pave the way for deprecating the existing Get*ByID and Get*ByName
Collection methods.

Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 2, 2023
descs: add ByIDGetter and ByNameGetter interfaces
448abbb 
These pave the way for deprecating the existing Get*ByID and Get*ByName
Collection methods.

Informs cockroachdb#87753.

Release note: None
craig bot pushed a commit that referenced this issue Jan 2, 2023
Merge #93813
34dbb59 
93813: descs: add ByIDGetter and ByNameGetter interfaces r=postamar a=postamar

These pave the way for deprecating the existing `Get*ByID` and `Get*ByName` `Collection` methods.

Informs #87753.

Release note: None

Co-authored-by: Marius Posta <[email protected]>
@postamar postamar mentioned this issue Jan 4, 2023
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 4, 2023
descs: by-ID database and schema lookups error when not found
df10c82 
Previously, for some inscrutable reason, by-ID lookups of database and
schema descriptors were treated differently from all other by-ID lookups
in that it was possible to not return an error when the descriptor was
not found. This commit fixes this discrepancy.

Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 4, 2023
sql: inline GetImmutableDescriptorsByID
559582b 
Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 4, 2023
sql: inline GetImmutableDescriptorByID
7532d8f 
Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 4, 2023
sql: inline GetImmutableDatabaseByID
44db76e 
Informs cockroachdb#87753.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 4, 2023
sql: inline GetImmutableSchemaByID
9e61d7c 
Informs cockroachdb#87753.

Release note: None
craig bot pushed a commit that referenced this issue Jan 6, 2023
@lidorcarmel
Merge #93755 #94695
39ee43d 
93755: streamingest: c2c UX fixes r=lidorcarmel a=lidorcarmel

CREATE TENANT FROM REPLICATION: currently has an output (the job ids of the producer and consumer). This PR removes the output.

ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output.

ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used.

Fixes: #94706

Epic: CRDB-18752

Release note: None

94695: sql,descs,*: rewrite by-ID and by-name descriptor lookups r=postamar a=postamar

This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the
descs.Collection and their usages with the recently-introduced
By(ID|Name)Getter methods.

While this commit shouldn't fundamentally change anything as far as
functionality is concerned, it isn't strictly-speaking a refactor
because the default behaviors have been changed slightly to make them
consistent accross similar kinds of lookups. Here's an illustrative
example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID
would honor the Required flag while the other GetImmutable*ByID methods
would ignore it and return an error when the descriptor is not found,
presently all immutable by-ID lookups always return an error when the
descriptor is not found.

The new API does away with the tree.(Common|Object)LookupFlags and these
have been cleaned up because now only the resolver uses them.

Fixes #87753.

Release note: None

Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
@craig craig bot closed this as completed in af6a72a Jan 6, 2023
kvoli pushed a commit to kvoli/cockroach that referenced this issue Jan 10, 2023
@lidorcarmel @kvoli
Merge cockroachdb#93755 cockroachdb#94695
064beb9 
93755: streamingest: c2c UX fixes r=lidorcarmel a=lidorcarmel

CREATE TENANT FROM REPLICATION: currently has an output (the job ids of the producer and consumer). This PR removes the output.

ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output.

ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used.

Fixes: cockroachdb#94706

Epic: CRDB-18752

Release note: None

94695: sql,descs,*: rewrite by-ID and by-name descriptor lookups r=postamar a=postamar

This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the
descs.Collection and their usages with the recently-introduced
By(ID|Name)Getter methods.

While this commit shouldn't fundamentally change anything as far as
functionality is concerned, it isn't strictly-speaking a refactor
because the default behaviors have been changed slightly to make them
consistent accross similar kinds of lookups. Here's an illustrative
example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID
would honor the Required flag while the other GetImmutable*ByID methods
would ignore it and return an error when the descriptor is not found,
presently all immutable by-ID lookups always return an error when the
descriptor is not found.

The new API does away with the tree.(Common|Object)LookupFlags and these
have been cleaned up because now only the resolver uses them.

Fixes cockroachdb#87753.

Release note: None

Co-authored-by: Lidor Carmel <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql,descs,*: rewrite by-ID and by-name descriptor lookups postamar/cockroach
1 participant
@postamar