refactor: the six columnar.h declarations nothing outside their file uses (#496) - #527
Conversation
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. +4/-93, and the four files it touches are the four that own the declarations it removes.
Verified each one is genuinely single-consumer rather than trusting the count, and my first attempt at that check was wrong in an instructive way. A plain grep -rl PgColumnarVMSetVisible reports two files, which would make static a compile error. It is a prefix of PgColumnarVMSetVisibleForRelation. With grep -rlw:
PgColumnarVMSetVisible src/columnar_visibilitymap.c
PgColumnarVMClearVisible src/columnar_visibilitymap.c
PgColumnarVMIsVisible src/columnar_visibilitymap.c
PgColumnarCodecAvailable src/columnar_compression.c
PgColumnarComputeFullyDeletedGroups src/columnar_metadata.c
PgColumnarReadBloomList src/columnar_metadata.c
Six for six. CI agreeing is the stronger evidence — static on a symbol used from another translation unit does not link — but the word-boundary check is what makes the claim readable rather than inferred from a green build.
Worth recording that my sloppy probe matched a longer name, since that is the same failure shape we have been cataloguing all day pointed at a review rather than at code. A prefix match is an instrument that cannot distinguish two symbols.
On the change
This is the right first slice of #496 and the right size. Six declarations that nothing outside their own file uses, made static, with the header shrinking by the same amount. No behaviour, no reordering, nothing that needs a matrix argument beyond the one CI already gives.
The value is not the 93 lines. It is that columnar.h stops advertising six things as an interface when they are implementation detail, so the next reader looking for the actual seam has six fewer false leads. #496's complaint is that 65% of the header's declarations have a single consumer; this is a demonstration that the number is real and that shrinking it is mechanical rather than risky.
Merging.
columnar.h declares 137 functions. 84 of them have exactly one consumer outside their defining file -- a private arrangement between two files that the other twenty are forced to recompile for, and that anyone reading the header has to scan past to reach the 49 that are genuinely shared. This moves the largest single group: the 22 declarations defined in columnar_metadata.c, into src/columnar_metadata.h beside it. The shared vocabulary they take -- the Native*Metadata structs, the GUCs, the format constants -- stays in columnar.h, which the new header includes. The census in #496 was stale and understated the problem. Re-measured on 3e86a7e: when filed today columnar.h lines 957 1045 extern declarations 159 164 single-consumer 79 84 so the header gained interface faster than #527 removed it. Re-measuring also caught an error in my own tool. Its first version attributed PgColumnarBloomBuild and PgColumnarEncodeChunk to columnar_write_state.c, which does not define either: it took whichever file matched first in directory order. Fixed to require a definition at column 0, which is the style this tree uses, and the ranking changed -- columnar_metadata went from outside the top three to the largest module at 22, and columnar_write_state from 25 to 11. The wrong module would have been extracted first. Gate: builds on pg15a, pg16a, pg17a, pg18a and pg19a, make clean between each (#536), zero warnings and zero errors on all five. That is the gate that matters for a declaration-only change: nothing here alters a line of executable code, and a consumer left without the declaration it needs fails to compile rather than misbehaving. native_vecdecode 24/24 and harness_selftest 54/54 on pg18a besides. Five modules remain -- reader (12), write_state (11), storage (10), customscan (6), delete_vector (6). One PR each: 84 declarations in a single diff is not a reviewable change.
A first, deliberately small bite of #496: the declarations in
columnar.hthat nothing outside their defining file uses. The issue counts 10 of these and calls them the cheapest thing available. Six of them are, and this is those six.No behaviour changes. 93 lines removed, 4 added.
What changed
PgColumnarCodecAvailablecolumnar_compression.cPgColumnarReadBloomListcolumnar_metadata.cPgColumnarComputeFullyDeletedGroupscolumnar_metadata.cstaticPgColumnarVMSetVisiblecolumnar_visibilitymap.cstaticPgColumnarVMClearVisiblecolumnar_visibilitymap.cstaticPgColumnarVMIsVisiblecolumnar_visibilitymap.cstaticThe three VM helpers are the low-level primitives;
columnar_tableam.cuses the wrappersPgColumnarVMClearForRowandPgColumnarVMSetVisibleForRelation, which stay exported. Deleted rather than madestaticwhere there was no in-file caller either, because an uncalledstaticis an unused-function warning, and this matrix treats warnings as failures.Why it is six and not ten
Four of the ten cannot be touched, and the reason is one the compiler will not give you.
PgColumnarBlockNextRun,PgColumnarBlockReaderInit,PgColumnarCheckFreeSpaceNoOverlapandPgColumnarEncodingNameare named intest/orsql/— they are reached through the fmgr by name, fromCREATE FUNCTION ... AS 'pgcolumnar', 'sym', not by a C caller.Making one of those
staticcompiles and links cleanly and then fails at run time when the SQL binding cannot resolve the symbol. So "no.cfile references it" is not sufficient evidence that a declaration is private, and the audit that produced this list disqualifies anything withPG_FUNCTION_INFO_V1or a mention undertest/orsql/before proposing it.That distinction is the part of this worth keeping for the rest of #496.
Verification
A successful build proves less than it looks like here: a shared library links with undefined symbols permitted, so a stale reference to a now-
staticfunction would not failmake— it would fail atdlopen, i.e. atCREATE EXTENSION. So the load-bearing evidence is the suites, not the compile.index_only27 checks / 0 fail — the suite that drives the visibility-map path these three helpers implement.CREATE EXTENSION, which is where a missing symbol would surface.I also tried to verify linkage directly with
nmand could not make it say anything trustworthy: the.soexports nothing in the dynamic table (194 symbols exist only in the full one), and the still-shared wrappers report the same symbol type as the newly-static ones, so the check could not distinguish the two cases it was supposed to distinguish. Recording that rather than quoting the output it did produce.The gate ran one commit before #524 merged; that commit adds
bench/provision.shonly, and no suite executes or reads it (bench_guardsinspectscb_guards.sh), so the matrix outcome cannot turn on it.Refs #496.