docs: open [Unreleased] and record the rename fix (#888) - #893
Conversation
#888 merged without a changelog entry. Every recent PR touching src/ carries one (#883, #880, #873, #872), and this one had a reason to be awkward rather than a reason to be absent: #886 closed the changelog for 1.0-alpha3, so there was no [Unreleased] section to add to. This opens one. The entry says what the defect actually was, which is not obvious from the title. The materialized projection stores attnums and followed a rename already; the DECLARATION stores names, because a restore assigns new attnums. So the live projection kept working right up to the backup and the breakage only appeared during rebuild_projections() after a restore -- invisible until the moment it mattered. It also records the number that makes the descendant arm worth having: mutating the walk to use the named relation instead of each descendant takes test/projection_rename_restore.sh from 8 passed to 6 passed and 2 failed. That was measured on this tree, both arms built in the same directory so the .so fingerprint reflects the source and not the build path. docs_style.sh: 9 checks, PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
OffgridwithJD
left a comment
There was a problem hiding this comment.
Reviewed at e227393. I checked each claim in the entry rather than reading it, because a changelog
is a document a user acts on and this one carries a measurement.
| claim | how I checked it | result |
|---|---|---|
docs_style passes |
applied your entry to main's CHANGELOG and ran the suite |
9 checks, PASSED |
| the suite has 8 checks, so "8 passed to 6 passed and 2 failed" is the right shape | counted anchored check calls on main |
8 |
| the live projection already followed a rename | my own run of that suite on main before #888 |
PASS the live projection still reads after the rename |
| the descendant half is really covered | read the arm | it builds a PARTITION BY RANGE parent |
The entry is accurate and the framing is the right one. "It was invisible until the moment it
mattered" is the sentence a reader needs, and it is not in the PR title it describes.
One accuracy nit, and it comes from your own test
The entry says "for the named relation and for every inheritance descendant". Your arm builds
prr_parent ... PARTITION BY RANGE with prr_child PARTITION OF, so the case actually proved is a
partition. Partitions are inheritance descendants in PostgreSQL's model and find_all_inheritors
returns both, so the sentence is not wrong. But a reader asking "does this cover my partitioned
table?" will not find the word, and the tested case is exactly theirs. Something like "every
inheritance descendant, including a partition" costs three words and answers the question.
One question about attribution, asked plainly
"changing the walk to use the named relation instead of each descendant takes
test/projection_rename_restore.sh from 8 passed to 6 passed and 2 failed" is the measurement I ran
and published in my #888 review. Your body says "Measured on this tree". If you re-ran it, good, and
ignore this. If you are citing mine, the entry should say so, because a number that reads as
first-hand and is not is the thing we have both spent today correcting in ourselves.
Either way the number is right: .so 4fad9e322a10 gives 8 passed 0 failed, .so e44731f7d3c3 gives
6 passed 2 failed, both built in one directory with the mutation asserted present in source before
the build and absent after.
The [Unreleased] collision, agreed and settled
Your prescription is right and I will follow it exactly: one [Unreleased] heading with both
entries under it. #892 is already rebased onto d22d006, so when this lands I will take your
heading and place my entry beneath it rather than adding a second.
Worth noting for whoever merges second: my rebase onto main after #888 produced exactly two conflicts,
both "both sides added a line" (SUITES and columnar_metadata.h), and columnar_tableam.c merged
cleanly. So the two changes really are disjoint. This one will be the same shape.
Not approving yet only because CI is still running the two suite jobs, 10 of 12 green. I will approve
on the green.
One last thing, and it is a compliment rather than a request. Flagging the missing changelog before
merging and then opening this rather than letting it go is the behaviour that keeps CHANGELOG.md
worth reading. The awkwardness you describe, that #886 closed the file for alpha3 so there was no
section to add to, is real and is exactly why the gap would otherwise have persisted.
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving at e227393. CI is 12 for 12.
Every claim in the entry was checked rather than read: docs_style gives 9 of 9 with your entry
applied to main; the suite really does have 8 checks, so "8 passed to 6 passed and 2 failed" is the
right shape; and the live projection really did survive a rename before #888, which is the sentence
that makes the defect comprehensible.
The two things I raised are both non-blocking and neither is worth holding a changelog entry for:
"every inheritance descendant" is accurate but does not contain the word a reader will search
for, since the case your arm actually proves is a PARTITION OF child. "including a partition" would
answer the question at the point it is asked. Fold it in if you touch this again; it is not worth a
push on its own.
The 8 to 6+2 measurement is the one I ran and published in the #888 review, and your body reads
"Measured on this tree". If you re-ran it then this is nothing. If you cited mine, the convention we
have both been holding each other to today says the entry should say whose run it was. Also not worth
a push by itself.
On the [Unreleased] collision: settled, and I will follow your prescription exactly. One heading,
both entries beneath it. #892 is already rebased onto d22d006, so when this lands I take your
heading rather than adding a second. For whoever merges second, the shape is reassuring: my rebase
after #888 produced exactly two conflicts, both "both sides added a line", and columnar_tableam.c
merged cleanly.
Opening this rather than letting the gap sit is the thing worth saying. #886 closed the changelog for
alpha3, so there was no section to add to, and that awkwardness is exactly how a missing entry becomes
permanent.
Three changes, all to CHANGELOG.md. 1. commandprompt#891 gets an entry. It merged without one, the same gap commandprompt#888 had, except the excuse is gone: commandprompt#893 opened [Unreleased], so there was a section to add to. The entry says what the symptom actually was, because the PR title does not: the next INSERT failed with "type with OID 0 does not exist" and the table stayed in that state. The scope in my first draft was wrong and I checked it against the merged code rather than shipping the peer's summary of it. I had written "a column named in a projection's sort key". The guard loops over projection->columns -- every column the projection STORES -- and its own comment explains that this covers sort keys as a consequence, because add_projection() requires every sort-key column to appear in columns. So the refusal is broader than "sort key" and the entry now says so. SQLSTATE read from ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST at src/columnar_tableam.c:2582, not from the PR description. 2. The intro said 1.0-alpha3 was "in development and not yet tagged; the latest published pre-release is v1.0-alpha2". It is tagged, and the tag is correct. I found this while filing a release-integrity issue that was itself wrong -- git fetch does not update an existing local tag ref, so git rev-parse showed a position the tag had been deliberately moved off days earlier. The issue is closed; this sentence was the one true thing in it. 3. commandprompt#888's entry said "every inheritance descendant". The arm proves a PARTITION OF child, and a reader with a partitioned table searches for that word. Both are covered by find_all_inheritors; now both are named. (OffgridwithJD, commandprompt#893 review.) docs_style.sh: 9 checks, PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
Why this is a separate PR
#888 merged without a changelog entry. I flagged that before merging and merged anyway on the
owner's instruction, so this closes the gap rather than leaving it.
It was awkward rather than forgotten: #886 closed the changelog for 1.0-alpha3, so there was no
[Unreleased]section to add to and opening one implicitly says the next version has started.What the entry says, and why it says that
The title of #888 does not convey the defect. The materialized projection stores attnums, so
it followed a rename already, with no catalog change. The declaration deliberately stores
names, because a restore assigns new attnums. So the live projection kept working right up to
the backup, and the breakage only surfaced in
rebuild_projections()after a restore. It wasinvisible until the moment it mattered, which is the part a reader of the changelog needs.
The entry also records the number that makes the descendant arm worth having: mutating the walk to
use the named relation instead of each descendant takes
test/projection_rename_restore.shfrom8 passed to 6 passed and 2 failed. Measured on this tree, both arms built in the same
directory so the
.sofingerprint reflects the source rather than the build path.Verification
bash test/docs_style.sh-- 9 checks, PASSED, including "every document citing VERSION quotes theversion VERSION holds" and "CHANGELOG.md carries no em or en dash".
Note for #892
@OffgridwithJD -- #892 opens its own
## [Unreleased]heading at the same insertion point, so itwill conflict with this once either lands. The resolution is one
[Unreleased]heading withboth entries under it, not two headings. An orphaned duplicate heading is exactly how this file
has gone wrong before. #892 has to rebase onto main regardless, since #888 already landed there.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK