fix: a bigint column against an integer literal now prunes chunk groups (#477)

CHANGELOG.md

@@ -41,6 +41,24 @@ which was true until that script existed.
 
 ### Fixed
 
+- A `bigint` column compared against an unadorned integer literal now prunes
+  chunk groups (#477). The scan key was dropped because the column type's default
+  comparison function cannot take an `int4` argument, so predicates of the form
+  `bigint_column > 16000` read every chunk group while `bigint_column >
+  16000::bigint` pruned normally. The comparison function is now resolved for
+  both types from the column's btree operator family, which supplies exactly this
+  for the built-in numeric types. Where a family provides no such function the
+  key is still skipped, as before.
+
+  `EXPLAIN` did not show the difference. `Columnar Pushed-Down Filters` counts
+  scan keys given to the reader rather than predicates able to exclude a group,
+  so it reported the filter as pushed down while nothing was skipped.
+
+  The bloom filter probe remains disabled for cross-type equality. The filter
+  stores hashes of column-type values, so hashing a differently typed constant
+  would probe a slot that was never written and could skip a group holding
+  matching rows.
+
 - `pgcolumnar.analyze()` now honours the per-column statistics target set by
   `ALTER TABLE ... ALTER COLUMN ... SET STATISTICS` (#414). It read the global
   `default_statistics_target` for every column, so a column given its own target

src/columnar_reader.c

@@ -17,10 +17,14 @@
 #include "fmgr.h"
 #include "access/detoast.h"
 #include "access/htup_details.h"
+#include "access/nbtree.h"
 #include "access/relscan.h"
 #include "access/tupmacs.h"
 #include "access/xact.h"
+#include "catalog/pg_am.h"
+#include "commands/defrem.h"
 #include "miscadmin.h"
+#include "utils/lsyscache.h"
 #include "port/atomics.h"
 #include "port/pg_bitutils.h"
 #include "utils/memutils.h"
@@ -412,6 +416,7 @@ pgcolumnar_build_predicates(PgColumnarReadState *readState, int nkeys, ScanKey k
 		int			attidx;
 		Form_pg_attribute att;
 		TypeCacheEntry *tce;
+		bool		crossType;
 
 		/* only plain "column op const" comparison keys are usable */
 		if (key->sk_flags & (SK_ISNULL | SK_ROW_HEADER | SK_ROW_MEMBER |
@@ -427,21 +432,59 @@ pgcolumnar_build_predicates(PgColumnarReadState *readState, int nkeys, ScanKey k
 		attidx = key->sk_attno - 1;
 		att = TupleDescAttr(readState->tupdesc, attidx);
 
-		/* avoid cross-type comparisons that our column cmp proc cannot do */
-		if (OidIsValid(key->sk_subtype) && key->sk_subtype != att->atttypid)
-			continue;
+		crossType = (OidIsValid(key->sk_subtype) &&
+					 key->sk_subtype != att->atttypid);
 
 		tce = lookup_type_cache(att->atttypid,
 								TYPECACHE_CMP_PROC_FINFO |
 								TYPECACHE_HASH_PROC_FINFO);
 		if (!OidIsValid(tce->cmp_proc_finfo.fn_oid))
 			continue;
 
+		/*
+		 * A cross-type key needs the comparison proc for the PAIR of types, not
+		 * the column type's default one (#477).
+		 *
+		 * This used to skip the key outright, with the comment "avoid cross-type
+		 * comparisons that our column cmp proc cannot do". That was correct about
+		 * the proc and wrong about the remedy: handing an int4 Datum to
+		 * btint8cmp would read a value that is not there, so refusing was right,
+		 * but refusing meant an int8 column compared against a bare integer
+		 * literal skipped NOTHING. Measured on identical data in two columns:
+		 * `idi > 16000` removed 7 chunk groups of 10, `seq > 16000` removed 0,
+		 * and `seq > 16000::bigint` removed 7. That is ordinary SQL, not an edge
+		 * case, and EXPLAIN still reported the filter as pushed down because that
+		 * counter counts scan keys rather than predicates able to exclude.
+		 *
+		 * btree opfamilies carry exactly this: integer_ops supplies btint84cmp
+		 * for (int8, int4). Ask the column's default opfamily for the ordering
+		 * proc over both types, and where it has none, keep skipping the key --
+		 * which is the old behaviour, still correct, now the fallback rather than
+		 * the rule.
+		 */
+		if (crossType)
+		{
+			Oid			opclass = GetDefaultOpClass(att->atttypid, BTREE_AM_OID);
+			Oid			opfamily;
+			Oid			cmpProc;
+
+			if (!OidIsValid(opclass))
+				continue;
+			opfamily = get_opclass_family(opclass);
+			cmpProc = get_opfamily_proc(opfamily, att->atttypid,
+										key->sk_subtype, BTORDER_PROC);
+			if (!OidIsValid(cmpProc))
+				continue;
+			fmgr_info_cxt(cmpProc, &readState->predicates[n].cmpFn,
+						  readState->readContext);
+		}
+		else
+			fmgr_info_copy(&readState->predicates[n].cmpFn, &tce->cmp_proc_finfo,
+						   readState->readContext);
+
 		readState->predicates[n].attidx = attidx;
 		readState->predicates[n].strategy = key->sk_strategy;
 		readState->predicates[n].compareValue = key->sk_argument;
-		fmgr_info_copy(&readState->predicates[n].cmpFn, &tce->cmp_proc_finfo,
-					   readState->readContext);
 		readState->predicates[n].collation = att->attcollation;
 
 		/*
@@ -450,9 +493,17 @@ pgcolumnar_build_predicates(PgColumnarReadState *readState, int nkeys, ScanKey k
 		 * built. The scan key already matches the column collation (a
 		 * differently collated predicate is not pushed; see PgColumnarBuildScanKeys),
 		 * so hashing the constant under the column collation is consistent.
+		 *
+		 * NOT for a cross-type key (#477). The filter was built by hashing
+		 * COLUMN-type values, so hashing an int4 constant with the int8 hash proc
+		 * probes a slot the writer never set and the group is skipped although it
+		 * may hold the row: a wrong answer, where the min/max path above is only
+		 * ever conservative. Cross-type equality therefore keeps min/max pruning
+		 * and forgoes the bloom probe.
 		 */
 		readState->predicates[n].hasHash = false;
-		if (key->sk_strategy == BTEqualStrategyNumber &&
+		if (!crossType &&
+			key->sk_strategy == BTEqualStrategyNumber &&
 			OidIsValid(tce->hash_proc_finfo.fn_oid) &&
 			PgColumnarCollationIsDeterministic(att->attcollation))
 		{
@@ -714,12 +765,22 @@ native_zone_excludes(SkipPredicate *pred, Form_pg_attribute att,
 			c1 = DatumGetInt32(FunctionCall2Coll(&pred->cmpFn, pred->collation,
 												 minv, pred->compareValue));
 			return (c1 > 0);
-		case BTEqualStrategyNumber: /* col = const : skip if const<min or const>max */
+		case BTEqualStrategyNumber: /* col = const : skip if min>const or max<const */
+
+			/*
+			 * Column first, constant second, like every other branch here.
+			 *
+			 * This read cmp(const, min) and cmp(const, max), which is equivalent
+			 * for a same-type proc and WRONG for a cross-type one (#477): the
+			 * argument order is part of a cross-type proc's signature, so
+			 * btint84cmp(int8, int4) handed (int4, int8) reads both operands from
+			 * the wrong widths. Same test, stated from the column's side.
+			 */
 			c1 = DatumGetInt32(FunctionCall2Coll(&pred->cmpFn, pred->collation,
-												 pred->compareValue, minv));
+												 minv, pred->compareValue));
 			c2 = DatumGetInt32(FunctionCall2Coll(&pred->cmpFn, pred->collation,
-												 pred->compareValue, maxv));
-			return (c1 < 0 || c2 > 0);
+												 maxv, pred->compareValue));
+			return (c1 > 0 || c2 < 0);
 		case BTGreaterEqualStrategyNumber:	/* col >= const : skip if max < const */
 			c2 = DatumGetInt32(FunctionCall2Coll(&pred->cmpFn, pred->collation,
 												 maxv, pred->compareValue));

test/native_skip.sh

@@ -57,14 +57,57 @@ check "equality result parity" \
 	"$(q 'SELECT count(*) FROM h WHERE id = 12345;')"
 check "equality skips groups" "$(gt0 "$(skipped 'SELECT id FROM n WHERE id = 12345')")" "yes"
 
-# Predicate on the bigint column (k = id*10) is equally skippable. The literals
-# are cast to bigint so the comparison is same-type: cross-type operators (int8
-# column vs int4 const) are deliberately not pushed down, on native as on 2.2.
+# Predicate on the bigint column (k = id*10) is equally skippable.
 check "bigint range parity" \
 	"$(pgc_set_hash 'SELECT id FROM n WHERE k BETWEEN 100000::bigint AND 101000::bigint')" \
 	"$(pgc_set_hash 'SELECT id FROM h WHERE k BETWEEN 100000::bigint AND 101000::bigint')"
 check "bigint range skips groups" "$(gt0 "$(skipped 'SELECT id FROM n WHERE k BETWEEN 100000::bigint AND 101000::bigint')")" "yes"
 
+# ---- the same predicate without the casts (#477) -----------------------------
+#
+# Those literals were cast to bigint so the comparison would be same-type, and
+# this comment used to say cross-type operators were "deliberately not pushed
+# down". They were not pushed down, and the consequence was not deliberate: an
+# int8 column compared against a bare integer literal skipped NOTHING, which is
+# what ordinary SQL looks like. Measured on identical data in two columns, one
+# int and one bigint: `idi > 16000` removed 7 groups of 10, `seq > 16000` removed
+# 0, and `seq > 16000::bigint` removed 7 again.
+#
+# EXPLAIN gave no way to see it. `Columnar Pushed-Down Filters` counts scan keys
+# handed to the reader, not predicates able to exclude anything, so it read 1
+# while zero groups were skipped. test/zonemap_cost.sh's correlated arm has been
+# in that state since it was written.
+#
+# The parity check is first and is not decoration: the risk in comparing an int8
+# column against an int4 constant is a WRONG answer, not a slow one, so rows come
+# before skipping in the order these are asserted.
+check "bigint vs integer literal returns the same rows as heap (#477)" \
+	"$(pgc_set_hash 'SELECT id FROM n WHERE k BETWEEN 100000 AND 101000')" \
+	"$(pgc_set_hash 'SELECT id FROM h WHERE k BETWEEN 100000 AND 101000')"
+
+check "and it skips groups, which the cast version already did (#477)" \
+	"$(gt0 "$(skipped 'SELECT id FROM n WHERE k BETWEEN 100000 AND 101000')")" "yes"
+
+# One-sided and equality too, because they take different branches of
+# native_zone_excludes and equality additionally gates the bloom probe.
+check "one-sided bigint vs integer literal skips groups (#477)" \
+	"$(gt0 "$(skipped 'SELECT id FROM n WHERE k > 150000')")" "yes"
+check "one-sided parity" \
+	"$(q 'SELECT count(*) FROM n WHERE k > 150000;')" \
+	"$(q 'SELECT count(*) FROM h WHERE k > 150000;')"
+
+check "bigint equality vs integer literal skips groups (#477)" \
+	"$(gt0 "$(skipped 'SELECT id FROM n WHERE k = 123450')")" "yes"
+check "bigint equality parity" \
+	"$(q 'SELECT count(*) FROM n WHERE k = 123450;')" \
+	"$(q 'SELECT count(*) FROM h WHERE k = 123450;')"
+
+# A value no row holds must still come back empty. Cross-type equality disables
+# the bloom probe (the filter hashes column-type values, so an int4 constant
+# would probe the wrong slot), so this rides on min/max alone.
+check "bigint equality on an absent value returns nothing" \
+	"$(q 'SELECT count(*) FROM n WHERE k = 123451;')" "0"
+
 # A predicate every group satisfies must skip nothing (correctness of the bound).
 check "non-selective scan skips nothing" "$(skipped 'SELECT id FROM n WHERE id > 0')" "0"
 check "non-selective parity" \

test/zonemap_cost.sh

@@ -71,6 +71,41 @@ cost_for() {	# $1 = column, $2 = extra SETs
 		grep -oE 'cost=[0-9.]+\.\.[0-9.]+' | head -1 | sed 's/.*\.\.//'
 }
 
+# ---- the physical premise, which this suite asserted only in its title -------
+#
+# Everything below prices pruning. Nothing below checked that any pruning
+# happens, and for the whole life of this file none did (#477).
+#
+# `seq` is bigint and `$CUT` is a bare integer, so the scan key was cross-type
+# and the reader dropped it: zero chunk groups removed, on the arm whose entire
+# purpose is to be the one that prunes. The suite still passed, because a cost
+# relation between two priced plans is true or false regardless of whether the
+# physical effect being priced occurs. #460's discount was validated here.
+#
+# So the premise is now measured from the executor's own counter, before any
+# cost is compared. A discount for pruning that does not happen is not a
+# conservative error, it is a wrong price.
+removed_for() {	# $1 = column
+	psql_run "SET max_parallel_workers_per_gather=0;
+	          SET enable_indexscan=off; SET enable_bitmapscan=off;
+	          EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF)
+	          SELECT tag, count(*) FROM zc WHERE $1 > $CUT GROUP BY tag;" 2>/dev/null |
+		grep -oE 'Columnar Chunk Groups Removed by Filter: [0-9]+' |
+		grep -oE '[0-9]+$' | head -1
+}
+
+SEQ_REMOVED=$(removed_for seq)
+SCAT_REMOVED=$(removed_for scat)
+echo "-- groups removed: seq = ${SEQ_REMOVED:-?}, scat = ${SCAT_REMOVED:-?}"
+
+check_num "premise: the correlated arm actually removes chunk groups (#477)" \
+	"$([ -n "$SEQ_REMOVED" ] && awk "BEGIN{exit !($SEQ_REMOVED > 0)}" && echo 1 || echo 0)" "1"
+
+# And the control removes none, which is what makes the discount's absence there
+# meaningful rather than incidental.
+check_num "premise: and the scattered arm removes none, so the two differ physically" \
+	"$([ -n "$SCAT_REMOVED" ] && awk "BEGIN{exit !($SCAT_REMOVED == 0)}" && echo 1 || echo 0)" "1"
+
 # ---- the correlated arm: pruning is real, so it must be priced -------------
 SEQ_NODE=$(node_for seq)
 SEQ_COL=$(cost_for seq "SET enable_indexscan=off; SET enable_bitmapscan=off;")