Skip to content

Commit f3f518a

Browse files
committed
clusterversion: update M.4 runbook with onlyif directive guidance
Add expanded documentation to the M.4 runbook based on lessons learned: 1. Enhanced explanation in Commit 7 section about what onlyif/skipif directives do and how to properly remove them: - Explains that directives GUARD the next statement(s) - Provides concrete example from udf_in_table showing wrong vs right - Adds pattern: remove BOTH directive AND guarded statement - Includes manual review steps after sed commands 2. New Error 6 in Common Errors section documenting 'relation already exists' failures: - Root cause: removing directive but not guarded statement - Identification steps using git diff - Fix steps with example from udf_in_table - Prevention tips These errors were encountered during PR #157767 and should help future quarterly M.4 tasks avoid the same issues. Release note: None
1 parent 455ea99 commit f3f518a

File tree

1 file changed

+285
-13
lines changed

1 file changed

+285
-13
lines changed

pkg/clusterversion/CLAUDE.md

Lines changed: 285 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2957,25 +2957,31 @@ Mark old version keys for future removal by prefixing them with `TODO_Delete_`.
29572957
29582958
**File:** `pkg/clusterversion/cockroach_versions.go`
29592959
2960-
**Find all version keys below the new MinSupported** (e.g., all V25_2* keys when bumping to V25_3):
2960+
**⚠️ IMPORTANT: DO NOT prefix the final release key (e.g., V25_2)**
2961+
2962+
The final release key (e.g., `V25_2`) is special and should NOT be prefixed with `TODO_Delete_` even when it becomes the old MinSupported version. Only prefix the internal version keys (e.g., `V25_2_Start`, `V25_2_AddSystemStatementHintsTable`).
2963+
2964+
**Why:** The final release keys are referenced in other parts of the codebase and tooling, and they serve as anchors for release tracking.
2965+
2966+
**Find all INTERNAL version keys below the new MinSupported** (e.g., all V25_2_* keys when bumping to V25_3):
29612967
29622968
```bash
29632969
# Example for bumping MinSupported from V25_2 to V25_3
2964-
# Find all V25_2 version keys:
2965-
grep -n "^\s*V25_2" pkg/clusterversion/cockroach_versions.go
2970+
# Find all V25_2_* version keys (NOT V25_2 itself):
2971+
grep -n "^\s*V25_2_" pkg/clusterversion/cockroach_versions.go
29662972
```
29672973
29682974
**Add TODO_Delete_ prefix:**
29692975
```go
29702976
// Before:
29712977
V25_2_Start
29722978
V25_2_AddSystemStatementHintsTable
2973-
V25_2
2979+
V25_2 // ← DO NOT PREFIX THIS ONE!
29742980
29752981
// After:
29762982
TODO_Delete_V25_2_Start
29772983
TODO_Delete_V25_2_AddSystemStatementHintsTable
2978-
TODO_Delete_V25_2
2984+
V25_2 // ← Keep as-is, no TODO_Delete_ prefix
29792985
```
29802986
29812987
**Create commit:**
@@ -3026,10 +3032,39 @@ git grep -l "MinSupported" pkg/ | grep -v "_test.go" | grep -v "CLAUDE.md"
30263032
- `pkg/sql/create_index.go`
30273033
- `pkg/sql/create_table.go`
30283034
- `pkg/sql/distsql_running.go`
3029-
- `pkg/sql/execversion/version.go`
3035+
- **`pkg/storage/pebble.go`** - Update `MinimumSupportedFormatVersion` constant
3036+
3037+
**⚠️ IMPORTANT: DO NOT modify `pkg/sql/execversion/version.go`**
3038+
3039+
This file uses a separate versioning mechanism and is owned by the **SQL Queries team**. It should NOT be modified as part of the M.4 MinSupported bump task. If you see version references in this file during your search, leave them unchanged.
30303040
30313041
**Update each file** by changing references from the old MinSupported version to the new one.
30323042
3043+
**CRITICAL: Update pkg/storage/pebble.go (around line 2459):**
3044+
3045+
When bumping MinSupported, you must also update the `MinimumSupportedFormatVersion` constant to match the Pebble format version for the new MinSupported version.
3046+
3047+
**How to determine the correct value:**
3048+
1. Look at `pebbleFormatVersionMap` in the same file (around line 2450)
3049+
2. Find the entry for the new MinSupported version (e.g., `clusterversion.V25_3`)
3050+
3. Use that Pebble format version for `MinimumSupportedFormatVersion`
3051+
3052+
**Example for bumping MinSupported from V25_2 to V25_3:**
3053+
```go
3054+
// pebbleFormatVersionMap shows:
3055+
// V25_3: pebble.FormatValueSeparation
3056+
// V25_2: pebble.FormatTableFormatV6 (being removed)
3057+
3058+
// Therefore, update MinimumSupportedFormatVersion:
3059+
-const MinimumSupportedFormatVersion = pebble.FormatTableFormatV6
3060+
+const MinimumSupportedFormatVersion = pebble.FormatValueSeparation
3061+
```
3062+
3063+
**Why this is important:**
3064+
- The test `TestMinimumSupportedFormatVersion` validates: `MinimumSupportedFormatVersion == pebbleFormatVersionMap[MinSupported]`
3065+
- If you forget this change, the test will fail showing: `expected: 0xNN, actual: 0xMM`
3066+
- This ensures new stores are created with the correct minimum Pebble format version
3067+
30333068
**Example pattern to search for:**
30343069
```bash
30353070
# Find files with V25_2 that aren't in testdata
@@ -3038,8 +3073,8 @@ git grep "V25_2" pkg/ | grep -v testdata | grep -v CLAUDE.md | grep -v "_test.go
30383073
30393074
**Create commit:**
30403075
```bash
3041-
git add pkg/clusterversion/cockroach_versions.go <other-modified-files>
3042-
git commit -m "clusterversion: bump MinSupported from v25.2 to v25.3
3076+
git add pkg/clusterversion/cockroach_versions.go pkg/storage/pebble.go <other-modified-files>
3077+
git commit -m "clusterversion, storage: bump MinSupported from v25.2 to v25.3
30433078
30443079
Part of the quarterly M.4 \"Bump MinSupported\" task as outlined in
30453080
\`pkg/clusterversion/README.md\`.
@@ -3059,6 +3094,7 @@ Changes include updates to:
30593094
- Catalog function descriptors
30603095
- Connection executor and DDL operations
30613096
- Distributed SQL execution versioning
3097+
- Storage layer: MinimumSupportedFormatVersion updated to match new MinSupported
30623098
30633099
Part of #147634 (reference PR for this quarterly task).
30643100
@@ -3308,6 +3344,67 @@ rm -rf pkg/sql/sqlitelogictest/tests/local-mixed-25.2/
33083344
33093345
**Remove references from test files:**
33103346
3347+
**⚠️ CRITICAL: Understanding `onlyif` and `skipif` Directives**
3348+
3349+
The `onlyif config` and `skipif config` directives in logic tests **guard the next statement(s)**:
3350+
- `onlyif config local-mixed-25.2` → Run the next statement ONLY on local-mixed-25.2
3351+
- `skipif config local-mixed-25.2` → Skip the next statement on local-mixed-25.2
3352+
3353+
**IMPORTANT: These two directives require DIFFERENT handling when removing a config:**
3354+
3355+
**Rule 1: For `onlyif config local-mixed-25.2` directives:**
3356+
- Remove BOTH the directive AND the guarded statement(s)
3357+
- The guarded statement is version-specific and won't be needed after the config is removed
3358+
3359+
**Rule 2: For `skipif config local-mixed-25.2` directives:**
3360+
- Remove ONLY the directive itself
3361+
- KEEP the guarded statement(s)
3362+
- The guarded statement should run in all configs now that local-mixed-25.2 is removed
3363+
3364+
**Examples:**
3365+
3366+
```bash
3367+
# Example 1: onlyif directive (remove both directive AND statement)
3368+
3369+
# BEFORE:
3370+
onlyif config local-mixed-25.2
3371+
statement error pgcode 0A000 pq: unimplemented: usage of user-defined function
3372+
CREATE TABLE t1(a INT PRIMARY KEY, b INT DEFAULT f1());
3373+
3374+
statement ok
3375+
CREATE TABLE t1(a INT PRIMARY KEY, b INT DEFAULT f1());
3376+
3377+
# AFTER (CORRECT - removed both directive and guarded statement):
3378+
statement ok
3379+
CREATE TABLE t1(a INT PRIMARY KEY, b INT DEFAULT f1());
3380+
3381+
# Example 2: skipif directive (remove ONLY directive, keep statement)
3382+
3383+
# BEFORE:
3384+
skipif config local-mixed-25.2
3385+
statement error pgcode 0A000 unimplemented: cannot evaluate function in this context
3386+
ALTER TABLE test_tbl_t ADD COLUMN c int AS (test_tbl_f()) stored;
3387+
3388+
# AFTER (CORRECT - removed only directive, kept statement):
3389+
statement error pgcode 0A000 unimplemented: cannot evaluate function in this context
3390+
ALTER TABLE test_tbl_t ADD COLUMN c int AS (test_tbl_f()) stored;
3391+
```
3392+
3393+
**Why the difference?**
3394+
- `onlyif`: The guarded statement was ONLY for that specific config. Removing the config means we don't need that statement anymore (there's usually an unguarded version later)
3395+
- `skipif`: The guarded statement was for ALL configs EXCEPT the one being removed. Now that we're removing that config, the statement should run everywhere
3396+
3397+
**Pattern to follow for `onlyif`:**
3398+
1. Find `onlyif config local-mixed-25.2` line
3399+
2. Identify what statement it guards (usually the next 3-10 lines until blank line or next directive)
3400+
3. Remove BOTH the directive AND the guarded statement
3401+
4. If this leaves an empty subtest or empty user block, remove those too
3402+
3403+
**Pattern to follow for `skipif`:**
3404+
1. Find `skipif config local-mixed-25.2` line
3405+
2. Remove ONLY the directive line
3406+
3. Keep all following statements unchanged
3407+
33113408
Find all files with local-mixed-25.2 references:
33123409
```bash
33133410
grep -r "local-mixed-25\.2" pkg/sql/logictest/testdata/logic_test/ \
@@ -3330,6 +3427,24 @@ sed -i '' \
33303427
"$file"
33313428
```
33323429
3430+
**⚠️ IMPORTANT: Manual review required after sed**
3431+
3432+
The sed commands above only remove the *directives*, not the statements they guard. You MUST:
3433+
1. Manually review each file that had `onlyif config local-mixed-25.2`
3434+
2. Identify and remove guarded statements that would cause duplicates
3435+
3. For `skipif config local-mixed-25.2`, the sed commands are sufficient - no manual work needed
3436+
4. Run tests to catch "relation already exists" or similar errors
3437+
3438+
**How to find files needing manual review (onlyif only):**
3439+
```bash
3440+
# Find files that had onlyif with local-mixed-25.2 (these need manual review)
3441+
git diff pkg/sql/logictest/testdata/logic_test/ pkg/ccl/logictestccl/testdata/logic_test/ | \
3442+
grep -B 2 "^-onlyif config.*local-mixed-25\.2" | \
3443+
grep "^diff --git" | sed 's/.*b\///' | sort -u
3444+
3445+
# Files with skipif don't need manual review - the directive removal is sufficient
3446+
```
3447+
33333448
**Remove empty LogicTest directive lines:**
33343449
33353450
If any files have headers like `# LogicTest: !local-mixed-25.2` that become empty `# LogicTest:`, remove them:
@@ -3389,7 +3504,7 @@ A typical M.4 bump should modify approximately 70-80 files across the 6 commits:
33893504
**Commit 1 (1 file):**
33903505
1. `pkg/clusterversion/cockroach_versions.go` - Prefix version keys
33913506
3392-
**Commit 2 (~12 files):**
3507+
**Commit 2 (~13 files):**
33933508
1. `pkg/clusterversion/cockroach_versions.go` - MinSupported constant
33943509
2. `pkg/crosscluster/logical/logical_replication_writer_processor.go`
33953510
3. `pkg/crosscluster/physical/alter_replication_job.go`
@@ -3403,6 +3518,7 @@ A typical M.4 bump should modify approximately 70-80 files across the 6 commits:
34033518
11. `pkg/sql/create_table.go`
34043519
12. `pkg/sql/distsql_running.go`
34053520
13. `pkg/sql/execversion/version.go`
3521+
14. **`pkg/storage/pebble.go`** - MinimumSupportedFormatVersion constant
34063522
34073523
**Commit 4 (~30 files):**
34083524
1. `pkg/sql/schemachanger/scplan/internal/rules/release_25_2/` - Entire directory deleted (~25 files)
@@ -3512,7 +3628,58 @@ Expected format:
35123628
35133629
### Common Errors and Solutions
35143630
3515-
#### Error 1: Bazel generation failure after removing config
3631+
#### Error 1: TestMinimumSupportedFormatVersion test failure
3632+
3633+
**Error:** Test fails with message like:
3634+
```
3635+
Error: Not equal:
3636+
expected: 0x18
3637+
actual : 0x15
3638+
Test: TestMinimumSupportedFormatVersion
3639+
Messages: MinimumSupportedFormatVersion must match the format version for 1000025.3
3640+
```
3641+
3642+
**Cause:** After bumping MinSupported from V25_2 to V25_3, forgot to update `MinimumSupportedFormatVersion` in `pkg/storage/pebble.go`.
3643+
3644+
**Why this happens:**
3645+
- The test validates: `MinimumSupportedFormatVersion == pebbleFormatVersionMap[MinSupported]`
3646+
- Old value (0x15 = `FormatTableFormatV6`) was correct for V25_2
3647+
- New MinSupported (V25_3) requires new value (0x18 = `FormatValueSeparation`)
3648+
3649+
**Fix:**
3650+
```bash
3651+
# Edit pkg/storage/pebble.go (around line 2459)
3652+
# Look at pebbleFormatVersionMap to find the correct format for V25_3
3653+
3654+
# Example for V25_2 → V25_3:
3655+
# pebbleFormatVersionMap shows:
3656+
# V25_3: pebble.FormatValueSeparation
3657+
# Therefore:
3658+
-const MinimumSupportedFormatVersion = pebble.FormatTableFormatV6
3659+
+const MinimumSupportedFormatVersion = pebble.FormatValueSeparation
3660+
3661+
# Add to the MinSupported commit (Commit 2)
3662+
git add pkg/storage/pebble.go
3663+
3664+
# If already committed, amend:
3665+
git commit --amend --no-edit
3666+
3667+
# Or if there are later commits, use interactive rebase:
3668+
# (See the detailed instructions in Commit 2 section above)
3669+
```
3670+
3671+
**Verification:**
3672+
```bash
3673+
./dev test pkg/storage -f TestMinimumSupportedFormatVersion -v
3674+
```
3675+
3676+
Expected: Test passes.
3677+
3678+
**Reference:** This pattern is followed in all previous MinSupported bumps:
3679+
- Commit accc0455bf9 (v25.1 → v25.2): Updated to `FormatTableFormatV6`
3680+
- Commit 5807873f56f (v25.2 → v25.3): Updated to `FormatValueSeparation`
3681+
3682+
#### Error 2: Bazel generation failure after removing config
35163683
35173684
**Error:** `panic: unknown config name local-mixed-25.2`
35183685
@@ -3591,6 +3758,105 @@ git commit -m "..."
35913758
git add -A # Adds modifications AND deletions
35923759
```
35933760
3761+
#### Error 6: "relation already exists" or duplicate statement errors in logic tests
3762+
3763+
**Error:** Test failures like:
3764+
```
3765+
expected success, but found
3766+
(42P07) relation "test.public.t1" already exists
3767+
```
3768+
3769+
**Cause:** Removed `onlyif config local-mixed-25.2` directives but forgot to remove the statements they guarded, resulting in duplicate statements that both execute.
3770+
3771+
**Example from udf_in_table:**
3772+
```bash
3773+
# File had two CREATE TABLE statements:
3774+
# 1. Guarded by "onlyif config local-mixed-25.2" (lines 88-98)
3775+
# 2. Unguarded default version (lines 533-547)
3776+
3777+
# After removing ONLY the directive (WRONG):
3778+
statement ok
3779+
CREATE TABLE t1(...) # ← Was guarded, now runs on all configs!
3780+
3781+
# Later:
3782+
statement ok
3783+
CREATE TABLE t1(...) # ← Unguarded, always runs → DUPLICATE!
3784+
```
3785+
3786+
**How to identify:**
3787+
```bash
3788+
# After removing directives with sed, search git diff for removed onlyif lines
3789+
git diff pkg/sql/logictest/testdata/logic_test/ | grep "^-onlyif config.*local-mixed-25\.2"
3790+
3791+
# For each file, manually check if guarded statements should also be removed
3792+
```
3793+
3794+
**Fix:**
3795+
1. Read the test file and identify what the removed `onlyif` directive was guarding
3796+
2. Determine if the guarded statement is a duplicate of an unguarded statement elsewhere
3797+
3. If duplicate: remove the guarded statement (usually 3-10 lines after the removed directive)
3798+
4. If the removal leaves an empty subtest or user block, remove those too
3799+
3800+
**Example fix for udf_in_table:**
3801+
```bash
3802+
# Remove the entire guarded statement block (lines 88-98)
3803+
# Keep only the unguarded version (lines 533-547)
3804+
```
3805+
3806+
**Prevention:**
3807+
- When running sed to remove directives, immediately grep for removed `onlyif` lines
3808+
- Manually review each file that had `onlyif` to check for guarded statements
3809+
- Run tests early to catch duplicate statement errors: `./dev testlogic`
3810+
3811+
#### Error 7: Incorrect pebble format version change during rebase
3812+
3813+
**Error:** After rebasing against master, `pkg/storage/pebble.go` has the wrong pebble format version for V25_3.
3814+
3815+
**Symptom:**
3816+
```go
3817+
// WRONG - V25_3 value changed when it shouldn't have:
3818+
var pebbleFormatVersionMap = map[clusterversion.Key]pebble.FormatMajorVersion{
3819+
clusterversion.V25_4_PebbleFormatV2BlobFiles: pebble.FormatV2BlobFiles,
3820+
clusterversion.V25_3: pebble.FormatTableFormatV6, // ← WRONG!
3821+
}
3822+
3823+
// CORRECT - Only removed V25_2, kept V25_3 unchanged:
3824+
var pebbleFormatVersionMap = map[clusterversion.Key]pebble.FormatMajorVersion{
3825+
clusterversion.V25_4_PebbleFormatV2BlobFiles: pebble.FormatV2BlobFiles,
3826+
clusterversion.V25_3: pebble.FormatValueSeparation, // ← CORRECT
3827+
}
3828+
```
3829+
3830+
**Cause:** During rebase, git may auto-merge changes to `pebbleFormatVersionMap` incorrectly. The commit that removes `local-mixed-25.2` should only remove the V25_2 entry from this map, not change the V25_3 value.
3831+
3832+
**How to identify:**
3833+
```bash
3834+
# Check the pebble format version in the logictest removal commit
3835+
git show <commit-sha>:pkg/storage/pebble.go | grep -A 4 "pebbleFormatVersionMap ="
3836+
3837+
# Compare with master to see what the correct V25_3 value should be
3838+
git show master:pkg/storage/pebble.go | grep -A 4 "pebbleFormatVersionMap ="
3839+
```
3840+
3841+
**Fix:**
3842+
```bash
3843+
# Start interactive rebase to edit the problematic commit
3844+
git rebase -i <base-commit>
3845+
3846+
# Mark the commit with pebble.go for 'edit'
3847+
# Then fix the file:
3848+
# Edit pkg/storage/pebble.go to restore correct V25_3 value
3849+
git add pkg/storage/pebble.go
3850+
git commit --amend --no-edit
3851+
git rebase --continue
3852+
```
3853+
3854+
**Prevention:**
3855+
- After rebase, always check `pkg/storage/pebble.go` changes carefully
3856+
- The change should only be a deletion of the old version's line (V25_2)
3857+
- Never change the values for existing newer versions (V25_3, V25_4, etc.)
3858+
- Reference PR #147634 to verify the expected pattern
3859+
35943860
### Quick Reference Commands
35953861
35963862
**Commit 1 - Prefix version keys:**
@@ -3606,9 +3872,15 @@ git commit -m "clusterversion: prefix version keys below 25.3 with TODO_Delete_.
36063872
git grep -l "MinSupported" pkg/ | grep -v "_test.go"
36073873
git grep "V25_2" pkg/ | grep -v testdata
36083874
3609-
# Update files manually
3610-
git add pkg/clusterversion/cockroach_versions.go <other-files>
3611-
git commit -m "clusterversion: bump MinSupported from v25.2 to v25.3..."
3875+
# Update files manually (including pkg/storage/pebble.go!)
3876+
# In pkg/storage/pebble.go, update MinimumSupportedFormatVersion to match
3877+
# the pebbleFormatVersionMap entry for the new MinSupported version
3878+
3879+
git add pkg/clusterversion/cockroach_versions.go pkg/storage/pebble.go <other-files>
3880+
git commit -m "clusterversion, storage: bump MinSupported from v25.2 to v25.3..."
3881+
3882+
# Verify with test
3883+
./dev test pkg/storage -f TestMinimumSupportedFormatVersion -v
36123884
```
36133885
36143886
**Commit 4 - Remove schema changer rules:**

0 commit comments

Comments
 (0)