Skip to content

cluster() is not numeric-only, and the error already said so (#827 follow-up) - #832

Merged
OffgridwithJD merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:docs/827-cluster-is-not-numeric-only
Aug 29, 2026
Merged

cluster() is not numeric-only, and the error already said so (#827 follow-up)#832
OffgridwithJD merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:docs/827-cluster-is-not-numeric-only

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Documentation only. Nothing executes differently. Follow-up to the finding I left on
#827, which I had not filed.

Claiming this so it cannot collide with your bug sweep — it touches only the two
extension scripts, one code comment, and the CHANGELOG.

The defect

COMMENT ON FUNCTION pgcolumnar.vacuum_sorted calls the alternative "the
numeric-only Z-order cluster()". Wrong in both directions.

cluster_type_supported() (src/columnar_vacuum.c) takes boolean, smallint,
integer, bigint, real, double precision, date, timestamp and
timestamptz. Several are not numeric. And numeric itself is absent — NUMERICOID
appears nowhere in the file — so the one type the sentence names is precisely the one
cluster() refuses.

The extension already contradicted it. A rejected column raises:

errhint: Z-order clustering supports integer, date/time, boolean, and
floating-point columns.

A user who trips the gate is told the truth. A user who reads the catalog first is
not. The comment now uses the errhint's own wording, so there is one description of
this rule instead of two that disagree.

This is user-facing: COMMENT ON FUNCTION lands in pg_description and is what
\df+ prints — exactly where someone choosing between vacuum_sorted and
cluster() looks.

Measured, not read

A positive control is included so the deny arms are not vacuous. 20,000-row table,
PG 18:

call result
vacuum_sorted('np','n') numeric accepted
vacuum_sorted('np','t') text accepted
cluster('np','i') int accepted — control
cluster('np','n') numeric rejected: column "n" of type numeric cannot be used as a clustering key
cluster('np','t') text rejected

The error a user gets for numeric refutes the word "numeric-only" on its own.

Why the upgrade script is half the fix, and not optional

native_upgrade_converge hashes obj_description(p.oid,'pg_proc') for every
function in the schema. The 1.0-alpha2 -> 1.0-alpha3 script did not re-issue this
comment, so correcting only the full script makes a fresh install and an upgraded one
disagree — and the suite says so. Run in this order:

arm native_upgrade_converge
unmodified main 8 of 8 PASSED
full script corrected only FAILED — both the 1.0-alpha and 1.0-alpha2 paths
+ upgrade script re-issues 8 of 8 PASSED

The middle row is the point: the gate catches the half-fix. Re-issuing in that script
is the established pattern — set_options, expire, parallel_copy and
sort_status already do it there. The upgrade copies the literal verbatim from the
full script, so the two cannot drift.

The third site

The same sentence sat in src/columnar_vacuum.c above pgcolumnar_vacuum_sorted.
That is where the catalog string came from, so it is corrected too. Leaving it would
leave the input to the defect in place.

Left alone deliberately: test/fixtures/pgcolumnar--1.0-alpha{,2}.sql and
pgcolumnar--1.0-alpha--1.0-alpha2.sql are faithful snapshots of what shipped, and
the alpha2->alpha3 re-issue corrects the upgrade path anyway. CHANGELOG.md:2422 is
a historical entry.

Gate

arm result
preflight, PG 15/16/17/18/19 built 5 of 5, 0 warnings each
matrix PG18 231 ran, 2 skipped, ALL PASSED
matrix PG19 233 ran, 0 skipped, ALL PASSED
docs_style 9 of 9
native_upgrade_converge 8 of 8
harness_selftest PASS on both arms

Those tallies are identical to main at 808cd46 measured on the same box today,
which is how I know the change is inert.

The first gate run failed, and it is worth recording rather than quietly
re-running: docs_style reported
CHANGELOG.md carries no em or en dash: got [2] want [0]. Both dashes were mine, in
the entry above. The house rule is deliberate and documented in the suite. Fixed
before pushing, so this branch has never been red, but it would have been the first
thing CI told you.

…ommandprompt#827 follow-up)

COMMENT ON FUNCTION pgcolumnar.vacuum_sorted describes the alternative as "the
numeric-only Z-order cluster()". That is wrong in both directions, and it is in
the shipped v1.0-alpha2, in the full script and in the upgrade script, so \df+
prints it to a user today.

cluster_type_supported() takes boolean, smallint, integer, bigint, real, double
precision, date, timestamp and timestamptz. Several of those are not numeric. And
numeric itself is absent: NUMERICOID appears nowhere in the file, so the one type
the sentence names is precisely the one cluster() refuses. Measured rather than
read, with a positive control so the deny arms are not vacuous: on a 20,000 row
table, vacuum_sorted accepts numeric and text; cluster() accepts int, and rejects
numeric with "column n of type numeric cannot be used as a clustering key" and
text likewise.

The extension already contradicted the sentence. A rejected column raises an
errhint reading "Z-order clustering supports integer, date/time, boolean, and
floating-point columns". A user who trips the gate is told the truth; a user who
reads the catalog first is not. The comment now uses the errhint's wording, so
the two agree and there is one description of this rule rather than two.

The same sentence sat in src/columnar_vacuum.c as a code comment above
pgcolumnar_vacuum_sorted. That is where the catalog string came from, so it is
corrected too; leaving it would leave the input to the defect in place.

The 1.0-alpha2 -> 1.0-alpha3 upgrade re-issues the comment, as set_options,
expire, parallel_copy and sort_status already do in that script. That half is not
optional. native_upgrade_converge hashes obj_description(p.oid,'pg_proc') for
every function in the schema, so correcting only the full script makes a fresh
install and an upgraded one disagree. Verified in that order: 8 of 8 on
unmodified main; with only the full script corrected it FAILS on both the
1.0-alpha and 1.0-alpha2 paths; 8 of 8 again once the upgrade script carries it.

The changelog entry that this branch used to carry has moved to the changelog
branch. Both edited the same `### Fixed` section from different bases, so
whichever merged first left the other conflicting, while GitHub reported both
mergeable because it compares each against main rather than against the other.

Nothing executes differently. Historical scripts and the alpha/alpha2 fixtures are
faithful snapshots of what shipped and are left alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KL9BxvtjERL34H1XV8BND2
@OffgridwithJD
OffgridwithJD force-pushed the docs/827-cluster-is-not-numeric-only branch from 8414061 to 4cfa7bb Compare August 29, 2026 22:50
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Aug 29, 2026
Seven defects were found and fixed for alpha3 and none of them reached
CHANGELOG.md. Four of the seven are present in the shipped v1.0-alpha2, so a user
deciding whether to upgrade could not learn about them from the one file written
for that purpose.

Each entry states plainly whether a released version is affected, because that is
the part a reader acts on and it is not the same answer for all seven. Checked
against the tag rather than assumed:

  affects v1.0-alpha2:  commandprompt#838 index entries for live rows destroyed
                        commandprompt#842 a backward fetch answered with forward rows
                        commandprompt#844 an encoded NUL defeating the Iceberg traversal guard
                        commandprompt#827 vacuum_sorted's comment calling cluster() numeric
                             only, which \df+ prints
  this cycle only:      commandprompt#834 Natts_options sized three stack arrays short
                        commandprompt#836 the equality preimage at an infinity
                        commandprompt#840 i128sum surviving a rescan

For the second group the constants, the function and the field respectively do
not exist at v1.0-alpha2, so no released version carries them. Saying so is
useful rather than pedantic: an entry that does not distinguish the two invites a
reader to go looking for a release that was never affected.

The commandprompt#827 entry was previously carried by the branch behind commandprompt#832, which also
changes the SQL scripts and a source comment. Both branches edited the same
`### Fixed` section from different bases, so whichever merged first left the other
conflicting, while GitHub reported both mergeable because it compares each against
main and not against the other. The changelog now lives here alone and commandprompt#832 keeps
only the code change.

Each entry carries the measurement that established the defect, in the form the
issue recorded it, so the entry can be checked rather than believed.

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

Copy link
Copy Markdown
Collaborator Author

Force-pushed: rebased onto main a45b4cf2 and the changelog entry has moved out of
this branch
, to the changelog PR.

The reason is worth recording. This branch and the changelog branch both added to the
same ### Fixed section of CHANGELOG.md, from different bases. GitHub reported both
as MERGEABLE/CLEAN, which is true and misleading: it compares each against main
independently and cannot see that they collide with each other. Tested both orders in a
worktree and each one conflicted on the second merge:

order A: changelog then this   ->  CONFLICT: CHANGELOG.md
order B: this then changelog   ->  CONFLICT: CHANGELOG.md

So this branch is now the code change alone: the two SQL scripts and the source comment.
Re-tested after the split, both orders merge cleanly.

@OffgridwithJD
OffgridwithJD merged commit 2e963c0 into commandprompt:main Aug 29, 2026
12 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