fix(meta): make drop streaming job catalog and notify frontend transactional

[kwannoel]

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

What's changed and what's your intention?

As mentioned: #17576 (comment).
The following sequence of events can occur:

1. MV with id 2000 failed after its catalogs were initially committed.
2. FE killed.
3. CN killed
4. From (3), Finish stream job fails.
5. From (2) FE restarted, subscribe to meta.
6. From (3), Meta cleans up catalogs, removing id=2000.
7. From (2) Meta notifies the newly subscribed frontend node of the existing catalogs. This does not include 2000 since it's already cleaned.
8. From (3), Meta issues a clean stream job command for 2000, but it's not in the FE catalog at all, only tables from (7) are inside. So FE will just panic when trying to remove this stream job's table.

The solution is just to make sure drop streaming job catalog and fe notify happens at the same time.

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.

[yezizp2012]

From (3), Meta issues a clean stream job command for 2000, but it's not in the FE catalog at all, only tables from (7) are inside. So FE will just panic when trying to remove this stream job's table.

IIUC, meta always notify catalog changes to FE while persisting catalog in metastore, which is mutually exclusive with the read of snapshot for subscribe. 🤔 And we always unwrap the operation of catalog changes in FE is to verify the mechanism of notification, we'd better not change it.

[kwannoel]

From (3), Meta issues a clean stream job command for 2000, but it's not in the FE catalog at all, only tables from (7) are inside. So FE will just panic when trying to remove this stream job's table.

IIUC, meta always notify catalog changes to FE while persisting catalog in metastore, which is mutually exclusive with the read of snapshot for subscribe. 🤔 And we always unwrap the operation of catalog changes in FE is to verify the mechanism of notification, we'd better not change it.

Yes it is exclusive originally. But after #17484, we can have:

1. fe receives and updates catalog snapshot on subscribe.
2. fe also receives delete notification for a dropped stream job. The table id may not be in the snapshot of 1. You can see the PR description for how this can happen. See https://buildkite.com/risingwavelabs/main-cron/builds/2788#01907e7a-280b-4057-8152-4c438253e8ce for the failing test.

[kwannoel]

And we always unwrap the operation of catalog changes in FE is to verify the mechanism of notification, we'd better not change it.

I don't mind keeping it, but haven't thought of a better way to solve this problem yet, since deleting a catalog in meta and FE receiving and processing the notification is not transactional.

[yezizp2012]

Yes it is exclusive originally. But after #17484, we can have:

1. fe receives and updates catalog snapshot on subscribe.
2. fe also receives delete notification for a dropped stream job. The table id may not be in the snapshot of 1. You can see the PR description for how this can happen. See buildkite.com/risingwavelabs/main-cron/builds/2788 for the failing test.

Does it caused by non-transactional of cancel_create_materialized_view_procedure?

I don't mind keeping it, but haven't thought of a better way to solve this problem yet, since deleting a catalog in meta and FE receiving and processing the notification is not transactional.

If so, can we make it transactional like start_create_materialized_view_procedure? We can release the core lock after notification.

[kwannoel]

If so, can we make it transactional like start_create_materialized_view_procedure? We can release the core lock after notification.

Actually yes, that could work.

[chenzl25]

Why do we need to call self.core.lock() two times separately in cancel_create_materialized_view_procedure?

[kwannoel]

Hmm not sure either. I just tried unifying it in one block let's see if the tests pass.

[kwannoel]

If so, can we make it transactional like start_create_materialized_view_procedure? We can release the core lock after notification.

Actually yes, that could work.

It works.

[yezizp2012]

LGTM!