Conversation
c1bf13a to
654543d
Compare
jdatcmd
left a comment
There was a problem hiding this comment.
Adversarial review of 654543d, built and run in pgcolumnar-dev on PG 18.4. The design holds and the three-state max_upper is right. One arm produces silently wrong answers, and I have a reproduction, an isolation and a candidate fix.
The pruning compares under a different collation than the operator it prunes for
elemCollation is taken from the ELEMENT type's cache entry, in both halves:
src/columnar_write_state.c:480 def->elemCollation = etce->typcollation;
src/columnar_reader.c:792 p->elemCollation = etce->typcollation;
A range type's comparisons do not use the element type's default collation. They use the collation the range type was DECLARED with, which the type cache carries separately as rng_collation. Measured on PG 18.4:
CREATE TYPE textrange_c AS RANGE (SUBTYPE = text, COLLATION = "C");
element text typcollation | default
range typcollation | (none)
range rngcollation | C
So for a range over a collatable subtype the two orders can disagree, and here they do:
range_cmp (C): [B,Z] < [a,z] -> t
element cmp under text default: 'B' > 'a' -> t
native_range_excludes takes unitLower from the stored minimum, which was selected under range_cmp (collation C), and then compares it against the probe's upper bound under elemCollation (collation default). The unit's least lower bound is B under C; under the default collation B sorts above a, so the "every value starts at or after the probe ends" test fires on a unit that does contain matches.
The reproduction
CREATE TYPE textrange_c AS RANGE (SUBTYPE = text, COLLATION = "C");
CREATE TABLE r_col (id int, span textrange_c) USING pgcolumnar;
CREATE TABLE r_heap (id int, span textrange_c);
INSERT INTO r_col VALUES (1, '["B","Z"]'::textrange_c), (2, '["a","z"]'::textrange_c);
INSERT INTO r_heap SELECT * FROM r_col;
SELECT count(*) FROM r_heap WHERE span && '["0","a"]'::textrange_c; -- 2
SELECT count(*) FROM r_col WHERE span && '["0","a"]'::textrange_c; -- 0Both rows genuinely overlap the probe. The heap says 2. The columnar table says 0.
Isolated three ways, because a wrong count has many possible owners
The same columnar table, with the custom scan turned off, returns 2. So the rows are stored and readable, and it is the scan's pruning that drops them:
SET pgcolumnar.enable_custom_scan = off;
SELECT count(*) FROM r_col WHERE span && '["0","a"]'::textrange_c; -- 2
The same query and data against origin/main, installed into the same cluster and the same database:
| build | Columnar Pushed-Down Filters |
heap | columnar |
|---|---|---|---|
origin/main (133c3fb) |
0 | 2 | 2 |
| this branch (654543d) | 1 | 2 | 0 |
Only the installed .so differs. This is a regression introduced by the branch, not a pre-existing defect the branch exposes.
A control that says the collation is the cause. The identical shape with the range declared at the DEFAULT collation agrees, and so does the C-collated type on a probe whose answer the mismatch cannot change:
| case | heap | columnar |
|---|---|---|
textrange_d (default collation), same data, same probe |
1 | 1 |
textrange_c, probe ["0","zzz"] |
2 | 2 |
textrange_c, probe ["0","a"] |
2 | 0 |
The existing collation gate does not cover this
pgcolumnar_clause_to_scankey already refuses a clause whose collation differs from the column's, and its comment says that refusing is what stops a differently ordered comparison from wrongly skipping a group. For a range column that gate is inert: the range type's typcollation is 0, so attcollation is 0, and op->inputcollid for && is 0 as well. The gate compares 0 with 0 and passes the clause through. The collation that matters is not the one it reads.
Candidate fix, tested
Taking the collation from the RANGE type's cache entry in both halves:
- def->elemCollation = etce->typcollation;
+ def->elemCollation = rtce->rng_collation;
- p->elemCollation = etce->typcollation;
+ p->elemCollation = rtce->rng_collation;
Rebuilt and re-run against the same database:
| case | heap | columnar |
|---|---|---|
| rows rewritten after the fix | 2 | 2 |
| the rows written BEFORE the fix, read after it | 2 | 2 |
textrange_c, probe ["0","zzz"] |
2 | 2 |
textrange_d control |
1 | 1 |
The second row is luck rather than a property: this probe is decided by the lower-side test, which reads minimum and is therefore independent of the stored max_upper. A max_upper recorded under the wrong collation can still be the wrong element, so the fix is not a repair for data already written. Nothing has shipped this column, so that costs nothing here, but a rebuild would be the honest advice if it had.
I am not proposing the fix as final. rng_collation is what range_cmp itself uses, so it is the right source, but you own this code and may prefer to carry the collation from the scan key.
How reachable is it
Not reachable with any built-in range type. tstzrange, daterange, int4range, int8range and numrange are all over non-collatable subtypes, so rng_collation is 0 and both expressions agree. It needs a user-defined range over a collatable subtype declared with a collation other than the subtype's default, which is the second argument of CREATE TYPE ... AS RANGE.
So: latent, not live, and silently wrong when it fires. I am requesting changes rather than noting it because the failure mode is a missing row rather than an error, and because the fix is two lines.
Second item: the suite ships in one harness
test/range_pruning.sh is 207 new lines with 23 ledger rows, and there is no test/pytest/test_range_pruning.py and no TESTS.md section. The owner's standing rule is that a test ships as both halves in the same change; one harness only is not finished. Nothing in the gate refuses it, which is why I am saying it here.
If you add the twin, the case above is worth an arm in it. It is a documented limitation either way: the page now says which predicates prune, and "a range type declared with a non-default collation" belongs in that list if the fix is deferred rather than taken.
What I checked and found sound
| property | how |
|---|---|
| EMPTY vs unbounded-above are distinguished | both halves ask isempty and upper_inf rather than testing the bound for NULL, which is the max(upper()) trap |
a NULL constant cannot reach DatumGetRangeTypeP |
con->constisnull is refused above the range branch |
@> is not confused with range-range containment |
RTContainsElemStrategyNumber plus an explicit con->consttype == rngelemtype->type_id |
&& is taken in either operand order, @> only with the var on the left |
varOnLeft is checked for one and not the other, correctly |
| the operator is identified by opfamily membership, not by name | a user-defined && outside the range GiST family is left alone |
| NULL values do not reach the accumulator | the block sits inside the non-null arm of if (nulls[c]) |
| the chunk fold treats unbounded as absorbing | one unbounded vector makes the chunk unbounded, in both directions |
ignoring inclusivity in max_upper is safe |
every upper-side prune is a strict inequality, or an equality the probe's own bound settles |
| the per-vector buffers coexist across the fold | chunkMaxUpper holds a pointer into a col that the chunk list keeps alive |
The heap differential oracle is the right instrument and is what let me state the row counts above without arguing about them.
…andprompt#1144) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…1144) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…t collide with btree strategies (commandprompt#1144) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…ented zero (commandprompt#1144) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…ommandprompt#1144) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…ommandprompt#1144) The entry states the three states max_upper carries and why two would not do, gives the measured group counts for the clustered and scattered fixtures, and records that the scattered zero is the documented outcome rather than a shortfall. It also names both scripts the column is added in, because a fresh install and an upgraded one have to converge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…rs (commandprompt#1144) Registering a suite without seeding it raises suites_not_covered, which the gate may only see fall. Seeding in the same change keeps it at 249: registered goes up by one and covered goes up by one. THE MAJOR SET IS MEASURED, NOT COPIED FROM THE NEIGHBOURING ROWS. range_pruning.sh was run against 15.18, 16.14, 17.6, 18.4 and 19beta2, 23 passed + 0 failed on each, and the five logs were merged in a SINGLE call, so every row carries 15;16;17;18;19 because that is what was observed. checks_never_observed_red was re-derived by counting on this tree, never by adding 23 to the previous value: awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l -> 1476 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…e's (commandprompt#1144) A range type is declared with a collation, which the type cache carries as rng_collation. Both halves of this change read the ELEMENT type's typcollation instead, which is a different value: for CREATE TYPE tr AS RANGE (SUBTYPE = text, COLLATION = "en_US.utf8") the range collates en_US.utf8 while text's typcollation is `default`. The writer then summarised max_upper under one ordering and the reader pruned under another, and the scan MISSED ROWS -- a wrong answer rather than an error. Reproduced against the heap, which answers the same query: overlap ["A","C"] heap 1200 columnar 0 overlap ["B","M"] heap 1200 columnar 0 NOT REACHABLE WITH A BUILT-IN RANGE TYPE, which is why every arm already in this suite passed with the defect present: tstzrange, daterange, int4range, int8range and numrange are over non-collatable subtypes, so rngcollation and typcollation are both 0 and agree however the value is computed. It needs a user-defined range over a collatable subtype with an explicit non-default COLLATION. The eight new arms fail on the unfixed source for the intended reason (three of them, at 0 rows against the heap's 1200) and pass on the fixed one. The collation is DISCOVERED rather than assumed: a box with no locale ordering differently from the database default records the arm as skipped rather than passing over a question it cannot pose. Found by @jdatcmd reviewing this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…ms owe (commandprompt#1144) The shell suite gained eight arms for the declared-collation defect, and the recent convention is that a new suite carries a pytest twin: of the last eleven shell suites added before this one, nine have one. test/pytest/test_range_pruning.py asserts the engine's own counters as typed JSON, the heap oracle on overlap and containment, the documented scattered zero, and the collation property on a user-defined range type it creates for the purpose. Own fixture, own row counts, own observations; neither file reads or runs the other. DECLARED INCOMPLETE RATHER THAN LISTED COMPLETE. The shell suite additionally pins the three states of max_upper and its own fixture geometry by name, and those arms are not ported, so the pair does not grade to zero. `INCOMPLETE` carries the reason, which is what stops a declared gap becoming a silent exemption. THE COLLATION IS DISCOVERED FROM pg_collation, not assumed and not found by catching an exception: naming a collation that does not exist raises, and one failed statement makes psycopg raise for every later one, so a try/except would both hide a real failure and poison the rest of the test. ICU collations are listed even in a build without ICU and refuse to be used, so the query excludes them. A FIXTURE THAT DID NOT DO WHAT IT SAID. set_options refuses a stripe_row_limit below 1000 and psql_run does not abort the suite, so the collation fixture had been keeping the default geometry while its own line claimed 200. Both harnesses now pass a legal value and the row count moves with it. Ledger: the eight new arms were run against 15.18, 16.14, 17.6, 18.4 and 19beta2, 31 passed + 0 failed on each, and the five logs merged in a single call so every row carries the major set that was observed. checks_never_observed_red re-derived by counting on this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
…#1144) A fixture that declares COLLATION "C" against a C default changes no ordering, so it is green on a correct tree and on a broken one. The arm discovers a discriminating collation and records itself unrunnable when there is none, but a reader needs to know that before concluding a quiet box means there is nothing to find. Raised by @jdatcmd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
359d6fd to
b61690d
Compare
jdatcmd
left a comment
There was a problem hiding this comment.
Re-review of b61690d. The collation defect is fixed and I confirmed it with my own reproduction rather than reading the diff. Built in pgcolumnar-dev on PG 18.4, .so f6ac1572944a, in a database whose default collation is en_US.UTF-8.
CREATE TYPE textrange_c AS RANGE (SUBTYPE = text, COLLATION = "C");
INSERT INTO r_col VALUES (1, '["B","Z"]'), (2, '["a","z"]');
SELECT count(*) FROM r_heap WHERE span && '["0","a"]'::textrange_c; -- 2
SELECT count(*) FROM r_col WHERE span && '["0","a"]'::textrange_c; -- 2
It read 0 on 654543d and reads 2 here. And it is not passing by not pruning, which is the way this fix could have been wrong:
Custom Scan (PgColumnarScan) on r_col
Columnar Pushed-Down Filters: 1
Both controls are unmoved: the ["0","zzz"] probe that always agreed is still 2 and 2, and the default-collated textrange_d shape is still 1 and 1. Both sites take rtce->rng_collation now.
Not approving yet only because suites (PG 17) and suites (PG 18) are still IN_PROGRESS. Everything else is SUCCESS.
One collision with #1199, and git does catch it
Both this branch and #1199 add TESTS.md section 75, because both derived max(existing) + 1 against a main whose highest was 74. I composed the two rather than predicting:
CONFLICT (content): Merge conflict in test/pytest/TESTS.md
5804 <<<<<<< HEAD
5805 ## 75. test_range_pruning.py: a range prunes on overlap, containment, ...
5838 =======
5839 ## 75. test_docs_upgrade_chain.py: the documented upgrade chain must be ...
5971 >>>>>>> docs/1197-the-documented-upgrade-chain
Both ## 75. lines sit inside the conflict region, so git speaks. I expected a silent duplicate and was wrong; the two sections are adjacent at the end of the file, which is the positional case where git does notice.
So the only thing to get right is the resolution: whichever lands second renumbers to 76, in the heading, in the TOC entry and in the anchor, and checks the pairing rather than the two sizes. Keeping both at 75 is the resolution the conflict invites and it is the wrong one. expected_tests.txt also merged with a conflict there, which is the safe direction.
Nothing else overlaps. Your docs/limitations.md change is at line 652 and #1199's is at 113, and your added prose contains neither previously shipped version nor reaches `X`, so #1199's claim-sentence count stays at one in the compose. I checked that rather than assuming it.
The page now says something that is not true for a range column
docs/limitations.md already has a section for exactly this, and this change makes it wrong:
## Skipping and collation
The collation of the comparison must match the collation of the column. That is
the collation that put the stored minimum and maximum in order.
For a range column neither sentence holds. The column's attcollation is 0, because a range type's typcollation is 0 and the declared collation lives in pg_range.rngcollation. The collation that ordered the summary is that one, which is precisely what your fix now reads. And the gate that the paragraph is describing -- op->inputcollid != attcollation in pgcolumnar_clause_to_scankey -- compares 0 with 0 for a range and passes the clause through, so it is not the protection for this case either.
The suite header at lines 207-219 explains all of this well. It is the one place a user will not look. A reader with a textrange declared COLLATION = "C" reads that section, checks their column's collation, finds nothing, and concludes they are outside the rule. Two sentences in that section, naming rngcollation for ranges, closes it.
Not blocking: the code is correct and the pruning is safe. It is a documentation gap in a section that already exists.
Census
cluster_tests 466 and guard_tests 398 derived through --pgc-expect-tests rather than through a counter is the right instrument, especially after this morning. Note that #1199 takes guard_tests to 402; your 398 is unchanged from your base, so a three-way merge takes 402 automatically and neither side needs to act.
checks_never_observed_red 1492 will conflict against whatever main holds by then, which is the half of that file worth having.
What I checked and found sound
| property | how |
|---|---|
| the fix is in both halves | columnar_write_state.c:490 and columnar_reader.c:802, both rtce->rng_collation |
| the pytest twin exists and is declared | test_range_pruning.py, and INCOMPLETE["range_pruning"] carries its reason rather than being listed complete |
| the declared reason is specific | it names what is ported and what is not, rather than saying "partial" |
| the scattered zero is asserted rather than skipped | the page states it and the suite arms it |
| old data cannot be pruned wrongly | rows written before max_upper existed read as NULL, which returns false |
The two-way underpowered probe you describe is the better story than my one-way one, and it is the reason the suite should say which database collation the arm needs: C.UTF-8 as a default makes COLLATION = "C" a no-op and the fixture green on a correct tree and a broken one alike.
jdatcmd
left a comment
There was a problem hiding this comment.
Approving b61690d. The two suites legs have landed and the run is 15 of 15 with nothing failing, CLEAN, and the check name set is identical to the last merged PR's head.
This lifts my CHANGES_REQUESTED from 654543d. The evidence is in the comment above and I am not restating it, except for the one line that matters: my reproduction reads 2 and 2 here against 2 and 0 on the previous head, with Columnar Pushed-Down Filters: 1 still in the plan, so the arm is not passing by giving up on pruning.
Two things to carry into the merge, neither a change request.
TESTS.md section 75 collides with #1199, which derived the same number against the same main. Git conflicts on it -- I composed the two and both ## 75. headings land inside the conflict region -- so whoever goes second renumbers to 76 in the heading, the TOC entry and the anchor, and checks the section/TOC pairing rather than the two counts.
The ## Skipping and collation section of docs/limitations.md now describes a rule that does not hold for a range column: attcollation is 0 there and the collation that ordered the summary is pg_range.rngcollation, which is what this change reads. Two sentences in a section that already exists. I would rather see it here than in a follow-up, but it is not worth holding a verified fix for.
…it did (#1144) `docs/limitations.md` tells a user that a pushed-down filter drives skipping only when the comparison collation matches the COLUMN's collation, and that this is the collation that ordered the stored minimum and maximum. Range pruning landed in #1196 and made that wrong for a range column, in a section that change did not touch. Measured rather than reasoned: attcollation of a text column declared COLLATE "en_US.utf8" 12378 attcollation of a range column over the same subtype 0 typcollation of the range type 0 pg_range.rngcollation "en_US.utf8" So a range column has no collation for a comparison to match. The ordering comes from the collation the range TYPE was declared with, and that is the value the scan reads when it decides whether to skip a unit. The `op->inputcollid != attcollation` gate is not the protection here either, for two independent reasons. The range path sets PGC_SK_RANGE and RETURNS before reaching it, and both sides are 0 for a range predicate, so the comparison would pass the clause through even if it ran. Reported by @jdatcmd, who also caught that this belongs in its own change rather than folded into another. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
Closes #1144.
&&and@>on a range column read every chunk group. A zone map recordedminimumandmaximumonly, and those are the range type's own btree ordering, which sorts by lower bound and then upper bound. The lexicographically largest range is not the one reaching furthest right: a chunk holding[1,2)and[3,100)has the samemaximumas one holding[1,2)and[3,4). Neither predicate can be answered from that.What it measures
200,000 rows, 20 chunk groups,
tstzrangeover three months, counted from the scan's own counters rather than inferred from a plan:The scattered zero is the documented outcome, not a shortfall. Every group holds a bound near the maximum, so no summary can exclude any of them.
docs/limitations.mdnow says which predicates prune and which do not, and the suite asserts the zero rather than skipping the case.What it adds
zone_mapgainsmax_upper, the greatest upper bound in the unit, as abyteawith three states. Two would not do:Every row written before this column existed reads as NULL, so an upgraded install prunes nothing until its data is rewritten and never prunes wrongly. An unbounded range is a different fact from an absent summary: the field is present and understated rather than missing, and a two-state design cannot say that.
PGC_SK_RANGEcarries the predicate to the reader, withPGC_RANGE_OVERLAPandPGC_RANGE_CONTAINS_ELEMnumbered 101 and 102 so they cannot collide with a btree strategy number.Proofs
A heap differential oracle, not a self-comparison: every query is run against a heap table built from the same generator, and the two row sets are compared. An earlier version of this suite compared a query to itself; that arm is gone.
Mutation. Removing the pruning makes 5 arms red on the clustered path and 2 on the containment path. The suite is sensitive to the thing it claims to test.
Two crashes found and fixed on the way here, both recorded in the history:
native_value_satisfiescalled a zeroedcmpFnfor a range predicate, which segfaulted the backend; andPGC_RANGE_OVERLAPwas first numbered 1, colliding withBTLessStrategyNumber.Five majors, measured rather than assumed.
range_pruning.shwas run against 15.18, 16.14, 17.6, 18.4 and 19beta2:The 23 ledger rows were seeded by merging those five logs in a SINGLE call, so every row carries
15;16;17;18;19because that is what was observed, not because it was copied from a neighbouring row.checks_never_observed_redwas re-derived by counting on this tree rather than by adding 23 to the previous value.suites_not_coveredstays at 249: registering the suite and seeding it move the two counts together.Packaging
The column is added in both
pgcolumnar--1.0-alpha4--1.0-alpha5.sqlandpgcolumnar--1.0-alpha5.sql, appended at the marker #1190 left for exactly this. A fresh install and an upgraded one have to converge, and:is what asserts it. Every commit in this branch carries the column in both scripts, so no commit bisects to the diverging state.
heap_getattrpastnattscrashes rather than returning NULL, so both the insert and the read are guarded ontupdesc->natts >= Anum_zone_map_max_upper. An old catalog reads as "no summary", which is the NULL state above.Gate
Run in the audit container, assert builds:
🤖 Generated with Claude Code
https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP