adapter: optimizer and dyncfg cleanups for OCC read-then-write

[aljoscha]

Motivation

Part 2 of 7 in a stack that moves DELETE, UPDATE and INSERT ... SELECT
off the coordinator onto the session task, using optimistic concurrency
control.

Two independent cleanups that the later parts need, both of which stand on
their own. The first is a live bug.

Closes SQL-588

Description

ConfigSet sync at catalog open. apply_updates only mirrors
SystemVars into the dyncfg ConfigSet when a durable
SystemConfiguration update is present. A deployment that configures a
parameter purely through system_parameter_default has no such update, so
nothing syncs, and anything reading that dyncfg during bootstrap sees the
compile-time default instead of the configured value. ENABLE_EXPRESSION_CACHE
is read a few lines below and is affected today. Syncing the effective
values once, after both the defaults and the durable configuration are in
the map, fixes every such read.

One subscribe optimization pipeline. Subscribe optimization had two
entry points that differed only in whether the source was a query or an
existing id, and each rebuilt the same pipeline around that one decision.
Both rebuilt the same SinkDesc verbatim, down to the same three comments,
and differed only in whether the source needed HIR lowering first. Taking the source as a SubscribeSource and the output shape as a
parameter leaves the pipeline in one place. The view optimizer's
expression-preparation variants collapse the same way, into one constructor
plus a builder method for the constant folding limit.

The dyncfg commit also splits dyncfg_updates, which computed the updates
and applied them to its own ConfigSet in one method. The only two callers
of the applying behavior are the ones that discard the result, so the call
sites read like a getter whose value is thrown away, and a later change
making the getter pure would compile cleanly while silently reintroducing
the bug. sync_dyncfgs names that intent. The six callers that forward the
updates to metrics or the controllers keep the pure dyncfg_updates.

Verification

Existing subscribe and COPY FROM coverage. No behavior change is intended
for the optimizer part. The dyncfg change is observable as
ENABLE_EXPRESSION_CACHE now taking effect when set only via
system_parameter_default.

[linear-code]

SQL-588

[ggevay]

LGTM, some minor comments.

Claude: Two more things beyond the inline comments:

1. Could the dyncfg fix get a regression test pinning the sync? For example: open a debug catalog with a system_parameter_default that sets some dyncfg to a non-default value, then assert state.system_config().dyncfgs() reflects it. The original bug stayed invisible because nothing asserts this, and such a test is also the only thing that would notice if dyncfg_updates() ever lost its apply side effect.

2. One question on the PR description: I could not find the drift it refers to ("one carried the reasoning about why the sink's as_of and the dataflow's until are set the way they are, the other had lost it"). At the merge base both match arms carry identical comments, and the as_of/until reasoning lives once in resolve(), which this PR does not touch. Were you thinking of the sequencing layer? There is real drift there: the coordinator path emits the EqualSubscribeBounds notice when as_of == up_to and the frontend path does not. We filed that separately as SQL-599, out of scope for this PR. Either way, worth a small tweak to the description so future archaeology does not hunt for a comment that never existed.

[ggevay]

nit: This is not really optimizing the "source". I'd name this optimize_inner.

[ggevay]

Looks a bit surprising that this getter has a side effect. Maybe this could be refactored somehow. (Claude on this in the below thread.)

[ggevay]

This line works because dyncfg_updates() applies the computed updates to the SystemVars' own ConfigSet as a side effect (updates.apply(&self.dyncfgs) at the end of the method). From the call site it reads like a discarded getter result, and a future "make this getter pure" refactor would compile cleanly and silently turn this line and the twin in apply_updates into no-ops, reintroducing the bug.

Consider splitting it: keep dyncfg_updates() pure, add a sync_dyncfgs() that computes, applies, and returns. Only this site and apply_updates need the applying variant. Nice side effect of your fix: those two are now sufficient (sync at open, re-sync on every durable system-config change), so the bootstrap-time calls in coord.rs no longer depend on their accidental sync.

[ggevay]

Tiny one: LirDataflowDescription becomes pub without a consumer in this PR (verified: reverting the pub compiles cleanly on this branch). Part 5 names it, so I assume intentional groundwork. Fine as is, just confirming it is deliberate.

[aljoscha]

(This is aj, Aljoscha's agent, acting on his behalf. He's away, so replies here
are mine and he'll review when he's back.)

Thanks, all four taken. The dyncfg_updates split is the best catch in here.

Split done. dyncfg_updates is now pure and a new sync_dyncfgs computes,
applies and returns. Only catalog/open.rs and catalog/apply.rs use the
applying variant. I checked the thing your comment asserts but doesn't show,
since it's what would have made this unsafe: none of the six remaining callers
(coord.rs ×2, flags.rs ×2, ddl.rs ×2) depends on the side effect. Each one
forwards the returned ConfigUpdates to metrics or the controllers and never
needs this process's ConfigSet mutated. So your "those two are now sufficient"
holds, and the pure getter is safe for everyone else.

You're right about the description, and the claim was simply false. I
checked at the merge base: both match arms carry identical // Make SinkDesc,
// No FORCE NOT NULL and // No REFRESH comments, the Query arm's extra
comments are about HIR to MIR lowering, and the as_of/until reasoning lives
once in resolve(), which this PR does not touch. There was no lost reasoning.
The real argument is that both arms rebuilt the same SinkDesc verbatim and
differed only in whether the source needed lowering first. Description rewritten
to say that. Thanks for pushing on it rather than letting it stand.

optimize_source renamed to optimize_inner, and you were right that the
old name was wrong: it optimizes the whole subscribe given a source, it does not
optimize the source. Worth noting the rename reached a call site added later in
the stack, in part 6, so that commit is amended too.

On the regression test, I'd push back slightly. The fix does have coverage,
just not in this PR. coord.rs reads
FRONTEND_READ_THEN_WRITE.get(catalog.system_config().dyncfgs()), so if the
sync broke, the flag would read its compile-time false even with
system_parameter_default setting it true, and the ~22 OCC tests in part 6 that
enable it that way would all fail. So it's not that nothing asserts this, it's
that the assertion lands a few PRs later.

I'd also argue the split addresses your underlying worry better than a test
would: it removes the trap rather than detecting it after the fact. A direct
unit test is still possible, but there's no existing debug-catalog-plus-
system_parameter_default harness, so it's real setup for coverage we already
have. Happy to add one if you'd still prefer it, and equally happy to be told
I'm wrong here.

LirDataflowDescription is deliberate, confirmed. Part 5 is the consumer.