Restore contsel/contjoinsel for containment & key-existence operators (#2356)

[crprashant]

Restore contsel/contjoinsel for containment & key-existence operators

Closes #2356.

Why

The containment (@>, <@, @>>, <<@) and key-existence (?, ?|, ?&) operators on agtype are bound to matchingsel / matchingjoinsel. matchingsel is intended for pattern operators (LIKE/regex) — during planning it invokes the operator's underlying function (agtype_contains) once per pg_statistic MCV entry (and per histogram bin). With realistic statistics targets that produces a planner-time regression that dominates simple OLTP-style point queries.

This PR rebinds those operators to the lightweight contsel / contjoinsel estimators, which return fixed selectivity constants without calling the operator function. This is a deliberate planning-speed vs. estimate-accuracy trade-off. Note this is not the binding PostgreSQL core uses for jsonb — core keeps jsonb's @>, <@, ?, ?|, ?& on matchingsel / matchingjoinsel (verified on REL_16/17/18_STABLE in pg_operator.dat). So this is an AGE-specific choice favoring workloads where these operators appear in selective point lookups, not a core-precedent restoration. A future improvement (tracked separately) could add a custom agtype selectivity function that is both cheap and statistics-aware.

What

• sql/agtype_operators.sql, sql/agtype_exists.sql: 10 operators flipped to contsel / contjoinsel.
• age--1.7.0--y.y.y.sql: appended ALTER OPERATOR ... SET (RESTRICT, JOIN) for all 10 operators so existing installs flip on ALTER EXTENSION age UPDATE.
• regress/sql/containment_selectivity.sql: new test that pins the bindings via pg_operator, a scoped no-leaked-matchingsel guard, functional smoke for all 10 operators, and an upgrade-path assertion that simulates a stale (pre-fix) install, replays the shipped ALTER OPERATOR block, and confirms every overload flips to contsel / contjoinsel (run in a rolled-back transaction).
• regress/expected/cypher_match.out, regress/expected/cypher_vle.out: refresh expected — test_enable_containment now picks Nested Loop + Index Only Scans over Seq Scan/Hash Join (a direct, better plan), and two MATCH p=... and show_list_use_vle queries flip row order (they had no ORDER BY, now given a stable ORDER BY; result set unchanged).
• Makefile: register the new test in REGRESS.

Validation

Regression suite

36/37 pass with EXTRA_TESTS="pgvector fuzzystrmatch pg_trgm". Only age_upgrade fails — pre-existing on master at 774e781b (verified by git stash && installcheck baseline of 32/33 with the same age_upgrade failure). The new containment_selectivity test (including the upgrade-path assertion) passes on PG18.

Reporter's exact methodology — reproduced

Reporter's three scripts (generate_graph.sql, setup_func_for_workload.sql, workload_select.sql) used unchanged. Run on PG18 with default_statistics_target = 1000 to populate MCV lists, matching the reporter's analyzed-graph conditions:

Metric	matchingsel	contsel	Delta
EXPLAIN planning time	1.42 ms	0.97 ms	−32%
EXPLAIN execution time	0.34 ms	0.31 ms	~0%
pgbench TPS (8 clients × 30s)	5247	7378	+40.6%

The −32% planning-time delta lines up with the reporter's "~30% of execution time spent in agtype_contains during planning" observation; the +40.6% TPS gain matches the "severe TPS drop in OLTP-style workloads" they reported. The win comes from discarding stats-based estimation at plan time (contsel returns a flat constant) — i.e. exactly the trade-off described above.

Upgrade path

Validated end-to-end during the benchmark and pinned by the new regression assertion: operator bindings flip from matchingsel → contsel via the same ALTER OPERATOR statements the upgrade SQL ships, while operators remain functional throughout.

pg_operator snapshots

-- Before (matchingsel binding shipped on master)
 oprname |  lhs   |  rhs   |  restrict_fn |    join_fn
---------+--------+--------+--------------+------------------
 <<@     | agtype | agtype | matchingsel  | matchingjoinsel
 <@      | agtype | agtype | matchingsel  | matchingjoinsel
 @>      | agtype | agtype | matchingsel  | matchingjoinsel
 @>>     | agtype | agtype | matchingsel  | matchingjoinsel
 ?       | agtype | agtype | matchingsel  | matchingjoinsel
 ?       | agtype | text   | matchingsel  | matchingjoinsel
 ?&      | agtype | agtype | matchingsel  | matchingjoinsel
 ?&      | agtype | text[] | matchingsel  | matchingjoinsel
 ?|      | agtype | agtype | matchingsel  | matchingjoinsel
 ?|      | agtype | text[] | matchingsel  | matchingjoinsel

-- After (this PR)
 oprname |  lhs   |  rhs   | restrict_fn |   join_fn
---------+--------+--------+-------------+-------------
 <<@     | agtype | agtype | contsel     | contjoinsel
 <@      | agtype | agtype | contsel     | contjoinsel
 @>      | agtype | agtype | contsel     | contjoinsel
 @>>     | agtype | agtype | contsel     | contjoinsel
 ?       | agtype | agtype | contsel     | contjoinsel
 ?       | agtype | text   | contsel     | contjoinsel
 ?&      | agtype | agtype | contsel     | contjoinsel
 ?&      | agtype | text[] | contsel     | contjoinsel
 ?|      | agtype | agtype | contsel     | contjoinsel
 ?|      | agtype | text[] | contsel     | contjoinsel

Driver workflows

Intentionally not run: this PR only adjusts pg_operator selectivity metadata. There is no C code, type, or wire-protocol change that python / go / node / JDBC drivers could observe.

Notes for reviewers

• This is a deliberate accuracy-for-speed trade-off: matchingsel does provide better estimates when good statistics exist on heavily-analyzed agtype columns, so very-low-selectivity containment predicates may get worse estimates under contsel. The win is constant-time planning. PostgreSQL core keeps jsonb on matchingsel, so this is a deliberate AGE-specific divergence, not a precedent restoration. A future improvement (out of scope here) would be a cheap, statistics-aware agtype selectivity function; happy to file as a follow-up if there's interest.
• The containment_selectivity regression test is intentionally precise so the diff is loud if anyone re-introduces matchingsel here. The scoped guard catches future operator additions that forget the right helper, and the upgrade-path assertion catches a silent regression in the shipped ALTER OPERATOR block.

[crprashant]

Self-review caught a bug in the smoke test: SELECT ...::agtype ? 'a' was being parsed as the (agtype, agtype) overload (so 'a' was treated as an agtype literal — invalid syntax), producing an ERROR rather than verifying the (agtype, text) binding works. Added an explicit ::text cast on the right-hand side and regenerated the expected output.

Force-pushed amend 50beb10. Full installcheck still 36/37 (only pre-existing age_upgrade fails on master too).

[Copilot]

Pull request overview

Restores lightweight planner selectivity/joinsel functions (contsel/contjoinsel) for agtype containment and key-existence operators to prevent planner-time regressions caused by matchingsel evaluating operator functions against MCV statistics during planning.

Changes:

• Rebinds 10 agtype containment/key-existence operators to contsel / contjoinsel in the install SQL.
• Adds upgrade SQL (ALTER OPERATOR ... SET (RESTRICT, JOIN)) so existing installs flip bindings on extension update.
• Introduces a regression test that pins the bindings via pg_operator, plus updates expected outputs impacted by the resulting plan/order changes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
sql/agtype_operators.sql	Switches containment operators’ RESTRICT/JOIN to contsel/contjoinsel.
sql/agtype_exists.sql	Switches key-existence operator overloads’ RESTRICT/JOIN to contsel/contjoinsel.
age--1.7.0--y.y.y.sql	Adds upgrade-time ALTER OPERATOR ... SET (RESTRICT, JOIN) for all affected operators.
regress/sql/containment_selectivity.sql	Adds regression coverage to pin operator bindings and smoke-test operator behavior.
regress/expected/containment_selectivity.out	Expected output for the new regression test.
regress/expected/cypher_match.out	Updates expected output for plan/order changes caused by new selectivity bindings.
regress/expected/cypher_vle.out	Updates expected output for row order changes (no ORDER BY) under the new plan.
Makefile	Registers containment_selectivity in REGRESS.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jrgemignani]

