Skip to content

docs: fix comments and docs that contradict the code (#291) - #336

Merged
ChronicallyJD merged 1 commit into
jdatcmd:mainfrom
ChronicallyJD:docs/291-comment-audit
Aug 2, 2026
Merged

docs: fix comments and docs that contradict the code (#291)#336
ChronicallyJD merged 1 commit into
jdatcmd:mainfrom
ChronicallyJD:docs/291-comment-audit

Conversation

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

What

Addresses #291 (audit all code comments and documentation). A multi-agent audit swept every source file and doc; each finding was adversarially re-checked against the code before inclusion. 38 verified defects are fixed here.

Categories

  • Comment describes the wrong function. columnar.h gave ColumnarComputeFullyDeletedGroups the all-visible function's comment (opposite meaning and wrong return type); both comments are now on the functions they describe. Three more comments in columnar.h sat above the wrong declaration (parallel-counter, fetch-by-row) and were moved.
  • Dead columnar. schema references. Many docstrings named columnar.export_parquet, columnar.import_arrow, columnar.add_projection, columnar.vm_selftest, GUCs like columnar.reclaim_coalesce, etc. The functions and GUCs live in the pgcolumnar schema, so a reader copy-pasting from a docstring would hit "does not exist". One user-facing errmsg named the non-existent columnar.storageid_seq, now corrected.
  • Comment contradicts the code. The FDW partition reader header said the value text is "taken literally ... decoding deliberately not done here", but the code percent-decodes it (security-relevant). groupagg_max_groups was documented as a plan-time gate when it is an execution-time hard cap that errors. columnar_relation_vacuum was labelled a no-op when it marks the VM fork and retires fully-deleted groups. The parquet codec header grouped gzip with Snappy as self-describing when gzip needs the caller's usize.
  • Stale phase-N headers. columnar_tableam.c ("stubbed for later phases" for now-implemented index/vacuum callbacks), columnar_reader.c ("Phase 1 stores value streams uncompressed"), columnar_write_state.c ("Phase D2b baseline ... uncompressed"), columnar_projection.c ("phase 1 ... no data written yet").
  • Orphaned / vestigial. A doc block for the deleted columnar_run_agg; a decompressed-chunk-cache setup comment for a cache that does not exist; an empty "Column cache" section in configuration.md.
  • Docs drift. ARCHITECTURE.md bloom-filter collation eligibility and the Arrow supported-type list were both stale.

Not here

The five stale "require superuser" claims this audit also surfaced are fixed in #335 (server-file docs), to keep that concern in one PR.

Verification

Comments and docs only; no code logic changed (the single errmsg change is a corrected object name). ste_check.py and docs_style.sh pass, and the tree compiles clean on PG18 and PG19.

🤖 Generated with Claude Code

Audit of the source comments and user-facing docs for issue jdatcmd#291. 38 verified
defects fixed, each grounded in the contradicting code:

- comments describing the wrong function or return type (columnar.h:
  ColumnarComputeFullyDeletedGroups carried the all-visible function's comment)
- docstrings naming a non-existent "columnar." schema when the SQL functions
  live in "pgcolumnar." (export/import_arrow, export/import_parquet, read_parquet,
  parquet_schema, add/drop/read/reconstruct_projection, vm_selftest, GUC names)
- a security-relevant header claiming the FDW partition reader takes the value
  text literally, when the code percent-decodes it
- stale "stubbed for later phases" or "phase 1 uncompressed" file headers for
  callbacks and formats that are now implemented (tableam, reader, write_state,
  projection)
- a comment claiming VACUUM is a no-op when it marks the VM and retires groups
- an orphaned doc block for a deleted function (columnar_run_agg) and a vestigial
  decompressed-chunk-cache setup comment
- groupagg_max_groups documented as a plan-time gate when it is an execution cap
- two docs claims the code contradicts (bloom collation eligibility, Arrow type
  list) in ARCHITECTURE.md, and an empty column-cache section in configuration.md

One user-facing error message that named a non-existent object
("columnar.storageid_seq") is corrected to "pgcolumnar.storageid_seq".

Comments and docs only; no code logic changed. Five stale "require superuser"
claims this audit also found are fixed in the separate server-file docs PR. STE
and docs_style gates pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
@ChronicallyJD

Copy link
Copy Markdown
Collaborator Author

Review: the right half of #291, and the claims hold where I checked

The framing is the important thing here. #291 as filed is an ASD-STE100 vocabulary
audit over roughly 12,400 lines of comments, and I have argued that half is
unverifiable by construction: the vocabulary list is licensed, no gate can check
it, so any claim of compliance is unfalsifiable. This PR does the other half
instead, and that half is checkable: a comment that contradicts the code is a
claim with a truth value.

Verified mechanically

"Comments and docs only; no code logic changed." I test-merged into main and
diffed non-comment lines. Exactly two changes survive:

- errmsg("columnar.storageid_seq does not exist")
+ errmsg("pgcolumnar.storageid_seq does not exist")
- int maxGroups;  /* GUC cap (planner guard) */
+ int maxGroups;  /* GUC cap enforced at execution (columnar_groupagg_lookup) */

The first is the corrected object name the body discloses; the second is a
trailing comment. So the claim holds.

Worth noting the raw git diff main pr336 looks alarming: it shows
pexport_remove_outputs being deleted, i.e. #333's cleanup. That is the tip-diff
artifact, not a revert. The branch is based on 8760182, which predates #333; the
merge is clean and pexport_remove_outputs survives it. Flagging because the diff
reads like a revert and is not one.

Spot-checked, 3 of the 38

I did not verify all 38. Three, chosen as the ones that would matter most if wrong:

  1. ColumnarComputeFullyDeletedGroups carrying the all-visible comment. Real,
    and worse than a mislabel: the comment says "no deletes (committed or
    in-progress)" on a function that computes fully deleted groups. Opposite
    meanings, and it also states the wrong return type. Anyone reading the header to
    decide which helper to call gets the wrong one.

  2. The FDW partition header contradicting the code. Real. Line 3334 says "The
    value text is taken literally ... that decoding is deliberately not done here",
    while the code percent-decodes and has explicit refusals for what encoding can
    smuggle through. This is the security-relevant one: a reader trusting the header
    would assume no decoding happens and reason about path handling accordingly.

  3. groupagg_max_groups documented as a plan-time gate. Real, and I verified
    this independently on Grouped vectorized aggregate (#289) #321 before this PR existed: with the cap at 100 and
    20,000 actual groups the node is still chosen at plan time and the query errors
    at execution. The old wording was wrong in both halves.

The categories the rest fall into are the checkable kind, and the dead columnar.
versus pgcolumnar. references are the same class of defect I hit myself in the
roadmap's Done table earlier: a reader copy-pasting gets "does not exist".

One thought for #291 itself

This PR is evidence for what I think the resolution of #291 should be: split it.
The contradiction sweep is finishable and gateable, and largely done here. The
vocabulary compliance is neither. Closing #291 on the strength of this without
saying so would leave the unverifiable half looking finished.

Nothing blocking from me.

@ChronicallyJD
ChronicallyJD merged commit 92ae8a5 into jdatcmd:main Aug 2, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant