refactor: columnar_reader gets its own header, the second of six (#496) - #547
Conversation
ChronicallyJD
left a comment
There was a problem hiding this comment.
Reviewed the same way as #546. Clean, no objection to the content. The
structural finding is shared with #546 and I have written it up in full there
rather than twice.
Verified
Every .c that gained #include "columnar_reader.h" uses symbols from it:
columnar_reader.c 15 of 15 columnar_customscan.c 8
columnar_tableam.c 4 columnar_vector.c 4
No include added that nothing uses.
The shared finding, in short
#546 and #547 are both single commits based on main, neither containing the
other, despite being titled "the first of six" and "the second of six".
Trial-merging them in sequence conflicts in three files:
src/columnar_reader.c src/columnar_tableam.c src/columnar_vector.c
Not columnar.h — the removed hunks there auto-merge. It is the #include
lines added at the same place in the consumers both PRs touch, so the conflicts
surface in .c files that look unrelated to a header split. With six planned,
every pair sharing a consumer collides the same way.
Full detail on #546.
Worth noting for the remaining four
columnar_customscan.c uses 8 of this header's 15 symbols, which is the largest
cross-file dependency I have seen in either split so far. If a later PR in the
chain moves customscan's own declarations, that is the pair most likely to
produce a conflict that is genuinely about the split rather than about include
placement, and it may be worth taking that one before the easier ones while the
chain is short.
…mandprompt#496) The 12 declarations defined in columnar_reader.c with exactly one consumer outside it: the fold entry points, the liveness cache, the projection and filtered-read calls. Consumed only by customscan, tableam and vector. Gate: builds on pg15a, pg16a, pg17a, pg18a and pg19a, make clean between each (commandprompt#536), zero warnings and zero errors on all five. native_vecdecode 24/24, native_fold_skipguard 4/4 and harness_selftest 54/54 on pg18a. NOT independent of commandprompt#546, and I claimed it was. I measured the closest pair of declaration removals in columnar.h between the two patches -- 49 lines, well outside git's 3-line context -- and concluded they could both sit on main. The gate applied both and refused: error: patch failed: src/columnar_reader.c:14 error: patch failed: src/columnar_tableam.c:13 error: patch failed: src/columnar_vector.c:36 The collision is not in the header at all. Both patches add their #include immediately after #include "columnar.h" in the three files that consume BOTH modules -- reader, tableam and vector -- so they edit the same anchor line. My arithmetic was correct about the thing it measured and blind to the surface that actually collides. This still bases on main rather than stacking on commandprompt#546, deliberately. Stacking is what auto-closed commandprompt#534 this morning when its base branch was deleted on merge, and the conflict here is three adjacent include lines: whichever of the two lands second wants a rebase that is mechanical and takes a minute. That is the cheaper of the two failure modes.
c1a62c5 to
7f17ffd
Compare
) write_state (11), storage (10), customscan (6) and delete_vector (6) — the 33 remaining declarations with exactly one consumer outside their defining file. ONE PR rather than four, and the reason is how commandprompt#546 and commandprompt#547 were reviewed: by running checks — every new include used, no moved symbol having more than one consumer — not by reading declarations. Correctness here is established by script, so four diffs are no easier to review than one, and one diff collides with commandprompt#546 and commandprompt#547 once instead of up to fifteen times. Every pair of module PRs sharing a consumer conflicts on the #include line, which is what made a six-PR series expensive. Not stacked, deliberately: a stacked PR auto-closes when its base is deleted on merge (commandprompt#534, this morning). Merge in any order; whichever is second wants a one-minute rebase of the include lines. A defect in my splitting tool, found by the count assert and worth recording because it silently produced a WRONG module assignment rather than an error: columnar.h:353 is extern void PgColumnarEnsureStorageRow(Relation rel);\t/* pre-create ... */ which ends in "*/", not ";". The block scanner ran to the next line ending in ";" and swallowed the declaration below it, filing PgColumnarReserveRowNumbers — a storage symbol — under write_state.h. It compiles either way, because a declaration is visible from whichever header the consumer includes; only the "matched 9 of 10" assert on the NEXT module caught it. Terminator detection now strips trailing comments. I checked whether it reached the open PRs. It did not: commandprompt#546 and commandprompt#547 carry exactly their intended sets, 22 and 12, nothing extra, nothing missing, nothing left behind in columnar.h. That is luck — neither module's declarations happened to sit beside one with a trailing comment. All four verified exact: 11/11, 10/10, 6/6, 6/6, no extra symbol, none missing, none stranded. columnar.h goes 1045 -> 997 lines and 164 -> 131 externs. Gate: pg15a, pg16a, pg17a, pg18a, pg19a, make clean between each (commandprompt#536), zero warnings and zero errors on all five. native_vecdecode 24/24, harness_selftest 54/54, replication 41/41 on pg18a.
) write_state (11), storage (10), customscan (6) and delete_vector (6) — the 33 remaining declarations with exactly one consumer outside their defining file. ONE PR rather than four, and the reason is how commandprompt#546 and commandprompt#547 were reviewed: by running checks — every new include used, no moved symbol having more than one consumer — not by reading declarations. Correctness here is established by script, so four diffs are no easier to review than one, and one diff collides with commandprompt#546 and commandprompt#547 once instead of up to fifteen times. Every pair of module PRs sharing a consumer conflicts on the #include line, which is what made a six-PR series expensive. Not stacked, deliberately: a stacked PR auto-closes when its base is deleted on merge (commandprompt#534, this morning). Merge in any order; whichever is second wants a one-minute rebase of the include lines. A defect in my splitting tool, found by the count assert and worth recording because it silently produced a WRONG module assignment rather than an error: columnar.h:353 is extern void PgColumnarEnsureStorageRow(Relation rel);\t/* pre-create ... */ which ends in "*/", not ";". The block scanner ran to the next line ending in ";" and swallowed the declaration below it, filing PgColumnarReserveRowNumbers — a storage symbol — under write_state.h. It compiles either way, because a declaration is visible from whichever header the consumer includes; only the "matched 9 of 10" assert on the NEXT module caught it. Terminator detection now strips trailing comments. I checked whether it reached the open PRs. It did not: commandprompt#546 and commandprompt#547 carry exactly their intended sets, 22 and 12, nothing extra, nothing missing, nothing left behind in columnar.h. That is luck — neither module's declarations happened to sit beside one with a trailing comment. All four verified exact: 11/11, 10/10, 6/6, 6/6, no extra symbol, none missing, none stranded. columnar.h goes 1045 -> 997 lines and 164 -> 131 externs. Gate: pg15a, pg16a, pg17a, pg18a, pg19a, make clean between each (commandprompt#536), zero warnings and zero errors on all five. native_vecdecode 24/24, harness_selftest 54/54, replication 41/41 on pg18a.
…dprompt#536) Rebased onto d19c2e4 after commandprompt#544, commandprompt#547 and commandprompt#549 landed. Rebuilt from main rather than resolved: the union resolution of an earlier rebase produced a lib.sh that bash -n rejected, and patching a mangled file is how a wrong resolution ships. Fixes a defect found in review by the other session, not by any check: the stamp was written with printf '%s\\n', which emits the four bytes 1 9 \ n. It worked only because the reader does tr -dc '0-9' and strips the junk; a direct comparison against 19 failed. That is the third over-escaping in this PR -- the message printf was the second -- and bash -n accepts all of them, because they are syntactically valid and semantically wrong. Two checks added for it: the stamp must read back as the bare major, and lib.sh must not contain the doubled-backslash writer.
Second of six for #496. The 12 declarations defined in
columnar_reader.cwith exactly one consumer outside it — the fold entry points, the liveness cache, the projection and filtered-read calls — consumed only bycustomscan,tableamandvector.Gate
make cleanbetween each (#536).native_vecdecode24/24,native_fold_skipguard4/4,harness_selftest54/54 on pg18a.This is NOT independent of #546, and I said it was
I set out to make the six module PRs independent so none of them auto-closes when a sibling merges — the failure that closed #534 this morning. I measured the closest pair of declaration removals in
columnar.hbetween this patch and #546's: 49 lines apart, comfortably outside git's 3-line context, and concluded they could both sit onmain.The gate applied both to one tree and refused:
The collision is not in the header. Three files consume both modules —
reader,tableam,vector— and both patches insert their#includeimmediately after#include "columnar.h", so they edit the same anchor line in the same three files. My arithmetic was right about the thing it measured and blind to the surface that actually collides, which is the second time today I have proved something adjacent to the question.So: still based on main, deliberately
The two options were stack or rebase, and rebase is cheaper here.
Stacking is what auto-closed #534: its base branch was deleted when the parent merged, and a closed PR whose base is gone can be neither reopened nor retargeted. The conflict in this case is three adjacent include lines — whichever of #546 and this one lands second wants a rebase that is mechanical and takes a minute.
A minute of rebase beats a PR that closes itself. Merge them in either order; I will rebase whichever is second.
The same collision will recur for every remaining module that shares a consumer, so this is the standing arrangement for the series rather than a one-off.
I have not merged this and will not.