Err(CoordinatorInconsistencies { catalog_inconsistencies: CatalogInconsistencies { internal_fields: [], roles: [], comments: [], object_dependencies: [], items: [StatementParseFailure { create_sql: "CREATE SUBSOURCE #22535
Assignees
Labels
C-bug
Category: something is broken
Comments
5 tasks
5 tasks
ParkMyCar
added a commit
that referenced
this issue
Nov 15, 2023
This PR adds a max length to the `Ident` struct. In #20999 we added the concept of a "max_identifier_length" which was enforced when parsing SQL, but it was not enforced for identifiers created internally, e.g. when generating a name for a progress subsource by appending "_progress" to the source name. The largest changes in this PR are the following APIs: * **`Ident::new(...)`** will return an `Err(IdentError)` if the provided string is longer than our max, which is 255 bytes. * **`ident!("some static str")`** macro, which enforces our invariants at compile time. This prevents a pattern like `Ident::new("some static str").expect("known correct")` from polluting the code base. * **`Ident::new_unchecked(...)`** checks the invariants with a `soft_assert!`. This pattern is an escape hatch for cases we know are valid, but can't express in the type system, e.g. appending a number to a static string. Wherever possible I used `Ident::new(...)` or `ident!(...)`, but there were some callsites that didn't already return an error, and didn't have a `&'static str` as an argument. For those cases I used `Ident::new_unchecked(...)` to prevent this PR from getting too large. At the very least by using `new_unchecked(...)` our tests will catch invalid `Ident`s via soft asserts. ### Motivation Fixes #22535 But also has the goal of eliminating these kinds of bugs entirely. Chatted about this on [Slack](https://materializeinc.slack.com/archives/C02FWJ94HME/p1699476924158659) and it seemed desirable. ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [x] This PR includes the following [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note): - Internal only change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What version of Materialize are you using?
3f50285
What is the issue?
Found using my WIP branch #22521
Testdrive reproducer:
The subsource name is too long, but we still accept the
CREATE SOURCEcommand and then fail later. If this doesn't fall into the Surfaces area, then please assign to Storage team. I think the fix is to check how long the source+subsource combination will be and not allow creating sources which will lead to too long subsources.The text was updated successfully, but these errors were encountered: