adapter: save system_parameter_defaults to disk
#25924
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nrainer-materialize
approved these changes
Mar 11, 2024
aalexandrov
force-pushed
the
eq_prop_fix_ci
branch
2 times, most recently
from
4fbe944 to
28dbfe7
Compare
Change the `testdrive` and `legacy-upgrad` mzcompose scripts to run
```sql
ALTER SYSTEM SET {key} = '{value}'
```
for each (key, value) pair from the `system_parameter_defaults`
associated with a `Materialized` service. This is needed in order to
force these values durably to disk and avoid the failures seen in
BuildKite runs like https://buildkite.com/materialize/tests/builds/77990.
…lue" This reverts commit f70b2946a8108bed6830ce032e6147a32c732752.
aalexandrov
force-pushed
the
eq_prop_fix_ci
branch
from
28dbfe7 to
a5d1131
Compare
|
I think we should create a new argument for |
aalexandrov
commented
Mar 11, 2024
| else: | ||
| c.up("materialized") | ||
|
|
||
| for key, value in DEFAULT_SYSTEM_PARAMETERS.items(): |
There was a problem hiding this comment.
The CI and also my local testdrive run
bin/mzcompose --find testdrive run default array.tdstill fail the consistency check, even though I'm now seeing an
ALTER SYSTEM SET enable_connection_validation_syntax = truecorresponding to this call when I run the test locally. However with this line added directly to array.td the test passes.
$ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr}
ALTER SYSTEM SET enable_connection_validation_syntax = true;
5 tasks
5 tasks
|
Closing in favor of #25950. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Save
system_parameter_defaultsto disk and revert back the hard-coded default forenable_equivalence_propagationtofalse.Motivation
This addresses an issue observed while trying to merge #24155.
More specifically, since
system_parameter_defaultsare not persisted to disk, Testdrive tests that run consistency checks re-hydrate the in-memory catalog without respecting the feature flags defined inDEFAULT_SYSTEM_PARAMETERSand consequently fail with the following error:For example, see this Buildkite run.
We will always hit these issues when introducing a feature flag that affects the optimizer pipeline across the board, so it's better to come up with a more general solution than "add
ALTER SYSTEM SET <optimizer feature> = trueto all affected*.tdfiles".Tips for reviewer
If this causes issues outside of our Testdrive tests (in other CI tests or in production, I suggest to parameterize the behavior of the
system_parameter_defaultsloop inload_system_configurationby another boolean parameterpersist_system_parameter_defaults: bool.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.