Optimizer fast path

[ggevay]

Edit: I've done some benchmarking in release mode now, and the performance improvement in release mode is quite small (see here), so I'm now not sure whether the complexity that this PR is introducing is actually worth it. We might want to hold off on merging this, so taking back to draft.

Edit 2: Now with @antiguru's other improvements, the speedup from this might be worth it. So marking ready for review.

This PR is about running only a tiny subset of the optimizer for fast path queries. The approach is that when we are about to call optimize_dataflow, we quickly check whether we already have a simple enough plan that it would go to the fast path. If yes, then we call a special, tiny optimizer instead of the whole thing.

Commits:

1. Trivial comment fixes.
2. This is not strictly necessary, but makes the optimizer more robust. See commit msg.
3. The main point of the PR.
4. At the point of the peek optimization where I added the new call to create_fast_path_plan, we haven't yet eliminated mz_now(), and create_fast_path_plan might choke on that. This commit makes us ignore this error and just not take the fast path optimizer.

There is a tricky reason for the change in src/environmentd/tests/testdata/http/ws, I'm investigating the details now. It's probably not an issue, since the actual fast path plan that we arrive at is the same, just the non-fast-path plan is slightly different, but even there the physical plan would be the same, the diff is only in the MIR plan. Edit: Figured out exactly what's happening: The reason for the diff is that in the normal optimizer pipeline we call ProjectionPushdown, which puts the MFP in a non-canonical state (Project at the bottom), which is usually canonicalized by CanonicalizeMfp, but here CanonicalizeMfp is not able to put back the Project at the top for this query, because it calls extract_non_errors_from_expr_mut, which gives up when it sees the error in this MFP. But this doesn't matter because create_fast_path_plan then calls a different MFP extraction method: extract_from_expression, so the fast path plan will have the same MFP as before. (And ProjectionPushdown is not doing anything useful here, because MFP optimization will take care of the projections.)

Motivation

https://materializeinc.slack.com/archives/C0761MZ3QD9/p1722545887422389?thread_ts=1722544237.348389&cid=C0761MZ3QD9

In my simplistic preliminary benchmarks (running a SELECT with a simple literal constraint on an indexed view locally in debug mode), I'm seeing a run time reduction from 20ms to 11ms. Edit: The difference was smaller in release mode. Edit 2: We should benchmark again, now that other parts of the system have been optimized in the meantime.

Tips for reviewer

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • Kicked off a limited Nightly: https://buildkite.com/materialize/nightly/builds/8931
• This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
• If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
• If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
• This PR includes the following user-facing behavior changes:

[shepherdlybot]

Mitigations

Completing required mitigations increases Resilience Coverage.

• (Required) Code Review 🔍 Detected
• Feature Flag
• Integration Test
• Observability
• QA Review
• Run Nightly Tests
• Unit Test

Risk Summary:

The pull request has a moderate risk score of 73, driven by predictors such as the number of files modified, the length of the PR description, the number of function declarations within files, and the delta of statements. Historically, PRs with these predictors are 55% more likely to cause a bug than the repository baseline. Additionally, the observed bug trend in the repository is decreasing.

Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity.

[jkosh44]

Adapter code LGTM

[jkosh44]

Is this test useful for anyone? I have no idea what it was testing, what's changed in it, or if it's correct.

[ggevay]

See the last paragraph of the PR description.

[ggevay]

Bringing it out from draft, because, due to @antiguru's other improvements, the speedup from this might be worth it now.

Rebased on main.

I've split off the first 2 commits ("Some trivial comment fixes" and "Make LiteralConstraints specially handle empty possible_vals") into a different PR: #28748

[antiguru]

I think this looks great, and it has a positive performance impact. It reduces the average query latency. As expected, the tail latency stays largely unchanged.

[antiguru]

It might be a bit wasteful to create the fast path plan here and then run through the optimization pipeline again? Especially when we already here determine it's a constant or simple peek, we could probably use the result instead of running the optimizer at all? I'm fine with merging as-is, but we should take note.

[ggevay]

run through the optimization pipeline again

Do you mean "again", in the sense that we have already run the optimize_mir_local optimizer above? Running optimize_dataflow below still does meaningful things, i.e., not all the things that we want to happen for fast path peeks are done in optimize_mir_local. The most important thing that we still need to do is LiteralConstraints, which can turn a full index scan into an index lookup. (And then we need CanonicalizeMfp again, because LiteralConstraints can leave the MFP in a non-canonical form. And we also need FoldConstants again, because LiteralConstraints might have got us a constant, if it found contradicting literal constraints. And ReduceScalars is just good to run all the time for various obscure reasons, see e.g. the last paragraph of 3. here.)

(Note that create_fast_path_plan doesn't run any optimizations. It should be a cheaper thing than even the fast_path_optimizer.)

A possible improvement that we could still do here is to not even run the optimize_mir_local for peeks that look like they are gonna be fast path, but just run fast_path_optimizer on the initial MIR plan. I tried to do this when originally working on this PR, and ran into some minor troubles, which I can't recall at the moment (maybe it was just that it would have required a somewhat larger refactoring of sequencing), so I just decided to keep this PR simple. But if the time comes when the optimizer run time becomes a significant portion of the run time of fast path peeks again, due to optimizations in other parts of the system, then I'm happy to revisit this. (I think I did some measurements with just hackily commenting out optimize_mir_local, and it showed that the run time of optimize_mir_local is significantly lower than the full optimizer pipeline, which this PR is eliminating.)

[ggevay]

I've created an issue for eliminating optimize_mir_local from fast path peeks, so that this doesn't get forgotten: https://github.com/MaterializeInc/materialize/issues/29501