sql: create plpgsql grammar and unimplemented errors #91569
Assignees
Labels
A-sql-pgcompat
Semantic compatibility with PostgreSQL
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
X-anchored-telemetry
The issue number is anchored by telemetry references.
Comments
awoods187
added
C-enhancement
|
@chengxiong-ruan had a prototype of the grammar from a breather week somewhere. |
blathers-crl
bot
added
the
T-sql-foundations
rafiss
added
the
X-anchored-telemetry
|
#96720 is a simpler way that would let us start seeing plpgsql function bodies in our telemetry data, without implementing the plpgsql grammar. I don't think there's too much of an added advantage to making unimplemented errors that are specific to plpgsql. But we should leave this issue open to track implementing the grammar. |
|
Hey @rafiss, @awoods187: Just wanted to confirm that the scope for this is to only add the grammar without the actual functionality? cc @e-mbrown |
|
Correct--the main reason to accelerate this work (and backport it to 22.2.x) is that it will let us have more specific unimplemented errors for plpgsql so that we can better prioritize what to build. We don't know what type of attempt at plpgsql was done and cant do automatic comparisons on missing feature by frequency. |
craig bot
pushed a commit
that referenced
this issue
Apr 13, 2023
87259: sql: change index field visible to invisibility r=rytaft a=wenyihu6 This commit changes the field NotVisible to visibility in the struct IndexDescriptor. All existing behavior should stay the same. Invisibility specifies the invisibility of an index to the optimizer and can be any float64 between [0.0, 1.0]. An index with invisibility 0.0 means that the index is visible. An index with invisibility 1.0 means that the index is fully not visible. By default, an index should be visible or invisibility 0.0. An index with invisibility 1.0 is ignored by the optimizer unless it is used for constraint check or is explicitly selected with index hinting. An index with invisibility between (0.0, 1.0) would be made fully not visible to a corresponding fraction of the queries (this will be supported in a future commit). By convention, we will refer any indexes with invisibility == 0.0 as visible, any indexes with invisibility == 1.0 as fully not visible, and any indexes with index visibility in-between as partially not visible. Visibility is not chosen here because we want the default value to be 0.0 (by default, index should be visible). NotVisibility is not chosen here because it sounds awkward. Informs: #82363 Release note: None 98147: sql: add plpgsql parser for telemetry purposes r=e-mbrown a=e-mbrown Informs: #91569 This commit adds the in use portions of the plpgsql parser from #91606. Release note: None 101188: ui: send request state to component, process data in components r=xinhaoz a=xinhaoz We should not decouple the data from a fetch request and fields describing the request/response like `inFlight`, `valid`, `lastUpdated` which are stored in redux, when passing them as props into the component. In traditional redux apps we we would be able to select the request state as a whole unit, getting fields like `isLoading`, `error` and etc with the data portion of the payload. Unfortunately, with the our split redux stores in db-console and CC, we were not able to select directly from the stores. Instead, we have been following a pattern of creating selectors for each required part of the request field (e.g. `selectData`, `selectError`, `selectDataIsValid`) for each cached request, having to create a copy of each selector in both db-console and cluster-ui. These selectors are then used to pass each field independently as a prop. One can see how this boilerplate code can quickly multiply with the number of new requests being created. This pattern was probably a mistake -- we use selectors to memoize computations when we want to transform data from the redux store. For modern React we have built-in utilities like `useMemo` that can accomplish this within the component. For many fields currently using `createSelector`, like `isLoading`, the logic is extremely simple, where the field is directly read and returned with no computation. This means caching with `createSelector`in most cases is unnecessary, so we have a lot of boilerplate with no useful effects. In addition, the processing of some of the data in each prop selectors (db-console and cluster-ui) means we have code that gets duplicated. This is hard to maintain and can become confusing as the prop may even go through more processing when inside the component. We should adopt a pattern of sending the entire request state for cached data fetches to the components reading them. This commit introduces the `RequestState<T>>` type which going forward should be used as the redux field type for all cached data fetches in cluster-ui, and be prop type used by components that use the response. This commit shows an example of what can be done with the statements page and sql stats response when we adopt this pattern. Note that for class components, where `useMemo` is not available, we can just use redux's selectors/reselect to help memoize when necessary. Epic: none Release note: None -------------------- DB-Console: https://www.loom.com/share/2dbc66751fce49aea25b5f341d26e76a CC: https://www.loom.com/share/3cd57711da2441eaab9fbf593607c03d 101431: pgwire: fix a data race in pre-serve error handling r=rafiss,stevendanna a=knz Fixes #101430. Prior to this patch, `PreServeConnHandler` used a single `errWriter` shared across all connections. This is incorrect because if two or more connections simultaneously encounter a pre-serve error, they would stomp on each other's data in the error writer. The fix is to make a new write buffer on each error, like what is already done in `pgwire.Server.sendErr()`. This bug was introduced when `PreServeConnHandler` was first implemented, back in 1814c21. Release note: None Epic: CRDB-23559 101446: sql/rowcontainer: skip flaky TestDiskBackedIndexedRowContainer subtest r=mgartner a=mgartner Informs #101326 Epic: None Release note: None Co-authored-by: Rebecca Taft <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]> Co-authored-by: Jane Xing <[email protected]> Co-authored-by: rharding6373 <[email protected]> Co-authored-by: e-mbrown <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Marcus Gartner <[email protected]>
chengxiong-ruan
added a commit
to ZhouXing19/cockroach
that referenced
this issue
Apr 13, 2023
Informs: cockroachdb#91569 This commit adds parser support for stmt_for statement with some minor refactoring on interfaces and common types. Release note: None
This was referenced Apr 28, 2023
This was referenced May 10, 2023
This was referenced May 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As we explore support for plpgsql, we should add a grammar file and unimplemented errors for the corresponding features. With grammar and unimplemented support, we can better understand the implementation sequencing (and it will be necessary for implementation in the future anyway)
Much of the initial work is completed as part of #91606 so we should follow up from there. See this Slack for more details.
Jira issue: CRDB-21320
Epic CRDB-799
The text was updated successfully, but these errors were encountered: