jrgemignani commented on PR #2417: URL: https://github.com/apache/age/pull/2417#issuecomment-4654414089
@crprashant Opus has opinions. Could you address these before I merge it? ## Review summary Thanks for the detailed writeup and benchmarks, @crprashant — the planner-time problem is real and the `contsel`/`contjoinsel` rebind is a reasonable fix. The operator coverage is complete (all 10 are currently on `matchingsel`) and the install/upgrade SQL lines up. Before merging, though, I'd like the **rationale corrected**, because the central justification doesn't match current PostgreSQL. ### The "core precedent" claim is inaccurate The PR's **Why** states: > PostgreSQL core binds jsonb's analogous operators (`@>`, `<@`, `?` on `jsonb`) to `contsel` / `contjoinsel` for exactly this reason. This PR restores that precedent for `agtype`. That isn't what core does. In `src/include/catalog/pg_operator.dat`, every jsonb containment / key-existence operator binds to **`matchingsel` / `matchingjoinsel`** — the same estimators AGE uses today — on every currently-supported major: | Operator (OID) | PG16 | PG17 | PG18 | |---|---|---|---| | `@>` jsonb,jsonb (3246) | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | | `?` jsonb,text (3247) | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | | `?\|` jsonb,_text (3248) | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | | `?&` jsonb,_text (3249) | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | | `<@` jsonb,jsonb (3250) | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | matchingsel/matchingjoinsel | Sources (`src/include/catalog/pg_operator.dat`, search `oid => '3246'`): - PG18: https://github.com/postgres/postgres/blob/REL_18_STABLE/src/include/catalog/pg_operator.dat - PG17: https://github.com/postgres/postgres/blob/REL_17_STABLE/src/include/catalog/pg_operator.dat - PG16: https://github.com/postgres/postgres/blob/REL_16_STABLE/src/include/catalog/pg_operator.dat So this isn't "restoring core precedent" — core deliberately keeps jsonb on `matchingsel`. What the PR actually does is make a **planning-speed vs. estimate-accuracy trade-off** that diverges from core. That's a defensible call given how expensive `agtype_contains` is at plan time, but the reasoning should say so rather than appeal to a precedent that doesn't exist. This also surfaces an internal contradiction: the **Notes for reviewers** section already says *"matchingsel does provide better estimates when good statistics exist; PostgreSQL core accepts the same trade-off for jsonb"* — the opposite of the **Why** section. (Minor: there's no `jsonb_sel` in core to mirror; jsonb itself just uses `matchingsel`.) ### The mechanism (and thus the perf win) is genuine For the record, the cost model checks out: `matchingsel` → `generic_restriction_selectivity()` → `mcv_selectivity()` + `histogram_selectivity()`, both of which `fmgr_info(get_opcode(...))` and invoke the operator's function against each MCV / histogram bin during planning. With `default_statistics_target = 1000` that's ~1000 `agtype_contains` calls per clause at plan time. `contsel` is a flat constant (`0.001`), so the speedup is real — it just comes from discarding stats-based estimation, which is exactly the trade-off to document. ### One coverage gap `containment_selectivity.sql` pins the bindings nicely on a **fresh install**, but it never exercises the **upgrade path** — and the appended `ALTER OPERATOR ... SET (RESTRICT, JOIN)` block is the whole point for existing installs. Could you add a check that runs `ALTER EXTENSION age UPDATE` (or the shipped `ALTER OPERATOR` statements) and re-queries `pg_operator` to confirm the bindings actually flip? Right now nothing fails if that block silently regresses. ### Bottom line Mechanically correct, complete operator coverage, real perf win. Happy to approve once: 1. **Why** is reworded to drop the inaccurate core-precedent claim and frame it as a speed/accuracy trade-off (and the contradiction with Notes for reviewers is resolved); 2. the **upgrade-path assertion** is added to the regression test. Optional: a one-line note that very-low-selectivity containment predicates may get worse estimates under `contsel` (the flip side of the trade-off). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
