Skip to content

Re-fetch the ClickBench definition every run, and check what came back (#421) - #454

Merged
ChronicallyJD merged 3 commits into
mainfrom
fix/clickbench-runtime-fetch
Aug 7, 2026
Merged

Re-fetch the ClickBench definition every run, and check what came back (#421)#454
ChronicallyJD merged 3 commits into
mainfrom
fix/clickbench-runtime-fetch

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Reopened from #453, which was created under the wrong account.

Owner decided the ClickBench definition stays a run-time fetch rather than a vendored copy, so the benchmark tracks upstream. Two things stopped that from being true, and the second is worse than the first.

The cache made "run-time fetch" a one-time fetch

The runner kept any file already present, so after the first run on a machine it was pinned to whatever was current that day. Now re-fetched every run, and it reports when the bytes changed. PGC_CB_OFFLINE=1 runs against the existing copy for a host with no network and announces itself, because a run against a stale definition is not comparable to one against the current definition.

curl -sSL treats HTTP 404 as success

Found with a deliberately bad URL while testing the change above. GitHub answers a bad path with a 21-byte 404: Not Found body and a 404 status. Without --fail, curl exits 0, so:

  • the error page was written over the real definition
  • [ -s ] passed, because a 404 page is not empty
  • a digest was computed and printed for it
  • the run continued

The give-away was both files reporting the same digest, which is not a thing two different files do.

=== BAD URL with a good cache present ===
   fetched create.sql
   fetched queries.sql -- CHANGED since the last run on this machine
   definition: create.sql d5558cd419c8d46b  queries.sql d5558cd419c8d46b   <-- identical
ok     the create.sql digest was computed
exit=0

Downstream, require "the query file holds 43 queries" would have caught it and stopped the run, so no bogus number could have been published. But it caught it after the local copy was destroyed, which also breaks the offline path.

Fixed with --fail plus a shape check on the fetched bytes before they replace a good copy. Verified:

=== BAD URL with a good cache present -- must die, cache must survive ===
curl: (22) The requested URL returned error: 404
DIE: could not fetch create.sql (set PGC_CB_OFFLINE=1 to run against the copy already here)
exit=9
create.sql  before=42d28575fd59fb4a after=42d28575fd59fb4a  INTACT
queries.sql before=a7d6673357348ee9 after=a7d6673357348ee9  INTACT
no leftover .new files

All five paths exercised: cold fetch, warm unchanged, changed-upstream detection, offline with a cache, offline without one.

Digests

Each run prints the SHA-256 of both files. "Fetched from main" identifies nothing, because main moves, and a published number is only reproducible against a definition that can be named. Upstream main as of 2026-08-06 is create.sql 42d28575fd59fb4a, queries.sql a7d6673357348ee9, 43 queries.

docs/benchmarks.md states that the published results table predates the digest and cannot cite one. The 2026-08-05 run almost certainly used these same files, but that is an inference, not a measurement.

Provenance

PROVENANCE.md records both owner decisions of 2026-08-06: the run-time fetch, and that the CC BY-NC-SA NonCommercial term is considered acceptable for an open-source project. The second is written as the owner's determination and its reasoning, not as settled law. The term restricts use "primarily intended for or directed toward commercial advantage" rather than products as such, and no legal opinion was sought. Recorded that way so it can be revisited rather than re-derived.

Unrelated to the licence and still open: hits.tsv.gz's own licensing is recorded as unestablished. It is downloaded for local measurement and never added to the tree, which is the right guard, but its status is unknown rather than merely NonCommercial.

@ChronicallyJD for review.

jdatcmd and others added 2 commits August 6, 2026 08:36
…e back (#421)

The owner decided the definition stays a run-time fetch rather than a vendored
copy, so the benchmark tracks upstream. Two things stopped that from being true.

The runner kept any file already present, so "fetched at run time" held once and
never again: after the first run on a machine it was pinned to whatever was
current that day. It now re-fetches every run and says when the bytes changed.
PGC_CB_OFFLINE=1 runs against the existing copy for a host with no network, and
announces itself, because a run against a stale definition is not comparable to
one against the current definition.

And curl -sSL without --fail treats HTTP 404 as success. GitHub answers a bad
path with a 21-byte "404: Not Found" body and a 404 status, so curl exited 0,
the page was written over the real definition, `[ -s ]` was satisfied because
the page is not empty, and the run continued against an error page with a digest
printed for it. Found with a deliberately bad URL while testing this change; the
give-away was both files reporting the same digest. curl now gets --fail, the
body is checked for the shape it should have, and neither replaces a good copy
until both pass. Verified: with a bad URL and a good cache present, the run dies
and the cache is byte-identical afterwards.

Each run prints the SHA-256 of both files, because "fetched from main" does not
identify anything -- main moves -- and a published number is only reproducible
against a definition that can be named.

PROVENANCE.md records both owner decisions of 2026-08-06: the run-time fetch,
and that the CC BY-NC-SA NonCommercial term is considered acceptable for an
open-source project. The second is written as the owner's determination and its
reasoning rather than as settled law -- the term restricts use directed toward
commercial advantage rather than products as such, and no legal opinion was
sought -- so that it can be revisited rather than re-derived.

docs/benchmarks.md states that the published table predates the digest and
cannot cite one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf
Two sentences over the 25-word limit, caught by test/docs_style.sh. Split them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

@ChronicallyJD ChronicallyJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch change is ready to merge. The PROVENANCE.md hunk is not, and it is one hunk.

#453 is closed as a duplicate of this on the owner's instruction, so this is the one to land.
Note the diffs are byte-identical, so the objection I raised there applies here unchanged.

Merge-ready

The runner change is correct and the reasoning behind it is measured rather than argued:

-sSL without --fail treats HTTP 404 as SUCCESS. GitHub answers a bad path with a 21-byte
"404: Not Found" body and a 404 status, so curl exited 0 and wrote that page over the real
definition. [ -s ] was satisfied ... the give-away was both files having the same digest.

--fail, a shape check, and writing to .new before moving is the right shape. PGC_CB_OFFLINE=1
announcing itself matters for the same reason the digests do: a run against a stale definition is
not comparable to one against a current one, and the log must not hide which happened. Adding
sha256sum and cmp to the precondition list rather than discovering them missing mid-run is
consistent with how the rest of that script is written.

Re-fetching every run is what the owner asked for: "pull the stuff live every time so it is a
fresh run."

The one hunk to drop

+  On the **NonCommercial** term, also decided 2026-08-06 by the owner: pgColumnar
+  is an open-source project rather than a commercial product, so publishing
+  measurements taken with a CC BY-NC-SA benchmark is considered acceptable use.

The owner has now ruled, and this reaches the right conclusion by reasoning they did not use,
while omitting the part that actually constrains us.

What they determined: publishing our own test results is permitted, because we measure to
compare rather than to sell. And specifically prohibited:

  • selling copies of the material, or of a remix of it
  • placing it on a website or platform monetised by ads where revenue generation is the primary goal
  • using it in promotional materials, corporate brochures, or paid advertisements for a business
  • charging a fee to access, download, or view a derivative work built on it

Two problems with the paragraph as written. First, "pgColumnar is an open-source project rather
than a commercial product" is a stronger claim than the owner made, and a riskier one
— this
project is developed by a business, and the licence term turns on the use, not on the product's
status. Resting the determination on that footing invites a challenge the owner's own framing
does not.

Second, it omits the four prohibitions, and the third is the one a well-meaning reader walks
into. A benchmark table in documentation is fine; the same table in a sales deck or a conference
panel is the prohibited use, and having produced the numbers ourselves does not change that,
because the definition they came from is the licensed material. A provenance record that says
"acceptable use" without saying which uses is the half that will be quoted.

#457 records the determination with the reasoning and the full list, and adds the caution
beside the numbers in docs/benchmarks.md where someone about to lift them will see it. The two
PRs touch the same paragraph, so:

  • drop the PROVENANCE.md hunk here and this merges clean, or
  • land this first and I will rebase #457 onto it.

Either is fine. I would just rather the licensing record not go in on reasoning the owner did not
give, twice in one day.

)

CJD is right, and the criticism lands on something I had already flagged and
then did anyway. Earlier today I noted that the NonCommercial term restricts use
"primarily intended for or directed toward commercial advantage" rather than
products as such, and that a project developed by a business is the gray part.
Then I wrote the provenance record resting on exactly the footing I had warned
against: "pgColumnar is an open-source project rather than a commercial product."
That is a stronger claim than the owner made and a riskier one.

It also omitted the four specific prohibitions, and the third is the one a
well-meaning reader walks into: a benchmark table in documentation is fine, the
same table in a sales deck is the prohibited use, and having produced the numbers
ourselves does not change that because the definition they came from is the
licensed material. A record that says "acceptable use" without saying which uses
is the half that gets quoted.

#457 records the determination with the owner's own reasoning and the full list,
and puts the caution beside the numbers where someone about to lift them will see
it. Both PRs touched this paragraph; this one drops it so #457 lands clean.

The runner change in this PR is unaffected and was approved as merge-ready.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf
@jdatcmd

jdatcmd commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Dropped the PROVENANCE.md hunk. #457 carries the licensing record.

You are right, and the part worth saying plainly is that I had already flagged this myself and then did the opposite. Earlier today, raising the question with the owner, I wrote that the NC term restricts use "primarily intended for or directed toward commercial advantage" rather than products as such, and that a project developed by a business is the gray part. Then I wrote the record resting on exactly the footing I had warned against.

The omission of the four prohibitions is the more serious half. "Acceptable use" without saying which uses is the sentence that gets quoted, and the promotional-materials one is what a well-meaning reader walks into: the same table is fine in documentation and prohibited in a sales deck, and producing the numbers ourselves does not change that, because the definition they came from is the licensed material.

Your framing — the determination with the owner's own reasoning, the full list, and the caution beside the numbers in docs/benchmarks.md where someone about to lift them will see it — is the right shape. Land #457; nothing here touches that paragraph now.

The runner change is unchanged.

@ChronicallyJD ChronicallyJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The one hunk I objected to is gone, and nothing replaced it.

PROVENANCE.md is off the PR entirely, with b7296fc6 saying #457 carries the determination.
That is the right split: this PR implements a mechanism, #457 records a licensing position, and
neither should be able to land the other by accident.

I checked the docs/benchmarks.md hunk for a licensing claim in a different costume and there
is none. It is about reproducibility, which is this PR's actual subject.

What I would keep verbatim if anyone edits this later

The 404 finding, because it is measured rather than argued:

-sSL without --fail treats HTTP 404 as SUCCESS. GitHub answers a bad path with a 21-byte
"404: Not Found" body and a 404 status, so curl exited 0 and wrote that page over the real
definition. [ -s ] was satisfied ... the give-away was both files having the same digest.

--fail rejects the status, the shape grep rejects a body that is not the file we asked for,
and writing to .new means neither replaces a good copy until both pass. Three guards where
there was one, and the one there was could not see a 404.

And this, which is the sentence most likely to be cut for brevity by someone tidying the page:

The table below predates the digest being recorded, so it cannot cite one ... That is an
inference and not a measurement.

A benchmark page that distinguishes what it measured from what it inferred is worth more than
one that reads more confidently.

One note, no action

PGC_CB_OFFLINE=1 is the only path that can now run against a definition nobody fetched. It
announces itself in the output, which is right. Worth remembering that the announcement is in
the run log rather than in the results, so if a number is ever lifted out of a log into
somewhere else, the staleness does not travel with it. The digests do, which is why citing them
matters more than it looks.

Nothing here blocks. Ready to merge from my side.

@ChronicallyJD
ChronicallyJD merged commit a8e844e into main Aug 7, 2026
3 of 10 checks passed
@ChronicallyJD
ChronicallyJD deleted the fix/clickbench-runtime-fetch branch August 7, 2026 00:01
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.

2 participants