@crprashant Can you rebase this PR? There have been some significant updates to the master as of late

[crprashant]

Done — rebased onto current master (73d0705). Conflicts in \Makefile\ and \�ge--1.7.0--y.y.y.sql\ resolved (kept the new \�gtype_jsonb_cast\ test entry and jsonb cast functions that landed since, appended the contsel ALTER OPERATOR block after). Ready for re-review.

[jrgemignani]

@crprashant -

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

[crprashant]

Pushed a fix addressing the latest review. During the rebase, an automated merge garbled age--1.7.0--y.y.y.sql (it inlined the new agtype<->jsonb cast section inside the agtype_to_jsonb function's string literal, leaving an unclosed quote) and left a double backslash in the Makefile REGRESS continuation.

Fixes in the latest push (f090a094):

• Makefile: restored a single trailing backslash so agtype_jsonb_cast and containment_selectivity chain correctly. Resolves the missing separator build failure.
• age--1.7.0--y.y.y.sql: regenerated from the pristine master version with only the 10 ALTER OPERATOR ... SET (RESTRICT, JOIN) statements appended. The agtype_to_jsonb definition is back to a clean one-liner and the file's diff vs master is now exactly the 34-line selectivity block — no unclosed quotes or stray fragments.

All three earlier review items (deterministic ORDER BY in cypher_match.sql/cypher_vle.sql and the scoped matchingsel guard) are preserved through the rebase. The PR is current with master and ready for re-review.

[jrgemignani]

@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).

