fix(sql-backend): change stream node type from json to binary to fix stack overflow #15040

yezizp2012 · 2024-02-06T17:05:23Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title. The stack overflow issue is caused accidentally when sea-orm is trying to deserialize column stream_node from table fragment. But when I tried deserializing the problematic data manually using serde_json, it did't show up. At present, it seems that only changing the type of this column can temporarily fix it.

changed stream_node type from json to binary to avoid some deserialization issues inside sea-orm.
some optimization to avoid returning the inserted model when it's not necessary.
bump sea-orm version.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Richard Chien <[email protected]>

) Signed-off-by: Bugen Zhao <[email protected]>

… with high barrier latency (#15011)

src/meta/model_v2/src/fragment.rs

src/ctl/Cargo.toml

kwannoel · 2024-02-07T07:55:17Z

src/meta/Cargo.toml

-serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+serde = { version = "1.0.196", features = ["derive"] }
+serde_json = "1.0.113"


Shall we revert the rest of the version upgrade as well?

The previous version was a bit outdated. The bump should have no harm, and it might be helpful for debugging stack overflow issue later. 🤔 I prefer to keep it.

The best practice should be only to bump the version in Cargo.lock through cargo update -p .., instead of making it a constraint in Cargo.toml.

BTW, the major motivation for introducing code owner for Cargo.lock is to avoid bumping dependency in a PR for other purposes.

The bump should have no harm

This is hard to say. Developers would review the change logs in the PRs opened by the dependency bot.

Indeed. I see, I should bump the version in a separate PR using cargo update -p instead of messing it with other logics.

The bump should have no harm

This is hard to say. Developers would review the change logs in the PRs opened by the dependency bot.

I have checked the change logs in sea-orm in order to find some clues to identify the issue. So it might be ok, since sea-orm is only used in meta and under testing. Won't do it next time. 😢

Btw, shall we remove all constraint in Cargo.toml and let them managed by Cargo.lock in a unified manner.

If we confirm that the issue only gets fixed after a minor version, then specifying the version as a requirement looks good. For serde, it would be more suitable to keep the version as 1 since it is a regular bump. 😄

src/meta/src/controller/catalog.rs

kwannoel

Generally LGTM.

wangrunji0408

rubber stamp for Cargo.lock

…stack overflow (#15040) Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Bugen Zhao <[email protected]> Co-authored-by: yezizp2012 <[email protected]> Co-authored-by: Li0k <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Richard Chien <[email protected]> Co-authored-by: Bugen Zhao <[email protected]> Co-authored-by: Dylan <[email protected]> Co-authored-by: Noel Kwan <[email protected]>

…stack overflow (#15040) (#15046)

src/meta/model_v2/src/fragment.rs

yezizp2012 added 2 commits February 6, 2024 23:56

fix: try to fix stack overflow in meta sql backend

d376e46

change stream node db type from json to binary

0676c0c

github-actions bot added the type/fix Bug fix label Feb 6, 2024

Fix "cargo-hakari"

2e77a0c

kwannoel self-requested a review February 7, 2024 06:15

yezizp2012 and others added 9 commits February 7, 2024 14:17

remove debug logs

f336df7

fix(test): fix case_when_optimization test (#15032)

8d61506

fix(storage): fix compactor oom 0206 (#15023)

d5b7b83

refactor(error): clean-up direct error formatting (part 5, done) (#14956

67ae88a

) Signed-off-by: Bugen Zhao <[email protected]>

feat(optimizer): support PullUpCorrelatedPredicateAggRule (#15026)

abc633e

fix(integration_tests): separate test for arrangement backfill cancel…

cc679ea

… with high barrier latency (#15011)

chore(bench): increase key count (#15002)

42dfa6f

Merge branch 'main' into zp/fix-meta-stack-overflow

013fb84

yezizp2012 marked this pull request as ready for review February 7, 2024 06:23

yezizp2012 requested a review from a team as a code owner February 7, 2024 06:23

yezizp2012 requested review from shanicky and zwang28 February 7, 2024 06:23

kwannoel reviewed Feb 7, 2024

View reviewed changes

src/meta/model_v2/src/fragment.rs Show resolved Hide resolved

BugenZhao reviewed Feb 7, 2024

View reviewed changes

src/ctl/Cargo.toml Outdated Show resolved Hide resolved

revert ctl serde version

1b2aa5c

kwannoel reviewed Feb 7, 2024

View reviewed changes

src/meta/src/controller/catalog.rs Show resolved Hide resolved

kwannoel approved these changes Feb 7, 2024

View reviewed changes

yezizp2012 enabled auto-merge February 7, 2024 08:11

yezizp2012 requested a review from BugenZhao February 7, 2024 08:11

yezizp2012 added the need-cherry-pick-release-1.7 label Feb 7, 2024

yezizp2012 requested a review from a team February 7, 2024 08:16

shanicky approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'main' into zp/fix-meta-stack-overflow

2bde1a9

wangrunji0408 approved these changes Feb 7, 2024

View reviewed changes

yezizp2012 added this pull request to the merge queue Feb 7, 2024

Merged via the queue into main with commit 05b84b4 Feb 7, 2024
27 of 28 checks passed

yezizp2012 deleted the zp/fix-meta-stack-overflow branch February 7, 2024 09:20

github-actions bot mentioned this pull request Feb 7, 2024

fix(sql-backend): change stream node type from json to binary to fix stack overflow (#15040) #15046

Merged

yezizp2012 pushed a commit that referenced this pull request Feb 7, 2024

fix(sql-backend): change stream node type from json to binary to fix …

c1e86d7

…stack overflow (#15040) (#15046)

xxchan reviewed Feb 7, 2024

View reviewed changes

src/meta/model_v2/src/fragment.rs Show resolved Hide resolved

yezizp2012 mentioned this pull request Feb 7, 2024

feat(sql-backend): revert stream_node column to json type in table fragment #15051

Closed

st1page mentioned this pull request Feb 8, 2024

2024-02-07 nexmark performance degradation #15054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql-backend): change stream node type from json to binary to fix stack overflow #15040

fix(sql-backend): change stream node type from json to binary to fix stack overflow #15040

yezizp2012 commented Feb 6, 2024 •

edited

Loading

kwannoel Feb 7, 2024

yezizp2012 Feb 7, 2024

BugenZhao Feb 7, 2024 •

edited

Loading

BugenZhao Feb 7, 2024

yezizp2012 Feb 7, 2024

BugenZhao Feb 7, 2024

kwannoel left a comment

wangrunji0408 left a comment

fix(sql-backend): change stream node type from json to binary to fix stack overflow #15040

fix(sql-backend): change stream node type from json to binary to fix stack overflow #15040

Conversation

yezizp2012 commented Feb 6, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

kwannoel Feb 7, 2024

Choose a reason for hiding this comment

yezizp2012 Feb 7, 2024

Choose a reason for hiding this comment

BugenZhao Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

BugenZhao Feb 7, 2024

Choose a reason for hiding this comment

yezizp2012 Feb 7, 2024

Choose a reason for hiding this comment

BugenZhao Feb 7, 2024

Choose a reason for hiding this comment

kwannoel left a comment

Choose a reason for hiding this comment

wangrunji0408 left a comment

Choose a reason for hiding this comment

yezizp2012 commented Feb 6, 2024 •

edited

Loading

BugenZhao Feb 7, 2024 •

edited

Loading