fix: attnum drift causes index mismatch with inheritance/hypertables (#288)#289
fix: attnum drift causes index mismatch with inheritance/hypertables (#288)#289
Conversation
…288) When a parent table has dropped columns, child tables created after the drop get different physical attnums for the same logical column. Compare indexed column names instead of raw attnums when matching parent/child BM25 indexes.
There was a problem hiding this comment.
LGTM — the column-name comparison approach is correct and well-tested; the two inline nits are low-impact and only reachable under catalog corruption.
Extended reasoning...
Overview
This PR modifies two code paths in src/am/scan.c (indexes_match_by_attribute) and src/types/query.c (find_first_child_bm25_index, bm25_text_bm25query_score) to compare column names instead of raw attribute numbers when matching parent/child BM25 indexes. Regression tests covering both paths are added to test/sql/inheritance.sql and test/expected/inheritance.out.
Security risks
None. The change is a catalog lookup substitution; no auth, crypto, or permission logic is touched.
Level of scrutiny
Moderate. The change is a targeted bug fix with a clear root cause (attnum drift after ALTER TABLE DROP COLUMN). The logic—resolve attnum → column name, compare names—is sound and mirrors the already-correct pattern used in find_first_child_bm25_index at query.c:493. Regression output confirms the tests pass and attnum drift is demonstrably present and handled.
Other factors
The two bugs flagged by the inline comments (both missing_ok=false instead of true) are real but minor: one causes dead NULL-guard code in scan.c with no incorrect behavior; the other creates a theoretical index_rel leak in query.c that only fires under catalog corruption and is reclaimed by PostgreSQL's ResourceOwner at transaction abort regardless. Neither is a regression introduced by this PR relative to the pre-fix behavior, and both could be cleaned up in a small follow-on. The core fix is correct and the test coverage is solid.
Address code review: get_attname with missing_ok=false would ereport(ERROR) instead of returning NULL, making the NULL guards dead code. Use missing_ok=true to match the pattern in find_first_child_bm25_index and allow graceful fallback.
|
@tjgreen42 any rough expected timeline for a v0.6.2 patch release landing this change and ideally #292 ? or should i build from main branch directly? |
@creatorrr I'm planning to cut the v1.0.0 release later this week, so you can either wait for that, or build from main directly for now. |
Summary
Fixes #288.
When a parent table has dropped columns, child tables created afterward
don't inherit the dropped-column slot. This causes the same logical column
to have different physical
attnumvalues in parent vs child. Two codepaths compared raw attnums and rejected valid parent/child BM25 index pairs:
indexes_match_by_attribute()insrc/am/scan.c— the index scanvalidation path. Failed with
ERROR: tpquery index mismatch.find_first_child_bm25_index()insrc/types/query.c— the scoringfallback path. Silently returned no matching child index.
Both now resolve attnums to column names via
get_attname()and comparethose instead. This handles TimescaleDB hypertables, plain
INHERITStables, and any other case where dropped columns cause attnum divergence.
Reproduces without TimescaleDB using plain inheritance:
Testing
test/sql/inheritance.sql(Test 4a and 4b)covering both the index scan path and the scoring fallback path