[jrgemignani]

@crprashant Btw, this was what it suggests -

Why

The containment (@>, <@, @>>, <<@) and key-existence (?, ?|, ?&) operators on agtype are bound to matchingsel / matchingjoinsel. During planning, matchingsel invokes the operator's underlying function (agtype_contains) once per pg_statistic MCV entry (and per histogram bin). With a realistic default_statistics_target, that planning cost dominates simple OLTP-style point queries — the regression reported in #2356.

This PR rebinds those operators to the lightweight contsel / contjoinsel estimators, which return fixed selectivity constants without calling the operator function. This is a deliberate trade-off: we give up MCV/histogram-based estimate accuracy for agtype containment in exchange for constant-time planning.

Note this is not the binding PostgreSQL core uses for jsonb — core keeps jsonb's @>, <@, ?, ?|, ?& on matchingsel / matchingjoinsel (verified on REL_16/17/18_STABLE). So this is an AGE-specific choice favoring workloads where these operators appear in selective point lookups, not a core-precedent restoration. A future improvement (tracked separately) could add a custom agtype selectivity function that is both cheap and statistics-aware.

[crprashant]

Thanks for the careful review — both points addressed in the latest push (85bca9f0).

1. Rationale corrected. You're right, the core-precedent claim was wrong — core keeps jsonb's @>/<@/?/?|/?& on matchingsel/matchingjoinsel on REL_16/17/18_STABLE. I reworded the Why (PR body), the comment block in age--1.7.0--y.y.y.sql, and the commit message to frame this as a deliberate planning-speed vs. estimate-accuracy trade-off that diverges from core, rather than a precedent restoration. I also fixed the contradictory Notes for reviewers line (dropped the inaccurate "core accepts the same trade-off for jsonb" / non-existent jsonb_sel) and added the one-line caveat that very-low-selectivity predicates may get worse estimates under contsel.

2. Upgrade-path assertion added. containment_selectivity.sql now has an upgrade-path section that:

• forces all 10 overloads back onto matchingsel/matchingjoinsel (simulating a stale pre-fix install) and asserts that state via pg_operator;
• replays the exact ALTER OPERATOR ... SET (RESTRICT, JOIN) block the upgrade script ships;
• asserts via pg_operator that every overload (both oprrest and oprjoin) flipped to contsel/contjoinsel.

The whole section runs in a BEGIN ... ROLLBACK so it observes the flip without mutating catalog state for any later test. I replayed the shipped statements rather than running ALTER EXTENSION age UPDATE because the dev upgrade script targets the placeholder version y.y.y and isn't a stable version-chain target in the regress harness — there's a comment in the test explaining this. Expected output was regenerated by building + installing against PG18 and running make installcheck REGRESS=containment_selectivity (passes).