From 36291119c44feadd4b97c7b7f03b891c8a22af73 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 15 Oct 2026 23:51:26 +0000 Subject: [PATCH] fix: a cidr or varchar column, or a domain over either, prunes chunk groups (#1433) A comparison on a cidr column resolves to inet's operator, and a varchar column's to text's, with the column relabelled. The key reached the reader with a column of cidr (or varchar) and an argument of inet (or text). The reader called it cross-type and asked the opfamily for an ordering proc over the pair. network_ops and text_ops register one only for (inet, inet) and (text, text), so the key was dropped. The column was read whole, while EXPLAIN still counted the filter as pushed down. A pair whose types are both binary-coercible to the type the column's default btree opclass orders is now same-type. The column's own cmp and hash procs (network_cmp and hashinet, bttextcmp and hashtext) are the opclass's, and the writer builds the zone maps and bloom filters with them, so the bloom probe stays consistent too. A polymorphic opclass input type is excluded: two enums are each coercible to anyenum without sharing an ordering. Red first, both twins, PG17: on main's reader, the six cidr, varchar and domain pruning arms of native_skip fail (removed 0), and the inet and text controls prune. type_matrix's cd arm was pinned at 0 with #1433 as its reason. It now requires pruning: red on main (removed 0), green here. Green on PG15 to 19: native_skip 65/65, type_matrix 57/57, and the cross-type and domain pruning suites pushdown_report, zonemap_cost and native_saop_pushdown. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 +++ src/columnar_reader.c | 32 +++++++++++ test/check_ledger.tsv | 2 +- test/check_ledger_budget.txt | 14 ++++- test/native_skip.sh | 44 +++++++++++++++ test/pytest/TESTS.md | 12 ++++ test/pytest/expected_tests.txt | 4 +- test/pytest/test_compare_to_bash.py | 7 +++ test/pytest/test_native_skip.py | 88 +++++++++++++++++++++++++++++ test/pytest/test_type_matrix.py | 2 +- test/type_matrix.sh | 8 +-- 11 files changed, 213 insertions(+), 8 deletions(-) create mode 100644 test/pytest/test_native_skip.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bc9814d8..7620decce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -430,6 +430,14 @@ true until the next version shipped. itself. The runtime filter now leaves such a join to the ordinary plan. Found by OffgridwithJD (#1437). `test/native_join_runtime_filter.sh` and `test_join_runtime_filter.py`. +- **A cidr or varchar column, or a domain over either, prunes chunk groups.** A + comparison on such a column resolves to inet's or text's operator, and the reader + called the key cross-type and dropped it, because the opfamily orders only + (inet, inet) and (text, text). The column was read whole, while EXPLAIN still + counted the filter as pushed down. Such a key now uses the column's own ordering + and hash, which the writer used to build the zone maps and bloom filters. Found + by OffgridwithJD reviewing #1426 (#1433). `test/native_skip.sh` and + `test_native_skip.py`. - **A concurrent `UPDATE` of the same row now fails with 40001 on a table with a unique index, as documented, not 23505 (#1421).** Two sessions updating one row of diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 98b29594a..410c5d009 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -29,6 +29,7 @@ #include "catalog/pg_am.h" #include "commands/defrem.h" #include "miscadmin.h" +#include "parser/parse_coerce.h" #include "utils/lsyscache.h" #include "utils/rangetypes.h" #include "port/atomics.h" @@ -884,6 +885,37 @@ pgcolumnar_make_predicates(SkipPredicate *out, int nkeys, ScanKey keys, crossType = (OidIsValid(argType) && argType != colType); + /* + * Nor is a pair of types that are both binary-coercible to the type the + * column's default btree opclass orders (#1433). A comparison on a cidr + * column resolves to inet's operator and a varchar column's to text's, + * so the key arrives with a column of cidr and an argument of inet. The + * two share one representation and the opclass's ordering, but + * network_ops registers an ordering proc only for (inet, inet), so the + * opfamily lookup below found nothing and the key was dropped: the + * column pruned nothing, as a domain did before #483. + * + * The column's own cmp and hash procs, from lookup_type_cache below, + * are the opclass's (network_cmp and hashinet for cidr, bttextcmp and + * hashtext for varchar). The writer builds the zone maps and bloom + * filters with those same procs, so treating the key as same-type keeps + * reader and writer in agreement, the bloom probe included. + * + * Not when the opclass orders a polymorphic type: two different enums + * are each binary-coercible to anyenum without sharing an ordering. + */ + if (crossType) + { + Oid opclass = GetDefaultOpClass(colType, BTREE_AM_OID); + Oid inType = OidIsValid(opclass) + ? get_opclass_input_type(opclass) : InvalidOid; + + if (OidIsValid(inType) && !IsPolymorphicType(inType) && + (colType == inType || IsBinaryCoercible(colType, inType)) && + (argType == inType || IsBinaryCoercible(argType, inType))) + crossType = false; + } + tce = lookup_type_cache(att->atttypid, TYPECACHE_CMP_PROC_FINFO | TYPECACHE_HASH_PROC_FINFO); diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 319cce7f4..08c1885fd 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -3335,7 +3335,7 @@ type_matrix type_matrix a range on bt holds rows on heap, so its comparison read type_matrix type_matrix a range on bt lets the zone maps skip chunk groups 15;16;17;18;19 2026-10-15 the scan never skips a row group by zone map (pgcolumnar_native_group_can_match ignored) type_matrix type_matrix a range on cd finds heap's rows 15;16;17;18;19 never - type_matrix type_matrix a range on cd holds rows on heap, so its comparison reads something 15;16;17;18;19 never - -type_matrix type_matrix a range on cd skips no chunk groups: a cidr key is treated as cross-type against inet and dropped (#1433) 15;16;17;18;19 never - +type_matrix type_matrix a range on cd lets the zone maps skip chunk groups 15;16;17;18;19 2026-10-15 a cidr key was treated as cross-type against inet and dropped, so the range removed no chunk groups (the tree before #1433) type_matrix type_matrix a range on ch finds heap's rows 15;16;17;18;19 never - type_matrix type_matrix a range on ch holds rows on heap, so its comparison reads something 15;16;17;18;19 never - type_matrix type_matrix a range on ch skips no chunk groups: every row group holds every value, since the column cycles through 26 letters 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 4e15196c1..50569a517 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -2803,4 +2803,16 @@ suites_not_covered 224 # never 2598 + 4 = 2602 # dated 868 + 4 = 872 # 2602 + 872 == 3474 -checks_never_observed_red 2602 +# +# #1433, a cidr or varchar key was dropped as cross-type, so the column pruned nothing +# (jdatcmd): 2602 -> 2601. type_matrix's cd range arm, pinned at 0 removed groups with +# #1433 as its reason, becomes 'a range on cd lets the zone maps skip chunk groups'. +# The pinned row leaves and the new one arrives dated, on PG17 by the tree before the +# fix, where the range removed 0 of 12. Five-major runs, 57/57 on each of 15-19. +# native_skip gains sixteen checks and is not a covered suite. +# +# ledger rows 3474 - 1 + 1 = 3474 +# never 2602 - 1 = 2601 +# dated 872 + 1 = 873 +# 2601 + 873 == 3474 +checks_never_observed_red 2601 diff --git a/test/native_skip.sh b/test/native_skip.sh index 8a4029fb3..0ff02d2d0 100755 --- a/test/native_skip.sh +++ b/test/native_skip.sh @@ -428,4 +428,48 @@ check "a partition takes the columnar custom scan (#436)" \ check "and it prunes row groups (#436)" \ "$(gt0 "$(skipped 'SELECT id FROM nskp WHERE id BETWEEN 5000 AND 5100')")" "yes" + +# ---- a type binary-coercible to its opclass's type must prune (#1433) -------- +# +# A comparison on a cidr column resolves to inet's operator and a varchar +# column's to text's, with the column relabelled. The key reached the reader +# with a column of cidr (or varchar) and an argument of inet (or text), so it +# was cross-type, and the opfamily registers an ordering proc only for +# (inet, inet) and (text, text). The key was dropped, and the column pruned +# nothing while EXPLAIN still counted the filter as pushed down. inet and text +# hold the same values and are the controls: the fixture prunes when the key is +# kept. Parity first in every arm, as above. +psql_run "CREATE DOMAIN nsk_cidr AS cidr;" +psql_run "CREATE DOMAIN nsk_vc AS varchar(20);" +psql_run "CREATE TABLE hb (id int, ip inet, cd cidr, dcd nsk_cidr, t text, vc varchar(20), dvc nsk_vc);" +psql_run "CREATE TABLE nb (id int, ip inet, cd cidr, dcd nsk_cidr, t text, vc varchar(20), dvc nsk_vc) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('nb', stripe_row_limit => 2048, chunk_group_row_limit => 1024);" +NB_GEN="SELECT g, a::inet, a::cidr, a::cidr, s, s, s + FROM (SELECT g, '10.0.' || (g / 256) || '.' || (g % 256) AS a, + lpad(g::text, 6, '0') AS s + FROM generate_series(1, 20480) g) x" +psql_run "INSERT INTO hb $NB_GEN;" +psql_run "INSERT INTO nb $NB_GEN;" +NB_IP="BETWEEN '10.0.20.0' AND '10.0.20.100'" +NB_S="BETWEEN '005000' AND '005100'" +for arm in "ip|inet|$NB_IP" "cd|cidr|$NB_IP" "dcd|a domain over cidr|$NB_IP" \ + "t|text|$NB_S" "vc|varchar|$NB_S" "dvc|a domain over varchar|$NB_S"; do + col=${arm%%|*}; rest=${arm#*|}; what=${rest%%|*}; pred=${rest#*|} + check "a range on $what returns the same rows as heap (#1433)" \ + "$(pgc_set_hash "SELECT id FROM nb WHERE $col $pred")" \ + "$(pgc_set_hash "SELECT id FROM hb WHERE $col $pred")" + check "and a range on $what skips groups (#1433)" \ + "$(gt0 "$(skipped "SELECT id FROM nb WHERE $col $pred")")" "yes" +done +# equality also takes the bloom probe, which hashes the constant with the +# column's hash proc: the same proc for both types of each pair +for arm in "cd|cidr|'10.0.40.7'" "vc|varchar|'012345'"; do + col=${arm%%|*}; rest=${arm#*|}; what=${rest%%|*}; val=${rest#*|} + check "equality on $what returns the same rows as heap (#1433)" \ + "$(q "SELECT string_agg(id::text, ',' ORDER BY id) FROM nb WHERE $col = $val")" \ + "$(q "SELECT string_agg(id::text, ',' ORDER BY id) FROM hb WHERE $col = $val")" + check "and equality on $what skips groups (#1433)" \ + "$(gt0 "$(skipped "SELECT id FROM nb WHERE $col = $val")")" "yes" +done + pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 72595d9e4..fe802fb8e 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -178,6 +178,7 @@ behaviour, the source of that number is named. - [130. test_rls_query_paths.py: row-level security holds on every read path](#130-test_rls_query_pathspy-row-level-security-holds-on-every-read-path) - [131. test_type_matrix.py: types the differential matrix never stored answer as heap](#131-test_type_matrixpy-types-the-differential-matrix-never-stored-answer-as-heap) - [132. test_iceberg_data_files.py: a delete entry in a data manifest is refused](#132-test_iceberg_data_filespy-a-delete-entry-in-a-data-manifest-is-refused) +- [133. test_native_skip.py: a type binary-coercible to its opclass's type prunes](#133-test_native_skippy-a-type-binary-coercible-to-its-opclasss-type-prunes) ## 1. How to read a test in here @@ -7209,3 +7210,14 @@ A partial pytest twin of `test/iceberg_data_files.sh` (#1341 item 49). It ports | `test_the_control_entry_is_listed` | a data manifest with one added data entry lists that one file | | `test_an_existing_entry_is_listed` | an EXISTING (status 0) entry is listed too, so a lister that keeps only ADDED entries fails | | `test_a_delete_entry_is_refused` | a position-delete entry, an equality-delete entry and a DELETED entry are each refused with 0A000, and the backend stays up | + +## 133. test_native_skip.py: a type binary-coercible to its opclass's type prunes + +A partial pytest twin of `test/native_skip.sh` (#1433). It ports only the arms that issue added, by the shell's names; the rest of the shell suite is declared INCOMPLETE in `test_compare_to_bash.py`. A comparison on a cidr column resolves to inet's operator, and a varchar column's to text's. The reader called such a key cross-type and asked the opfamily for an ordering proc over the pair, which `network_ops` and `text_ops` do not register, so the key was dropped and the column pruned nothing. inet and text hold the same values and are the controls. Each arm asserts heap's answer, then that chunk groups were removed. A premise checks that heap's answer holds the rows the arm expects. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_a_range_on_a_coercible_type_prunes` | a range on inet, cidr, a domain over cidr, text, varchar and a domain over varchar returns heap's rows and removes chunk groups | +| `test_equality_on_a_coercible_type_prunes` | equality on cidr and on varchar, which also takes the bloom probe, returns heap's row and removes chunk groups | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 3155afd4a..ec0e00570 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -731,4 +731,6 @@ guard_tests 438 # test_join_runtime_filter.py, by collection over the cluster files on this tree. # cluster_tests 604 -> 609 for #1341's test_iceberg_data_files.py, five tests (two # listed controls and three parametrized refusals), by collection over the cluster files. -cluster_tests 609 +# cluster_tests 609 -> 617 for #1433's test_native_skip.py, eight tests (six ranges and +# two equalities, parametrized), by collection over the cluster files on this tree. +cluster_tests 617 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 526afb198..9967566da 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -174,6 +174,13 @@ # about the PORT rather than a licence: the standing arm does not grade it, so the # reason is the only thing standing between a declared gap and a forgotten one. INCOMPLETE = { + # #1433 ported the arms it added: cidr, varchar and a domain over each prune + # as inet and text do, with heap's answer first. The rest of the shell suite + # (range, equality, bigint, the #477 and #483 arms, the partitioned table) is + # not. + "native_skip": "the #1433 binary-coercible arms and their inet and text " + "controls are ported; the range, equality, bigint, #477, #483 " + "and partitioned-table arms are not", # #1341 item 49 ported the arms it added: a delete or removed entry inside a # data manifest is refused, with a control that is listed. The rest of the # shell suite (the pyiceberg oracle, path rebasing and its escapes, the diff --git a/test/pytest/test_native_skip.py b/test/pytest/test_native_skip.py new file mode 100644 index 000000000..6736c2dee --- /dev/null +++ b/test/pytest/test_native_skip.py @@ -0,0 +1,88 @@ +"""A type binary-coercible to its opclass's type prunes row groups (#1433). + +A comparison on a cidr column resolves to inet's operator, and a varchar column's to +text's, with the column relabelled. The reader received a key with a column of cidr +(or varchar) and an argument of inet (or text), called it cross-type, and asked the +opfamily for an ordering proc over the pair. network_ops and text_ops register one +only for (inet, inet) and (text, text), so the key was dropped and the column pruned +nothing, while EXPLAIN still counted the filter as pushed down. + +inet and text hold the same values and are the controls: the fixture prunes when the +key is kept. Each arm asserts heap's answer before it asserts pruning, because using +the wrong comparison would give a wrong answer, not only a slow one. + +This ports only the #1433 arms of `test/native_skip.sh`; the rest of that suite is +declared INCOMPLETE in `test_compare_to_bash.py`. Independent of the shell arms: +different names, values and row count. +""" +import re + +import pytest + +REMOVED = re.compile(r"Columnar Chunk Groups Removed by Filter: (\d+)") + + +@pytest.fixture +def tables(pgc_conn): + pgc_conn.execute("CREATE DOMAIN skp_cidr AS cidr") + pgc_conn.execute("CREATE DOMAIN skp_vc AS varchar(16)") + cols = "id int, ip inet, cd cidr, dcd skp_cidr, t text, vc varchar(16), dvc skp_vc" + pgc_conn.execute(f"CREATE TABLE skh ({cols})") + pgc_conn.execute(f"CREATE TABLE skn ({cols}) USING pgcolumnar") + pgc_conn.execute("SELECT pgcolumnar.set_options('skn', stripe_row_limit => 2000, " + "chunk_group_row_limit => 1000)") + gen = ("SELECT g, a::inet, a::cidr, a::cidr, s, s, s FROM " + "(SELECT g, '172.16.' || (g / 200) || '.' || (g % 200) AS a, " + "'k' || lpad(g::text, 5, '0') AS s FROM generate_series(1, 18000) g) x") + pgc_conn.execute(f"INSERT INTO skh {gen}") + pgc_conn.execute(f"INSERT INTO skn {gen}") + return pgc_conn + + +def _removed(conn, sql): + plan = "\n".join(r[0] for r in conn.execute( + "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) " + sql).fetchall()) + m = REMOVED.search(plan) + return int(m.group(1)) if m else 0 + + +def _ids(conn, table, where): + return conn.execute(f"SELECT string_agg(id::text, ',' ORDER BY id) FROM {table} " + f"WHERE {where}").fetchone()[0] + + +IP_RANGE = "BETWEEN '172.16.30.0' AND '172.16.30.120'" +S_RANGE = "BETWEEN 'k06000' AND 'k06120'" + + +@pytest.mark.parametrize("col, what, pred", [ + ("ip", "inet", IP_RANGE), + ("cd", "cidr", IP_RANGE), + ("dcd", "a domain over cidr", IP_RANGE), + ("t", "text", S_RANGE), + ("vc", "varchar", S_RANGE), + ("dvc", "a domain over varchar", S_RANGE), +]) +def test_a_range_on_a_coercible_type_prunes(tables, expect, col, what, pred): + heap = _ids(tables, "skh", f"{col} {pred}") + expect.num(len(heap.split(",")) if heap else 0, 121, + f"premise: heap's range on {what} holds 121 rows") + expect.text(_ids(tables, "skn", f"{col} {pred}"), heap, + f"a range on {what} returns the same rows as heap (#1433)") + expect.num(min(_removed(tables, f"SELECT id FROM skn WHERE {col} {pred}"), 1), 1, + f"and a range on {what} skips groups (#1433)") + + +@pytest.mark.parametrize("col, what, val", [ + ("cd", "cidr", "'172.16.55.9'"), + ("vc", "varchar", "'k11009'"), +]) +def test_equality_on_a_coercible_type_prunes(tables, expect, col, what, val): + """Equality also takes the bloom probe, which hashes the constant with the + column's hash proc: the same proc for both types of each pair.""" + expect.text(_ids(tables, "skn", f"{col} = {val}"), "11009", + f"equality on {what} returns the same rows as heap (#1433)") + expect.text(_ids(tables, "skh", f"{col} = {val}"), "11009", + f"premise: heap's equality on {what} finds row 11009") + expect.num(min(_removed(tables, f"SELECT id FROM skn WHERE {col} = {val}"), 1), 1, + f"and equality on {what} skips groups (#1433)") diff --git a/test/pytest/test_type_matrix.py b/test/pytest/test_type_matrix.py index 999c7bd23..7c427d801 100644 --- a/test/pytest/test_type_matrix.py +++ b/test/pytest/test_type_matrix.py @@ -69,7 +69,7 @@ def _both(conn, sql): "ch": "every row group holds every value, since the column cycles through 26 letters", "e": "every row group holds every value, since the column cycles through the 4 labels", "n": "every row group holds NaN and both infinities, so each zone map spans every value", - "cd": "a cidr key is treated as cross-type against inet and dropped (#1433)", + # cd was pinned here until #1433, when a cidr key was dropped as cross-type } diff --git a/test/type_matrix.sh b/test/type_matrix.sh index 392f2dc4e..c4569b026 100755 --- a/test/type_matrix.sh +++ b/test/type_matrix.sh @@ -88,13 +88,13 @@ diff_query "each column's count and NULL count is heap's" "SELECT $COUNTS FROM % # review of #1426). Neither id is NULL in any column. # # Each arm also says whether the zone maps skipped chunk groups, from EXPLAIN ANALYZE, -# so an arm that should prune cannot pass on a scan that read everything. Four measure -# 0 of 12 on PG17 and PG18, and are pinned at 0 with the reason: +# so an arm that should prune cannot pass on a scan that read everything. Three measure +# 0 of 12 on PG17 and PG18, and are pinned at 0 with the reason. cd was a fourth until +# #1433, when a cidr key was treated as cross-type against inet and dropped: declare -A NO_PRUNE=( [ch]="every row group holds every value, since the column cycles through 26 letters" [e]="every row group holds every value, since the column cycles through the 4 labels" - [n]="every row group holds NaN and both infinities, so each zone map spans every value" - [cd]="a cidr key is treated as cross-type against inet and dropped (#1433)") + [n]="every row group holds NaN and both infinities, so each zone map spans every value") removed() { # removed SQL -> the columnar scan's "Chunk Groups Removed by Filter" q "EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) ${1//%T/t_col}" | sed -n 's/.*Columnar Chunk Groups Removed by Filter: \([0-9]*\).*/\1/p' | head -1