-
Notifications
You must be signed in to change notification settings - Fork 476
SQL Queries v25.4 known limitations #20811
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
Conversation
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
Files changed:
|
✅ Netlify Preview
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice!
src/current/_includes/v25.4/known-limitations/read-committed-limitations.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the one nit I listed in the comments, there's one thing that I don't understand, which has to do with the duplication of limitations across the two sections of the main page: "v25.4" and "v25.3 and earlier". Of the 8 new issues listed in the "v25.4" section, 5 of them (those under the "User-defined functions" "Stored procedures", "Mixed-isolation workloads", and "Data domiciling" headers) are also listed in the "v25.3 and earlier" section. Is this accidental, or am I not understanding something? As a reader, I would think that a limitation is either new or it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much this matters, but this introduces a second "Known limitations" header to the /docs/dev/views page, which slightly messes with the ability to navigate to the bottom one via the ToC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize! Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On your other point, thank you for raising it. This is admittedly clunky, and is an outcome of how the includes are set up. In the v25.3 section, all KLs for a given topic are contained in the include for that topic. That include file is embedded on the respective feature pages (like for UDFs and Stored Procedures), where all the relevant KLs can also be viewed.
Because of how the main Known Limitations page is set up, we also have the newly identified KLs in the top section. The "correct" way to do this would be to have the lines appear only 1) in the top section of new KLs, 2) on the various feature pages, outside the KL includes (so that they aren't missing there). I felt this would add too much burden for the next version, because we'd have to take the "new" lines and distribute them among the appropriate include files. The way it is now, we just have to clear everything in the "new" section when we update this page for 26.1.
The headings on this page may also be misleading: "New limitations" means "newly identified limitations"; "Limitations from v24.3 and earlier" means "limitations we already knew about when these versions were released". I don't think most (if any) of these would be considered "newly introduced" limitations unless the feature itself were new, in which case it wouldn't be in the "earlier" section anyway. Perhaps we should adjust the headings. In any case, it means it's not a total contradiction to have them in two places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to get my head around this... I'm a little confused because I don't see this same duplication in previous versions when I check, for example, https://www.cockroachlabs.com/docs/v25.3/known-limitations
I guess I'd like to better understand what's changed.
I suppose in a ideal case you could have two separate include files, one for old limitations and one for new limitations, and you separate them on the main known limitations page and join them up on the feature page, but I understand that that might be too much overhead and/or might be out of scope for this PR.
None of this is a big deal I'm just curious and dogged haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the reason the 25.3 "new" limitations aren't duplicated is because they were only placed in the "new" section and not in any of the feature sections. In this PR, I moved them all to existing & new subheadings on the KLs page that I thought were appropriate.
Also, the use of includes for KLs is intended to enable them to be embedded in multiple docs. The way I do it currently, all the KLs for (e.g.) stored procedures are in a stored-proc-limitations.md include that's embedded on both the KLs page and the Stored Procedures page. When a new stored proc limitation is identified, it seems simplest to add the limitation to that main list. However, that list can't then be reused in the "new" section, because it displays all the KLs in history. So for this version, I manually pasted the same limitations from the includes into the "new" KLs section.
In contrast, v25.3 have common include files for "geospatial" or "rls" (row-level security), so those are one-off includes. It also doesn't seem useful to have the KLs within includes, since they're only being used in one place. This system is still relatively new and could use another look. I'm open to ideas.
In the meantime, do you think the temporary duplication here is acceptable, or should I remove the dupes in the v25.3 section and put them back inside the includes for next version (I guess I did do that work this time, too)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we feel the distinction is worth making (this is a newly-identified limitation vs. this is a limitation that we'd already identified in previous versions) then I think we should probably avoid the duplication, as it's sort of confusing. Having separate include files for old and new limitations could work for that, but also maybe there's a cleverer solution?
Then again, I don't think the duplication will have an effect beyond mild confusion for anyone (possibly no one) who notices it. It's clear in any case that the limitation still affects v25.4, so who cares when it was introduced. Unless... possibly it matters to someone who's considering upgrading to the newest version, and that known limitation is relevant to that decision. Maybe then it should be very clear that the limitation is genuinely new. But maybe that's an edge case.
I guess long term I'm anti-duplication but maybe a rethink of the include file situation is a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ... I completely forgot that I had already solved this problem when I came up with this system. Some of the include files have a conditional block for new KLs that prevents them from appearing when they're on the Known Limitations page (where they will already be at the top). For the next version, you just move them outside that conditional block.
This should be fixed now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah amazing, elegant solution
| - A `RECORD`-returning UDF cannot be created without a `RETURN` statement in the root block, which would restrict the wildcard type to a concrete one. [#122945](https://github.com/cockroachdb/cockroach/issues/122945) | ||
| - User-defined functions are not currently supported in: | ||
| - Expressions (column, index, constraint) in tables. [#87699](https://github.com/cockroachdb/cockroach/issues/87699) | ||
| - Partial index predicates. [#155488](https://github.com/cockroachdb/cockroach/issues/155488) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also want to include this one in a conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
DOC-15164
DOC-14898
DOC-14846
DOC-14842
DOC-14701
DOC-14568
DOC-14552
DOC-14129
DOC-13606
DOC-11780
DOC-11740
DOC-11739
DOC-11738
DOC-12328
DOC-12327