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

Server/Tests: Introduce CockroachDB Simple Query tests #8839

Closed
2 tasks done
Tracked by #8788
sassela opened this issue Aug 24, 2022 · 6 comments
Closed
2 tasks done
Tracked by #8788

Server/Tests: Introduce CockroachDB Simple Query tests #8839

sassela opened this issue Aug 24, 2022 · 6 comments
Assignees
Labels
c/tests 🧪 Related to testing GraphQL engine cockroachdb t/native-dbs

Comments

@sassela
Copy link
Contributor

sassela commented Aug 24, 2022

This issue involves using the CockroachDB test harness in the Haskell integration test suite.

  • including that test fixture in the Schema/* and Queries/Simple specs
  • creating issues for any specs that fail, and linking to them here. We can coordinate work with the CockroachDB team.
@sassela sassela added c/tests 🧪 Related to testing GraphQL engine cockroachdb labels Aug 24, 2022
@sassela
Copy link
Contributor Author

sassela commented Aug 24, 2022

depends on #8799

@seanparkross seanparkross changed the title server/tests: introduce CockroachDB Simple Query tests Introduce CockroachDB Simple Query tests Aug 25, 2022
@robertjdominguez robertjdominguez changed the title Introduce CockroachDB Simple Query tests Server/Tests: Introduce CockroachDB Simple Query tests Aug 25, 2022
@danieljharvey
Copy link
Contributor

danieljharvey commented Sep 5, 2022

Have added all the tests. So far they pass if:

a) We use our HGE hotfix that stops us sending prepared arguments (as cockroach is still strict about being sent unused prepared arguments)

b) We enable stringifyNumbers for the tests, like we do with BigQuery, as integers from Cockroach are coming back as strings. Edit: this is expected behaviour: https://www.cockroachlabs.com/docs/stable/sql-faqs.html#why-are-my-int-columns-returned-as-strings-in-javascript

@sassela
Copy link
Contributor Author

sassela commented Sep 6, 2022

@danieljharvey is this actually blocked, then? Or, assuming the prepared args hotfix is already on main, could we not merge the new tests onto main?

@danieljharvey
Copy link
Contributor

The prepared args for metadata are fixed and in main but the fix for regular queries is "just don't send the user metadata" which can't be merged because it unblocks console work but ruins any query that uses permissions: https://github.com/hasura/graphql-engine-mono/pull/5592

The Simple Query tests are done though, have added https://github.com/hasura/graphql-engine-mono/pull/5773 so that they can be merged behind a flag and then switched on once prepared args is fixed.

@sassela
Copy link
Contributor Author

sassela commented Sep 7, 2022

Gotcha, so confirming here that cockroachdb/cockroach#86375, specifically a change to the wire protocol prepare, is the only thing blocking us from enabling those tests.

craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Sep 19, 2022
86603: changefeedccl: redact user from confluent schema registry r=jayshrivastava a=jayshrivastava

This change redacts the confluent schema registry key
from the jobs table.

Fixes: #85902

Release justification: This change is important because
it prevents a user's secret key from being store in the DB.
The footprint of this change is small as it only touches
a specific changefeed option - confluent schema registry.

Release note (enterprise change): Previously, SHOW CHANGEFEED
JOBS would reveal confluent schema registry user information,
including a user's secret key. This change makes this info
redacted, meaning it will not be stored in CockroachDB
internal tables at all.

87166: kvserver: shorten `raft.process.handleready.latency` help text r=tbg a=erikgrinaker

We should get confirmation in #87112 that this size is below the limit before merging this.

---

AWS managed Prometheus rejects `raft.process.handleready.latency`
because the help text is too long. The text is currently 1123 bytes, so
the limit is suspected to be 1024 bytes. This patch reduces the size of
this help text to 938 bytes.

Resolves #87112.

Release justification: bug fixes and low-risk updates to new functionality

Release note (ops change): Reduced the length of the
`raft.process.handleready.latency` metric help text to avoid it being
rejected by certain Prometheus services.

87535: sql: support `#typeHints` greater than `#placeholders` for prepare stmt r=rafiss a=ZhouXing19

Previous, we only support pgwire prepare stmt with the number of typehints equal or smaller than the number of the placeholders in the query. E.g. the following usage are not supported:

```
Parse {"Name": "s2", "Query": "select $1", "ParameterOIDs":[1043, 1043, 1043]}
```
Where there is 1 placeholder in the query, but 3 type hints.

This commit is to allow mismatching #typeHints and #placeholders. The former can be larger than the latter now.

This feature is needed for [CRDB's support for Hasura GraphQL Engine](hasura/graphql-engine#8839 (comment)).

Release justification: Low risk, high benefit changes to existing functionality

Release note: For pgwire-level prepare statements, add support for the case where the number of the type hints is greater than the number of placeholders in the given query.

87870: kvnemesis: reset op.Result r=erikgrinaker a=tbg

We found out (in #87814) after a wild goose chase that op.Result was not
reset, so unless operations were cognizant of this fact and
always fully repopulated the Result field, weird failures
could result from txn retries.

Reset the field.

Release note: None


88078: ui: update filter labels r=maryliag a=maryliag

Update filter label from "App" to "Application Name" and "Username" to "User Name" on SQL Activity and Insights pages.

Fixes #87960

<img width="467" alt="Screen Shot 2022-09-16 at 6 40 51 PM" src="https://user-images.githubusercontent.com/1017486/190827281-36a9c6df-3e16-4689-bcae-6649b1c2ff86.png">


Release note (ui change): Update filter labels from "App" to "Application Name" and from "Username" to "User Name" on SQL Activity and Insights pages.

88111: sql: fix `pg_get_viewdef` for materialized views r=rafiss a=knz

Fixes #88109.

Release note (bug fix): The function `pg_catalog.pg_get_viewdef` now works properly for materialized views.

Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
@danieljharvey
Copy link
Contributor

All tests have been added to the suite, some fail but have been commented out. These are the tickets to track those issues:

https://github.com/hasura/graphql-engine-mono/issues/5987
https://github.com/hasura/graphql-engine-mono/issues/5985
cockroachdb/cockroach#88355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/tests 🧪 Related to testing GraphQL engine cockroachdb t/native-dbs
Projects
None yet
Development

No branches or pull requests

3 participants