schema: review-report-v2
target: "CommitFest 61, Patch 6055: Whole-row fixes for DROP COLUMN, SET COLUMN DATA TYPE"
head: a82e33a83d7cd39f3abd18c7b941625ca78d4c35
base: a82e33a83d7cd39f3abd18c7b941625ca78d4c35
verdict: needs-review
blocker_counts:
  verified: 0
  unverified: 0
diff_sha: d98eecf92cd4c2bf764e33cc7d788cb453be2c4c8d4193157302ccd5d6807966
model: glm-5.2:cloud
date: 2026-08-14

Review: Whole-row fixes for DROP COLUMN, SET COLUMN DATA TYPE

CommitFest 61, Patch 6055 | Author: Jian He | Reviewer: pg-review-harness (glm-5.2:cloud) Thread: https://postgr.es/m/CACJufxGA6KVQy7DbHGLVw9s9KKmpGyZt5ME6C7kEfjDpr2wZCw@mail.gmail.com

Scope

The patch modifies ATExecDropColumn() in src/backend/commands/tablecmds.c to also drop CHECK constraints and indexes that contain whole-row Var references (attnum 0) when any column is dropped. The existing dependency machinery already drops objects that directly reference the dropped column; the patch closes a gap where objects referencing the whole row (e.g., CHECK (t IS NOT NULL), CREATE INDEX ON t ((t)) WHERE t IS NOT NULL) were orphaned with a stale rowtype after DROP COLUMN.

Changed files: src/backend/commands/tablecmds.c (+122 lines), src/test/regress/expected/constraints.out (+17), src/test/regress/expected/indexing.out (+25), src/test/regress/sql/constraints.sql (+11), src/test/regress/sql/indexing.sql (+9)

Changed function: ATExecDropColumn (lines 9425-9738)

Verdict: needs-review

No BLOCKERs. Three MAJOR findings (2 correctness, 1 test). Live thread objection from Matthias van de Meent (2026-08-05) regarding the scanning approach. The MAJ-1 else if bug must be fixed before commit. The MAJ-2 scope discrepancy should be clarified with the author. The TST-1 expected-output mismatch must be fixed for the test suite to pass.

Findings

Verification Summary

Repair Validation Summary

• MAJ-1: Repair patch applied to disposable build, compiled successfully, and tested. The else if → if change with bms_free and continue correctly drops indexes with whole-row references in indexprs even when indpred is non-null. Manual test confirmed: CREATE INDEX ON maj1_fix_test ((maj1_fix_test)) WHERE a > 0 is dropped after ALTER TABLE DROP COLUMN c.
• TST-1: One-line expected-output fix applied and verified. The index name idxpart_idxpart_idx matches PostgreSQL's actual naming convention.
• MAJ-2, MIN-1, MIN-2, MIN-3, NIT-1 to NIT-4: Concrete patches provided but not independently compiled (trivial fixes or refactor proposals for the author).

Changed-Code Coverage

Execution lane: host-disposable (passed) Counter epoch: fresh (disposable build with -fprofile-arcs -ftest-coverage)

Metrics

Uncovered lines (2)

Both are defensive elog(ERROR) paths that should never execute in normal operation:

1. Line 9555: elog(ERROR, "constraint \"%s\" of relation \"%s\" does not exist", ...) — infeasible (requires corrupted catalog state)
2. Line 9580: elog(ERROR, "cache lookup failed for index %u", indexoid) — infeasible (requires corrupted catalog state)

Touched function

Non-instrumented changes

• src/test/regress/expected/constraints.out — test expected output
• src/test/regress/expected/indexing.out — test expected output
• src/test/regress/sql/constraints.sql — test SQL
• src/test/regress/sql/indexing.sql — test SQL

Thread Reconciliation

Thread: 621482 (17 messages, 2025-09-08 to 2026-08-05) Ledger report: commitfest-6055-review-thread-ledger

Key thread events:

• Jinbinge (2026-01-19) already raised MIN-1 (bms_free leak)
• Author himself recognized MIN-2 in v5 (2025-09-23) but fix was lost in v14
• Kirill Reshke (2026-01-18) already raised MIN-3 (single scan)
• Lakshmi (2026-07-20) tested v14 and confirmed it works
• Matthias van de Meent (2026-08-05) raised a live design objection: criticizes the scanning approach as O(total_dependent_objects), proposes pg_depend registration with own objsubid, and argues indexes should be rebuilt not dropped. This is a design-level objection the committer should weigh.

Aggregated Test Plan

1. Existing tests: constraints and indexing regress tests (indexing fails due to TST-1)
2. MAJ-1 test: CREATE INDEX ON t ((t)) WHERE a > 0; ALTER TABLE t DROP COLUMN c; — verify index is dropped
3. MAJ-2 test: CREATE TABLE t (... CONSTRAINT cc CHECK (t is not null)); ALTER TABLE t ALTER COLUMN c SET DATA TYPE bigint; — verify error is raised
4. Coverage tests: CHECK constraint whole-row drop, index expression whole-row drop, index predicate whole-row drop

Provenance

• Patch reviewer seat: glm-5.2:cloud (handoff: commitfest-6055-review-handoff-patch-reviewer)
• Security reviewer seat: glm-5.2:cloud (handoff: commitfest-6055-review-handoff-security-reviewer)
• Thread ledger: commitfest-6055-review-thread-ledger
• Coverage: host-disposable lane, fresh gcov epoch
• Model routing: policy v1, hash 997c0355... (all seats overridden to glm-5.2:cloud)

Residual Risk

• The MAJ-1 else if bug is a real correctness issue that causes latent index corruption. It must be fixed before commit.
• The MAJ-2 scope discrepancy means the SET DATA TYPE whole-row problem remains unfixed. The author should clarify whether this is intentional.
• Matthias van de Meent's design objection (pg_depend registration vs scanning) is a valid architectural concern that may warrant a different approach in a future patch.
• The bms_free leak (MIN-1) and redundant scans (MIN-2, MIN-3) are performance issues that should be addressed but are not blockers.