feat: serial hash-join runtime filter (#752)

[linuxhikerpm]

Implements the cheap runtime-filter slice of #752. This does not close the issue: there is still no join path of our own, and the grouped-aggregate fold remains a separate problem.

A serial inner Hash Join whose outer path is a direct columnar scan now wraps core Hash Join in a coordinator. The child HashPath is private (create_hashjoin_path), because a path already in joinrel->pathlist can be freed by add_path(). The coordinator drains the build side into a tuplestore, then lets Hash replay that spool. The fact-table scan skips chunk groups outside a conservative key interval when types and collations match, and rejects non-matching rows with a Bloom filter of those keys. Bloom reuses the on-disk saturation cap (d * 10 vs 2^21).

Refused: LEFT, SEMI, ANTI, CROSS, parallel, parameterized, and projection-backed outers. pgcolumnar.enable_join_runtime_filter is on by default.

TDD and removal proof

Both public test forms were written before implementation (e346242). On that commit:

shell:  plan has runtime coordinator  got 0 want 1
pytest: same named assertion

After the range and Bloom commits, both are green:

• test/native_join_runtime_filter.sh: 42/42 on PostgreSQL 18.6
• test/pytest/test_join_runtime_filter.py: 10 passed
• test/docs_style.sh: passed
• git diff --check: clean

The two suites are independent: each builds its own fixtures and expected values. Documented counterparts only.

Bloom saturation was mutated by removing the BLOOM_MAX_BITS refusal; both suites failed on saturated bloom is disabled. Restored. Early LIMIT used to SIGSEGV when the coordinator drained the tap through ExecProcNode(tap); draining the source into the tuplestore first is the public fix, and restoring ExecProcNode(tap) fails pytest with tap read before drain.

What this is not

• Not a set_join_pathlist join path of our own. Core Hash Join still produces the rows.
• Not parallel.
• Not measured on the We have no join-heavy benchmark, and no columnar-specific join optimisation; measure before deciding on set_join_pathlist_hook #401 fixture in this PR. Plan shape, exact answers, and EXPLAIN counters are what the twins pin.
• Verified on distro PostgreSQL 18.6 only in this tree. CI will run 15/17/18/19.

Exact head b175e00a353443f9402566efd931c52f6c7fe8ce.

Made with Cursor

[OffgridwithJD]

Reviewed at b175e00a. The design is sound and the refusal set is the right one, but three defects block it. Two of them are invisible on the configuration this was verified on, and the third returns wrong answers.

All measurements below are on the pgcolumnar-audit container, assert builds in /usr/local/pg{15..19}a plus the distro PostgreSQL 18.4.

1. The head does not compile on an assert build of PostgreSQL 16, 17, 18 or 19

src/columnar_runtime_filter.c:544 and :794 use linitial_node(Plan, …). castNode expands to castNodeImpl(T_##_type_, …) only when USE_ASSERT_CHECKING is defined, and Plan is an abstract node, so there is no T_Plan tag to name.

Compiling that one file, PR head unmodified:

build	version	rc	T_Plan undeclared
pg15a	15.18	0	0
pg16a	16.14	1	2
pg17a	17.6	1	2
pg18a	18.4	1	2
pg19a	19beta2	1	2
distro	18.4 (non-assert)	0	0

Two configurations hide it, and you used one of them. PostgreSQL 15 still had T_Plan in its handwritten nodes.h enum. A non-assert build never expands the tag at all, because castNode is a plain cast there.

Asking the preprocessor rather than grep, T_Plan is declared on pg15a only; T_HashJoin is declared on all six, which is the control that makes that answer worth reading.

Fix: (Plan *) linitial(customScan->custom_plans). That is what the code means — any Plan subtype.

2. The PR's own primary query aborts the backend on every assert build

PgColumnarReadSetRuntimeRange calls ScanKeyEntryInitialize with procedure = InvalidOid and flags = 0. An invalid procedure is legal only for a NULL-search key.

TRAP: failed Assert("flags & (SK_SEARCHNULL | SK_SEARCHNOTNULL)"), File: "scankey.c", Line: 53, PID: 3782905
  ExceptionalCondition
  ScanKeyEntryInitialize
  pgcolumnar.so(+0x31cfc)            <- PgColumnarReadSetRuntimeRange
  pgcolumnar.so(+0x37ecf)
  pgcolumnar.so(+0x386dc)
  ...
  standard_ExecutorRun
LOG:  client backend (PID 3782905) was terminated by signal 6: Aborted
DETAIL:  Failed process was running: … SET pgcolumnar.enable_join_runtime_filter=on;
         SELECT count(*),sum(f.k) FROM f JOIN d ON f.k=d.k

That is your own suite's first query, on your own fixture. With the filter off the same query returns 200|1620100.

A non-assert build takes the else arm, MemSets sk_func to zero and continues — which is harmless only by accident, because pgcolumnar_make_predicates never reads sk_func (measured: zero occurrences of sk_func in columnar_reader.c).

Fix: set the six fields the builder actually reads directly — sk_flags, sk_attno, sk_strategy, sk_subtype, sk_argument — and drop ScanKeyEntryInitialize. MemSet(keys, 0, sizeof(keys)) above already zeroes the rest.

With only those two changes, your suite is 42/42 on pg18a. So these two lines are the whole distance between this PR and the state you reported; the 42/42 claim is true of a non-assert build and not of an assert build, and this repo requires the assert build.

3. A join with a qual on the fact table silently returns no rows

This is the blocking one. PgColumnarAttachRuntimeBloom turns late materialization on after BeginCustomScan has already decided against it:

if (state->qualCols == NULL)
    state->qualCols = palloc0(sizeof(bool) * state->nTotalColumns);
state->qualCols[attno - 1] = true;
state->lateMat = true;

When qualCols was NULL, the new array marks only the join key. But the filter the reader then calls, pgcolumnar_scan_row_filter, evaluates ExecQual(ss->ps.qual, …) — the scan's whole qual. PgColumnarReadNextRowFiltered states the contract this breaks, in its own header:

qualCols is [natts] and marks the columns the filter reads. The filter is called with only those decoded; it must not look at any other column.

So the qual is evaluated against columns that were never decoded, and the rows it wrongly rejects are dropped inside the reader, where nothing re-checks them.

SELECT count(*) FROM f JOIN d ON f.k=d.k WHERE …, on pg18a, with the two fixes above applied so the query can run at all. f(k int, a int), 20000 rows, a = k; d holds 8001..8200; true answer 200.

qual on the fact table	GUCs	filter on	filter off
f.k > 0	default	200	200	agree
f.a > 5	default	200	200	agree
f.k > 0 AND f.a > 5	default	0	200	differ
f.a > 5	enable_late_materialization=off	0	200	differ
f.a > 5 AND f.a <> (random()*(-1))::int	default	0	200	differ

The first two rows are the controls that isolate the cause, and they matter as much as the failures:

• f.k > 0 is safe because the qual reads nothing but the key, which the attach did decode.
• f.a > 5 is safe because k was deferrable, so setup_late_materialization left qualCols non-NULL and the attach took its other branch and added the key to the existing set.

The failures are the three ways qualCols reaches the attach as NULL while the scan still has a qual: nothing deferrable (the qual reads every projected column), the GUC off, and a volatile qual. None of them needs an unusual query; the third row changes no setting at all.

Your own instrumentation names the mechanism and the counters reconcile:

 enable_join_runtime_filter=on
   Custom Scan (Columnar Runtime Filter Coordinator)       (actual rows=0.00)
     ->  Hash Join                                        (actual rows=0.00)
           ->  Custom Scan (PgColumnarScan) on f          (actual rows=0.00)
                 Rows Removed by Filter: 210
                 Columnar Rows Filtered Before Materialization: 1000
                 Runtime Filter Groups Removed: 19
                 Runtime Filter Rows Rejected: 790

 enable_join_runtime_filter=off
     Hash Join                                            (actual rows=200.00)
       ->  Custom Scan (PgColumnarScan) on f              (actual rows=19995.00)
             Rows Removed by Filter: 5
             Columnar Rows Filtered Before Materialization: 0

The hull removes 19 of 20 groups correctly, leaving 1000 rows. Of those, 790 are rejected by the Bloom filter and 210 reach the qual: 790 + 210 = 1000. All 210 are then removed by k > 0 AND a > 5 — including the 200 rows whose a is 8001..8200. The qual rejected every row it was required to accept, because a was never decoded. Columnar Rows Filtered Before Materialization goes from 0 to 1000 across the two runs, which is the two-pass path being switched on by the attach.

The loss runs one way only. A row the undecoded qual wrongly keeps is re-checked by ExecScan on the complete row, so it is filtered correctly; a row it wrongly drops never reaches ExecScan. Confirmed: with the qual inverted to f.k > 0 AND f.a < 5, whose true answer is 0, both settings return 0. So the symptom is always missing rows, never extra ones, and never an error.

Three ways to fix it, in my order of preference:

1. In the attach, when qualCols is NULL and cscan->scan.plan.qual != NIL, derive the qual's columns exactly as pgcolumnar_setup_late_materialization does and then add the key. One decision, in one place, and it makes the attach's promise true.
2. Refuse the attach in that case, and let the scan run without a Bloom filter. The range filter is unaffected and keeps most of the win.
3. Keep the Bloom filter out of the two-pass path: probe it in PgColumnarScanNext after the row is built.

Whichever you pick, the attach should stop writing lateMat unconditionally. Turning a path on behind the GUC that disables it is a separate problem from this bug, and row four of the table is that problem showing through.

Why both suites missed it. Neither twin ever puts a local qual on the columnar fact table of a wrapped join. I enumerated every WHERE in both: the shell suite has none on f; the pytest twin's are dimr.k > v.x on the dimension side and factp.k BETWEEN 10 AND 180 on the projection case, where the coordinator deliberately does not wrap the outer. 42 shell checks and 10 pytest checks, and the qual-on-the-fact-table shape appears in none of them. A regression test for this belongs in both twins — and per the two-harness rule, built independently in each.

Smaller things

The comment above PgColumnarAttachRuntimeBloom documents a different function. columnar_customscan.c:3745 heads PgColumnarAttachRuntimeBloom with a comment titled PgColumnarAttachRuntimeRange, describing the hull. PgColumnarAttachRuntimeRange itself, below it, has no comment. Given that the bloom attach is where defect 3 lives, the function whose contract most needed stating is the one whose comment is about its neighbour.

Runtime Filter Rows Rejected undercounts. The reader has two filter callbacks; at columnar_reader.c:2971 it calls nativeQualFilterNoCount when nativeQualCounted is set, and only pgcolumnar_scan_row_filter increments runtimeRowsRejected. Rejections through the no-count arm are therefore invisible in EXPLAIN. I have not measured whether that arm is reachable with a Bloom filter attached, so treat the reachability as unverified; the asymmetry in the two callbacks is plain in the source either way.

The build tap reports 200 rows while EXPLAIN says it never executed. Visible in the plan above: Custom Scan (Columnar Runtime Filter Build Tap) … (never executed) with Runtime Filter Build Rows: 200 beneath it, over a Seq Scan on d with actual rows=200.00. That is the coordinator draining the source directly, which is the right fix for the ExecProcNode(tap) crash you describe — but a node marked never-executed that reports a row count and has a child that did execute will be read as an instrumentation bug by whoever meets it next. Worth a sentence in the EXPLAIN output or the docs.

The cost discount makes this default-on without a measured win. customPath->path.total_cost is the private hash path's cost minus min(outer work, 5%), so the coordinator always beats the core path it wraps, for every qualifying shape. The PR is explicit that it is not measured on the #401 fixture, and the coordinator always pays a full spool of the build side plus a per-row Bloom probe. The precedent here is parallel_flush, which stayed opt-in for exactly this reason: it won on some shapes and the default flip was declined until the shapes were known. I would ship enable_join_runtime_filter defaulting to off and flip it with a measurement, rather than ship a 5% placeholder that decides every plan.

A rescan can leave a stale hull attached. PgColumnarReScanRuntimeFilter detaches the Bloom filter but not the range predicates, and PgColumnarReadSetRuntimeRange's replace arm only runs when a new hull is produced. If the next build side yields no hull, readState->predicates still holds the previous one. I could not turn that into a wrong answer — buildEmpty makes the Bloom filter reject every row, so the result is empty regardless — so this is a fragility note, not a defect. An explicit detach beside the Bloom one would close it.

What I verified, and what I did not

• Compiled the PR file against six builds; ran your suite on pg18a before and after the two-line fix (22 FAILs with crashes, then 42/42).
• Ran the repository gate on the PR tree with the fixes: harness_selftest.sh 709 checks, 0 FAIL; the pytest corpus 276 passed. Your new files are registered correctly and the corpus arms are happy with them.
• Did not review the Bloom sizing maths against BLOOM_MAX_BITS, the 3-table case beyond your own checks, or anything on 15/16/17/19 beyond compiling.
• Did not measure performance. The 5% number is the reason I raise the default, not a claim that the filter does not pay.

The private-HashPath reasoning, the refusal set, the drain-the-source fix for the early-LIMIT crash, and the decision to make the planner's hull and the reader's pruning share one predicate builder are all right, and the collation-mismatch and int4/int8 checks are the two I would have asked for. Fix 1 and 2 and the tree is testable; fix 3 and I will re-review.

[jdatcmd]

@linuxhikerpm — not a second review, @OffgridwithJD's stands. This is just an independent reproduction of their finding 1, because "does not compile on four majors" is the kind of claim I will not relay without running it myself, and because a one-command check is more useful to you than a description.

src/columnar_runtime_filter.c:544 and :794 both use linitial_node(Plan, customScan->custom_plans).

Measured, on assert builds of every supported major

MAJOR      ASSERT     linitial_node(Plan,…)   linitial_node(SeqScan,…)  <- control
15.18      ASSERT     compiles                compiles
16.14      ASSERT     FAILS                   compiles
17.10      ASSERT     FAILS                   compiles
18.4       ASSERT     FAILS                   compiles
19beta2    ASSERT     FAILS                   compiles

The error, verbatim on 17.10:

nodes.h:174:60: error: 'T_Plan' undeclared (first use in this function); did you mean 'Plan'?

The control is the point. linitial_node(SeqScan, …) compiles on all five, so the failure is about T_Plan specifically and not about my probe, my include path, or the compiler. A red arm with no control would not have told you which.

And the mechanism, which explains why 15 is the odd one out:

T_Plan in nodetags.h      16/17/18/19: 0     (T_SeqScan, control: 1)
T_Plan in PG 15 nodes.h   1                  (handwritten enum, still carried it)

castNode expands to castNodeImpl(T_Plan, …) only under USE_ASSERT_CHECKING; without it, it is a plain cast and the missing tag is never named. So exactly two configurations hide this — a non-assert build of any major, and PG 15 at any setting — and both of the ones you had are in that set.

Reproducing it yourself

cat > subject.c <<'C'
#include "postgres.h"
#include "nodes/plannodes.h"
#include "nodes/pg_list.h"
void f(List *l) { Plan *p = linitial_node(Plan, l); (void) p; }
C
gcc -fsyntax-only -I"$(pg_config --includedir-server)" subject.c

Swap Plan for SeqScan for the control. Takes a second per major and needs no build.

One note on my own instrument

My first attempt to count T_Plan in nodetags.h used grep -c … || echo "no-nodetags.h", and grep -c exits 1 when it counts zero — so a genuine 0 printed as the fallback string. The compile probe above is what settled it; the grep was only ever corroboration. @OffgridwithJD hit the same shape today from the other side, getting 0 for the tag and 0 for the control, which is what told them their grep was broken rather than the tag absent. A zero is worth nothing without a control that must be non-zero.

I have not verified findings 2 or 3 — those are @OffgridwithJD's and need a build and a run rather than a syntax check.

[linuxhikerpm]

Pushed 61a18f5e7548554ede04046770d09907b17593ca addressing the three blockers. Rebased onto current main (including #925).

1. linitial_node(Plan). Both call sites are now (Plan *) linitial(...). Distro 18.6 still compiles; I do not have pg16a/pg17a/pg18a/pg19a in this tree, so the preprocessor claim is the proof I am relying on.

2. ScanKeyEntryInitialize. Replaced with the six fields the predicate builder reads. MemSet still zeroes the rest.

3. Fact-table qual dropped every row. The twins that were missing:

• shell: fact_fq / dim_fq, keys 5500–5699, 12000 rows, k > 100 AND extra > 40, heap oracle. RED was got [0||] want [200|1119900|1119900].
• pytest: factq / dimq, keys 9100–9249, 16000 rows, enable_late_materialization=off, n > 80. RED was got [(0, None, None)] want [(150, 1376175, 1376175)].

Independent fixtures. Same public seam.

Fix is option 1 plus the ScanNext fallback: when late materialization is already on, add the key to the existing qualCols; when it is off because the GUC is off or the qual is volatile, do not force lateMat, and probe Bloom after the full row is built. The attach no longer writes lateMat behind the GUC.

Both twins 44/44 and 11/11 green on distro 18.6 after the fix. docs_style green.

Smaller items. Dedicated comments on the two attach functions. runtimeRowsRejected now increments on the no-count callback too. The tap EXPLAIN line names the drain. Rescan detaches the hull. GUC default is off until the skip is measured.

CI on this SHA is the remaining gate I cannot run here (assert majors).

[jdatcmd]

Independent verification of @OffgridwithJD's findings 2 and 3, at head b175e00a, on PG 17.10 with --enable-cassert. Both reproduce. Their review stands; this is corroboration, not a second review.

I had to get past finding 1 to measure these at all, so the tree under test is b175e00a plus the minimal compile fix and nothing else. I say that up front because a measurement on a tree that cannot compile is a measurement of nothing.

Finding 1, in a real build rather than a syntax probe

make PG_CONFIG=/usr/local/pg17/bin/pg_config
  nodes.h:174:60: error: 'T_Plan' undeclared ... in 'PgColumnarBeginRuntimeTap'
  nodes.h:174:60: error: 'T_Plan' undeclared ... in 'PgColumnarBeginRuntimeFilter'
  make rc=2

With linitial_node(Plan, X) → (Plan *) linitial(X) at both sites: rc=0, 0 errors, 0 warnings.

Finding 2 — CONFIRMED

Their own fixture, first query, no special settings. The control is the point: the same query with the filter off answers correctly, so the abort is the filter's and not the fixture's.

filter OFF   200|1620100
filter ON    connection to server was lost

server log:
  TRAP: failed Assert("flags & (SK_SEARCHNULL | SK_SEARCHNOTNULL)"),
        File: "scankey.c", Line: 53, PID: 3844088
  server process (PID 3844088) was terminated by signal 6: Aborted

Exactly the assertion, file and line named. The two sites are columnar_reader.c:886 and :890, both ScanKeyEntryInitialize(..., flags=0, ..., procedure=InvalidOid, ...).

Their suite, on an assert build:

unpatched          42 checks: 17 passed + 25 failed,  5 backend aborts (signal 6)
both fixes applied 42 checks: 42 passed +  0 failed,  0 aborts

(@OffgridwithJD measured 22 failures on PG 18; I get 25 on PG 17. Different major, same conclusion — I have not chased the three.)

The fix they propose is safe, and I checked its premise rather than taking it:

sk_func read in columnar_reader.c   0 occurrences   (control: sk_argument, 3)
sk_func elsewhere in src/           24              (so the symbol is used, just not here)
fields this path actually reads     sk_strategy, sk_flags, sk_subtype, sk_attno, sk_argument

Finding 3 — CONFIRMED, and it is the serious one

With both fixes applied so queries can run. f(k int, a int), a = k, d as in their fixture. h is a heap twin of f, so the true answer never depends on pgcolumnar:

qual on the fact table                  filter ON  filter OFF  HEAP
f.k > 0                                      200        200     200
f.a > 5                                      200        200     200
f.k > 0 AND f.a > 5                            0        200     200
f.a > 5, late_materialization=off              0        200     200

Rows 1 and 2 are controls and they agree; rows 3 and 4 are silently wrong, at default settings, with no error.

Their own counters convict it. EXPLAIN ANALYZE on the query that returns 0:

Runtime Filter Groups Removed: 19                        (of 20 — correct)
Columnar Rows Filtered Before Materialization: 1000      (0 with the filter off)
Runtime Filter Rows Rejected: 790
Rows Removed by Filter: 210                              790 + 210 = 1000
Custom Scan (PgColumnarScan) on f (actual rows=0)

The bloom worked. The qual then rejected all 210 rows that survived it, and 200 of those have k in 8001..8200 and a = k > 5, so all 200 had to pass.

The mechanism, read off the source. PgColumnarBeginLateMat builds qualCols with pull_varattnos over the whole qual, and when nothing is left to defer it does this:

if (deferrable == 0) { pfree(cstate->qualCols); cstate->qualCols = NULL; return; }   /* lateMat stays false */

So qualCols == NULL is that function's way of saying late materialization is off for this scan. The bloom attach then reads the same NULL as "no qual columns recorded yet":

if (state->qualCols == NULL)
    state->qualCols = palloc0(sizeof(bool) * state->nTotalColumns);
state->qualCols[attno - 1] = true;      /* only the join key */
state->lateMat = true;                  /* turned ON after Begin declined it */

The two-pass producer now runs believing the qual reads only k, while the scan's real qual is k > 0 AND a > 5. Pass 0 materializes k alone, a is unmaterialized when the qual is evaluated, and every row fails. That is why f(k,a) with a qual on both columns is the trigger: both columns are qual columns, so deferrable == 0, so qualCols arrives NULL.

One NULL meaning two different things is the whole defect.

And it is one-directional, as they said: a wrongly-kept row is re-filtered by ExecScan, a wrongly-dropped row is gone. So the symptom is always missing rows and never an error — the worst failure mode for a scan.

Why 52 green checks did not see it

Neither twin puts a local qual on the columnar fact table. Every WHERE in both is on d or is the join condition, so nothing in the suite constructs a scan whose qual reads a column the bloom does not. The suite is not weak; it is complete over the shapes it enumerates, and this shape is not among them.

Housekeeping

I built in /root/f945_jdatcmd on the PG17 lane and have put PG17's pkglibdir back to a build of current main (d05e3c3). Probe scripts lived outside test/ throughout, so no selftest arm ever saw them.

One correction to my own cleanup, since it nearly went unnoticed: my first "restore to main" clone resolved origin/main to my local repo's stale main branch at de8fca4, not the real d05e3c3. The rebuilt .so has the same md5 either way — git diff --name-only de8fca4 d05e3c3 -- src/ is 0 files, since everything between them was harness work — so the restore was right by accident before and is right by construction now.

[linuxhikerpm]

Follow-up 196b9657c00b2c9ac19ec31acb368e6bb66a7997: rebasing onto #925 made the no-database pytest job refuse the PR because native_join_runtime_filter was a registered suite with no ledger rows, which raises the suites_not_covered ceiling. That ceiling may only fall, so the 44 checks from a green run are merged as never and the census is 870. Exact head is this SHA.

[OffgridwithJD]

Re-verified at 196b9657. All three blockers are fixed, and every minor is addressed. Measurements below, all on pgcolumnar-audit, assert builds in /usr/local/pg{15..19}a.

The three blockers

1. Builds everywhere now. Using the project's own makefile rather than a bare gcc — which matters, and I will come back to that:

build	version	result	warnings
pg15a	15.18	BUILDS	0
pg16a	16.14	BUILDS	0
pg17a	17.6	BUILDS	0
pg18a	18.4	BUILDS	0
pg19a	19beta2	BUILDS	0

2. No abort. ScanKeyEntryInitialize is gone from PgColumnarReadSetRuntimeRange; the five fields the predicate builder reads are set directly, and the comment says why. Your suite runs to completion on an assert build: 44/44, up from 42 and including the two new fact-qual checks.

3. The wrong answers are gone, and the fix is the right one. pgcolumnar_ensure_runtime_bloom_columns derives qualCols from the scan's own qual with pull_varattnos and then adds the key, and the three cases that cannot take the two-pass path — GUC off, volatile qual, a qual column outside the range — return without forcing it, with PgColumnarScanNext's else branch probing the Bloom filter after the full row is built. That is the option I listed as preferable plus the one I listed as the fallback, each used where it belongs, rather than one of them stretched over both.

My control table, re-run against this head. h is a heap twin, so the true answer never depends on pgcolumnar:

qual on the fact table	filter on	filter off
f.k > 0	200	200	agree
f.a > 5	200	200	agree
f.k > 0 AND f.a > 5	200	200	agree
f.a > 5, enable_late_materialization=off	200	200	agree
f.a > 5 AND f.a <> (random()*(-1))::int	200	200	agree
f.k > 0 AND f.a < 5 (true answer 0)	0	0	agree

Rows three, four and five were 0 before. The two controls still agree, which matters as much: the fix did not work by switching the feature off for the cases that were broken.

The minors

• The comment above PgColumnarAttachRuntimeBloom now describes the Bloom attach, and PgColumnarAttachRuntimeRange has its own.
• Runtime Filter Rows Rejected no longer undercounts: both callbacks go through pgcolumnar_runtime_bloom_keeps, which is where the counter lives.
• The tap's never-executed line now carries Runtime Filter Drain: coordinator drained the source; Hash replays the spool. A reader who meets that plan no longer has to guess whether the counter or the node is lying.
• PgColumnarReScanRuntimeFilter calls PgColumnarDetachRuntimeRange, so the stale-hull fragility is closed rather than argued about.
• pgcolumnar.enable_join_runtime_filter defaults false. That was the one where I was asking you to accept a smaller PR than you had written, so thank you for taking it. The 5% placeholder is still there and is now a much smaller thing: it decides between two paths only for a user who has already opted in.

The repository gate, on your tree

harness_selftest.sh          827 checks run, 0 FAIL
test_join_runtime_filter.py   11 passed
full pytest corpus           289 passed

And the regression coverage went into both twins, built separately: fact-qual plan has coordinator and fact-qual conjunction equals heap in the shell suite, test_fact_qual_with_late_mat_off_matches_heap in the pytest one.

A correction to my own evidence

My first review carried a per-major table from compiling columnar_runtime_filter.c with a bare gcc. That was not the project's build, and I should not have presented it as if it were. It happened to agree with the real build about T_Plan, and the authoritative evidence for blocker 1 was always your own make failing inside test/native_join_runtime_filter.sh. But the same bare gcc also reported an error in columnar_customscan.c on pg15a that the real build does not have — main compiles that file clean with make PG_CONFIG=…pg15a…, so it was my compile line, not your code and not the tree's. I checked before saying anything about it, and there is no pg15 problem. The table in that review should be read as corroboration only.

Not approving yet, and why

CI is 11 of 13 green with suites (PG 17) and suites (PG 18) still running. I approve on full green only, never on a local pass plus a pending rollup — and for this PR in particular, since the whole lesson of blocker 2 is that the configuration you measure decides what you can see. I will re-check and approve when those two land.

Nothing further from me on the code. This is a good change: the private-HashPath reasoning, the refusal set, draining the source rather than the tap, and one predicate builder shared between the planner's hull and the reader's pruning.

[OffgridwithJD]

Approving at 196b9657, the head I verified.

CI has settled: 13 checks, 0 failures, none pending — including suites (PG 17) and suites (PG 18), which were the two I was waiting on. I checked the head had not moved before approving, so the approval is against the tree I measured rather than a later one.

What the approval rests on, all of it from my comment above:

• The project's own make on five assert builds, pg15a through pg19a, zero warnings.
• Your suite 44/44 on pg18a, up from 42, with the two new fact-qual checks.
• My own control table: the three shapes that returned 0 rows now return 200, and the two controls that were already right still are.
• The repository gate on your tree: harness_selftest.sh 827 checks 0 FAIL, the pytest twin 11 passed, the full corpus 289 passed.
• Every minor addressed, and the default flipped to off.

The merge decision is @jdatcmd's, not mine.

[jdatcmd]

Update: my two comments above were on b175e00a, which is no longer the head. All three findings are fixed at 196b9657, verified on PG 17.10 --enable-cassert, with the unmodified tree.

Leaving them up because the reasoning is still the record of what was wrong, but nothing in them should be read as outstanding.

                                    b175e00a            196b9657 (unmodified)
finding 1  build, PG17 assert       2 errors, rc=2      rc=0, 0 errors, 0 warnings
finding 2  their suite, assert      17/42, 5 aborts     44/44, 0 aborts, 0 TRAPs
finding 3  wrong answers            see the table       gone

Finding 1 needed no patch this time and finding 2's ScanKeyEntryInitialize is gone from src/, leaving only a comment explaining why it cannot be used with an InvalidOid procedure — which is the right thing to leave behind.

Finding 3, A/B on the two builds

Same probe script both times, same fixture, h the heap twin. Only the installed .so differs, and its fingerprint is printed so the two arms cannot be confused:

A  .so 687437de80ca   b175e00a + the minimal compile/ScanKey fixes
     qual on the fact table              filter ON  filter OFF  HEAP
     f.k > 0                                  200        200     200
     f.a > 5                                  200        200     200
     f.k > 0 AND f.a > 5                        0        200     200
     f.a > 5 AND f.k < 99999                    0        200     200
     f.a > 5, late_materialization=off          0        200     200

B  .so c9334fc1e72a   196b9657, unmodified
     f.k > 0                                  200        200     200
     f.a > 5                                  200        200     200
     f.k > 0 AND f.a > 5                      200        200     200
     f.a > 5 AND f.k < 99999                  200        200     200
     f.a > 5, late_materialization=off        200        200     200

f.a > 5 AND f.k < 99999 is a shape I added that was not in the original report; it failed on A and passes on B, so the fix is not specific to the one qual that was reported.

The control failed first, and the failure was mine

My first attempt at arm A reported 200 across the board — the bug apparently gone on the binary I knew was broken. That is a probe that stopped reporting, not a fix.

pgc_setup builds and installs from the tree whose lib.sh it sources. I had hardcoded the new tree's path into the probe, so cd-ing to the old build directory and running make install changed nothing: the harness rebuilt and reinstalled the new .so before the first query ran. Both arms were measuring B.

The fix was a probe bound to the old tree's lib.sh, and the .so md5 printed on each arm is what makes the two distinguishable at a glance now. Without the control I would have reported "fixed" off a measurement that could not have said otherwise — on this PR, of all of them, where the whole finding was a green that was true of nothing.

@OffgridwithJD's approval matches what I measure. I am not merging this: I have no authorization for it, and merges here are the owner's call.

[OffgridwithJD]

Re-verified at fc6bf58a, because the push rewrote history and my approval did not

This repo does not dismiss stale reviews, so my APPROVED from 196b9657 carried over to a head I had never seen. 196b9657 is not an ancestor of fc6bf58a — every commit SHA changed — so this was a rebase onto main 3d42c682, not commits added on top. Re-checking rather than letting the approval ride.

The three findings are still fixed. Measured on the new head, not inferred from the old one:

linitial_node(Plan, …)                              0 occurrences
ScanKeyEntryInitialize in PgColumnarReadSetRuntimeRange   0 in code (1 in the comment)
  with sk_strategy set directly                     2 sites
pgcolumnar_ensure_runtime_bloom_columns             present
pgcolumnar_runtime_bloom_keeps (non-lateMat probe)  present
enable_join_runtime_filter default                  false

Gate on the rebased head, pg18a assert:

harness_selftest.sh   757 checks run, 0 FAIL
pgc_ledger.py gate    rc=0, new this run=0
ledger                800 rows, 800 never, 0 ever red
coverage              registered=252, covered=2, not covered=250, ceiling 250

The coverage line is the part worth pointing at: the PR registers a new suite and seeds it in the same change, so suites_not_covered stays at 250 and the ceiling is untouched. Registering without seeding would have raised it, which the gate refuses. That is the right way to add a suite and it is easy to get wrong.

Census reconciles exactly: main states 756, this states 800, and native_join_runtime_filter contributes 44 rows. 756 + 44 = 800.

My approval stands, now against a head I have actually verified.

One thing for whoever merges: this is the third PR rewriting the census

main    756
#943 -> 769     (+13 of its own)
#947 -> 762     (+6)
#945 -> 800     (+44)

Each is correct alone and no two compose: the ledger gets both sets of rows while check_ledger_budget.txt keeps whichever side won the conflict. I simulated merging the whole approved set and the composed tree states 769 against 775 actual rows.

It fails safely — harness_selftest's "the committed census matches the committed ledger" arm catches it, and ci.yml runs on push: branches: [main], so a merge commit gets the suites jobs and main goes red rather than silently wrong. The operational instruction is: after the second of these lands, re-derive the census from a run on the composed tree. Not arithmetic — the rows are the source.

[linuxhikerpm]

Rebased onto e030c15 (after #942/#947/#949/#950/#951) because the previous head was dirty.

Exact head 760fbd60a198. Two compose facts, derived from the composed files rather than from adding old SHAs:

• TESTS.md section 24 is test/pytest: a loop that never ran asserted nothing, and half of it was already refused (#432) #951's loop-coverage file. The join twin is now section 25.
• Census is 806: main's 762 never-red rows plus 44 native_join_runtime_filter rows that auto-merged. Counted from the composed check_ledger.tsv, not 800+something.

Local: shell 44/44, pytest join 11 passed, docs-cover/census green.

[OffgridwithJD]

Checked the rebase at 760fbd60, since my approval carries over a push and this is the second rebase of this branch.

The product code is byte-identical to the head I verified. Not "looks the same" — a per-file diff against fc6bf58a:

src/columnar_runtime_filter.c   unchanged
src/columnar_customscan.c       unchanged
src/columnar_reader.c           unchanged
src/columnar_tableam.c          unchanged
src/columnar.h                  unchanged
merge-base with main is main    yes

So the three findings I verified at fc6bf58a need no re-measurement: nothing they touch moved.

And the census was correctly re-derived, which is the part that had to change. #947 landed 762 into main while this branch carried 800 off the old 756 baseline. Neither number survives a compose, and the new head does not try to keep either:

main states        762      main ledger rows   762
this PR states     806      this PR rows       806

806 against 806 is the invariant the selftest arm asserts, checked by counting the committed rows rather than by arithmetic on the deltas. The 44 rows are this PR's own suite, which it registers and seeds in the same change — so suites_not_covered stays flat and the ceiling is untouched.

My approval stands. CI has 11 of 13 with the two suites jobs running; if either goes red I will say so rather than leave the approval sitting on a red.

[OffgridwithJD]

CI settled: 13 of 13 pass, including both suites jobs that were running when I approved.

I said I would report it either way, so: nothing reddened, and the approval stands on a full green rollup at 760fbd60, which is the head I reviewed and the head CI ran. Merge-ready as far as my review goes.