feat: native-spelling encrypted-domain aliases (int4/int8/int2/float4/float8/decimal)#381
feat: native-spelling encrypted-domain aliases (int4/int8/int2/float4/float8/decimal)#381tobyhede wants to merge 16 commits into
Conversation
…/float4/float8/decimal)
…l (kept in canonical-dir sweep)
…oss operators Also raise test Postgres max_locks_per_transaction to 1024 so concurrent install/uninstall #[sqlx::test] cases don't exhaust the shared lock table over the enlarged surface.
…aises, uninstall teardown
…-in type) Addresses CodeRabbit review: bare native spellings (int4, decimal, ...) resolve to the pg_catalog built-in, not the encrypted public.<name> domain — a plaintext footgun. Document schema-qualification prominently; fix the adding-an-alias example to not imply integer already aliases 'int'.
…lti-alias safe) Fixes the latent gap where a family with >1 alias would leave alias<->alias comparisons un-shadowed (silently degrading to native jsonb). generate_all now fans out over every unordered pair of group_names(); byte-identical for the current single-alias-per-family catalog.
…milies Every aliased ordered-scalar family (smallint/int2, integer/int4, bigint/int8, real/float4, double/float8, numeric/decimal) now verifies cross-name = / <> (HMAC) and < <= > >= on _ord/_ord_ore/_ord_ope (ORE/OPE), both operand directions, over real encrypted doubles fixtures with an equal-plaintext/distinct-ciphertext self-guard. Wires real/double into DOUBLES_TOKENS + the doubles loader so floats get equal-plaintext encrypted fixtures too.
…pported ops CHECK-identity and operator public/blocker split now iterate ALIAS_PAIRS (all 6 canonical/alias pairs) in both operand directions; unsupported cross-op raise test covers <, @>, <@, || (not just <).
…urn-type compute; doc render_type Extract the CREATE OPERATOR block into operators/create.sql.j2, included by both operators.sql.j2 and cross.sql.j2 (byte-identical output, verified by parity). Hoist dd_sig/returns into the blocker branch where it's used. Restore the render_type doc comment.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
There was a problem hiding this comment.
Overview
Adds Postgres-native spelling aliases (int2/int4/int8/float4/float8/decimal) as full standalone encrypted domains, generated from a new aliases field on the catalog's DomainFamily, plus cross-name operators over every unordered pair of group names so an alias and its canonical twin compare through the encrypted index term rather than falling through to native jsonb.
The codegen design is the strongest part of this PR. group_names() as the single fan-out site is the right seam; cross_name_operators() deriving the shadow set from Domain/Domain signatures (rather than a hardcoded symbol list) with a pinned assertion so a new operator fails loudly is exactly the "derive from data AND assert the result" pattern the module already uses; extract_arg correctly emits no cast when both operands are domains; blockers stay plpgsql/non-STRICT; and the R4 orphan-sweep keep-set fix is real and tested.
What I verified
I built release/cipherstash-encrypt.sql from this branch and exercised it against a stock postgres:17 (max_locks_per_transaction=64, max_connections=100).
Everything the PR claims about behaviour holds:
- Transactional install succeeds at default lock settings.
psql -1 -f release/cipherstash-encrypt.sql→ exit 0. I expected the +84% object count (4,543 → 8,359 top-levelCREATEs) to blow the shared lock table and contradict the "no user-facing impact" note on the compose bump. It does not. The bump really is test-harness-only. Good call flagging it in the PR body. - Cross-name equality routes HMAC in both directions. Two payloads with equal
hmand distinctccomparetasint4_eq = integer_eqand asinteger_eq = int4_eq. - Cross-name ordering binds the encrypted operator.
pg_operatorfor<over(int8_ord, bigint_ord)hasoprcode = eql_v3.lt. - All four unsupported cross ops raise on the
_eqrole (<,@>,<@,||). - Uninstall is clean.
DROP SCHEMA … CASCADEtakes all 4,035 EQL operators to zero, which makes the newv3_uninstall_testspg_operatorassertion well-placed.
Blocking
1. The branch is 74 commits behind eql_v3, and CIP-3472 has invalidated the feature's premise
eql_v3 now prefixes every public column domain with eql_v3_ (PUBLIC_TYPNAME_PREFIX, crates/eql-domains/src/lib.rs:61; DomainFamily::domain_name → Domain::sql_typname). Today eql_v3 ships public.eql_v3_integer, not public.integer.
The rebase itself is mechanical — render_type_named calls domain_name(&d.full_name(name)), and post-rebase context::domain_name prepends the prefix, so aliases would come out as public.eql_v3_int4. The problem is semantic:
- The headline benefit — "a schema can use whichever native name it already reaches for" — evaporates.
public.eql_v3_int4is neither native nor familiar, andpublic.eql_v3_integeris no harder to type. - The entire "
⚠️ Always schema-qualify" argument in the PR body, indocs/reference/aliases.md, and in theCHANGELOG.mdentry becomes false:eql_v3_int4cannot shadow a built-in, because no built-in is named that.
Given the cost (~29.5k LOC, +84% installed objects, +540 operators in public), I think this needs a design decision before the rebase rather than after: does an eql_v3_int4 spelling still earn its keep alongside eql_v3_integer?
2. Changelog discipline: wrong file, no changeset, and a dead PR link
.changeset/ exists on eql_v3 but not at this branch's merge-base, so the branch predates the Changesets migration. Per CLAUDE.md, root CHANGELOG.md is now the frozen pre-3.0 archive and must not be hand-edited — the release changelog is assembled from .changeset/*.md. As it stands this PR edits the archive and ships no changeset, so the alias feature would not appear in the released changelog at all.
Separately, the entry links ([#370](https://github.com/cipherstash/encrypt-query-language/pull/370)). PR #370 does not exist (gh pr view 370 → "Could not resolve to a PullRequest"). This is #381. Moving the prose into a changeset fixes both problems at once — Changesets adds the PR link itself.
Correctness
3. docs/reference/aliases.md — the one safety-critical paragraph is wrong for double
The doc asserts:
every bare-family name in this set —
int2,int4, … and their canonical spellingssmallint,integer,bigint,real,double precision,numeric— is also a built-in PostgreSQL type name, resolved frompg_catalog, which always shadows thepublicschema.
Against the installed surface:
bare int4 -> pg_catalog.int4 (typtype = b) ✅ as documented
bare double -> public.double (typtype = d) ❌ the ENCRYPTED domain
SELECT count(*) FROM pg_type … WHERE nspname='pg_catalog' AND typname='double'; --> 0
double is not a pg_catalog type — the built-in is double precision. So bare double resolves through search_path to public.double, and the doc's warning is precisely inverted for that name: a user who writes CREATE TABLE t (x double) trusting this paragraph gets an encrypted domain, not a plaintext column.
The mechanism is also misdescribed for the rest. integer, smallint, bigint, real, decimal, numeric bind through the grammar (col_name_keyword → typename), not through pg_catalog shadowing public. The pg_catalog typnames are int2/int4/int8/float4/float8/numeric — i.e. mostly the aliases, not the canonical spellings.
The advice ("always schema-qualify") stays correct; the explanation and the double case do not. Note this section probably disappears under CIP-3472 anyway — but if any of it survives, it needs fixing.
4. Cross-family comparison still degrades silently, and aliases make the near-miss much easier to type
-- equal hm, distinct c
('…hm:a,c:x'::jsonb::public.int4_eq) = ('…hm:a,c:y'::jsonb::public.bigint_eq) --> f
EXPLAIN VERBOSE:
((…::int4_eq)::jsonb = (…::bigint_eq)::jsonb) -- native jsonb =That is exactly the silent-wrong-answer this feature exists to eliminate, one family over. It is pre-existing — the base branch has no cross operators at all — so this is not a regression, and I don't think it must block.
But it's worth weighing: before this PR the wrong pairing looked wrong (integer_eq vs bigint_eq are visibly different types). After it, the wrong pairing is int4_eq vs int8_eq — one character apart, both spelled "int", and adjacent in exactly the mental model the alias table invites. aliases.md covers this in one sentence ("any incidental behaviour is not part of the contract"), which understates an operator that returns false for equal plaintexts rather than raising.
Minimum: promote it to a int4_eq = int8_eq example. Better, as a follow-up: the cross_name_operators() classifier is already signature-derived, so emitting cross-family blockers is the same machinery — though the pair count needs a look before committing to it.
Test quality
5. The R3 guard cannot fail for the reason it claims
crates/eql-codegen/src/generate.rs:
assert!(sql.contains("eql_v3_internal.") && sql.contains("RETURNS jsonb")); // the || blockerTwo independent contains over the whole file: it passes as long as some blocker exists and some function returns jsonb. If the || blocker regressed to RETURNS boolean this would still be green, which defeats the point of an R3 guard on signature-derived return types. Anchor it to the concat blocker's own signature:
assert!(sql.contains(
"CREATE FUNCTION eql_v3_internal.\"||\"(a public.integer_eq, b public.int4_eq)\nRETURNS jsonb"
));6. unsupported_cross_name_ops_raise can't distinguish the directions
The assertion is msg.contains("not supported"). The blocker's RAISE names only the left domain, so the two directions genuinely produce different messages, and the test asserts neither. Worth pinning the domain name per direction.
Relatedly, the message itself reads a little oddly for a cross operator: operator < is not supported for public.int4_eq when the real fact is that the pair has no ordering. Naming both operands would be clearer, and would give the test something direction-specific to assert.
7. alias_domains_exist_with_identical_check normalizes nothing
canon.map(|s| s.replace(canonical, "T")) == alias_c.map(|s| s.replace(alias, "T"))The comment above it says the generated CHECK body carries no type name — which is true, so both replaces are no-ops and the test is really "the CHECK bodies are byte-identical". That's the stronger, clearer assertion; the .replace() calls imply a normalization that isn't happening and would be substring-fragile if a type name ever did appear. Also worth adding assert!(canon.is_some()) so a vanished canonical domain fails on its own terms.
Nits
- 540 new operators land in
public, noteql_v3.CREATE OPERATORis emitted unqualified, so all 4,035 EQL operators are created inpublic(pre-existing, andDROP SCHEMA … CASCADEdoes clean them up — verified). Just noting thepublicoperator footprint grew ~15% and nothing in the PR mentions it. group_names()allocates aVecper call and is called inside thegenerate_allloops. Irrelevant at this scale, but it could return animpl Iterator.aliases.mdisn't linked from any docs index that I can see.
Summary
The generator work is well-factored and the behaviour is correct — I confirmed the cross-name routing, the blockers, the uninstall teardown, and (against my own expectation) that the object-count growth doesn't break transactional installs at default lock settings.
What holds it up is not the code. The branch was written against a base that no longer exists: CIP-3472 renamed the public domains to eql_v3_*, which removes both the ergonomic reason for the aliases and the shadowing hazard the docs are built around. I'd resolve that question before spending a rebase on 29.5k lines of generated SQL. Items 2 and 3 are small and should be fixed regardless; 5–7 are cheap test hardening.
freshtonic
left a comment
There was a problem hiding this comment.
The implementation is excellent and I'm confident in it — requesting changes only on the release/changelog seam, which is a process fork rather than a code problem.
Implementation: verified clean
Regenerated the surfaces from this branch and checked the invariants mechanically (not by eye):
- SQL surface regenerated → zero drift (16 type surfaces incl. all 6 aliases); Rust bindings regenerated → zero drift, and the aliases correctly produce no new bindings, matching the "purely a SQL-surface concern" design.
cargo test -p eql-domains -p eql-codegen→ 94 + 98 pass, including the new alias/cross-file unit tests and the parity gate.- Footgun invariants hold on the real generated
integer__int4_cross.sql: wrappers areLANGUAGE sql IMMUTABLE STRICTsingle-SELECT with noSET search_path(inlinable); blockers areLANGUAGE plpgsqland not STRICT; noCREATE CAST; alias domains are... AS jsonb(no domain-over-domain); no opclass on a domain; noeql_v2references. - Coverage is strong:
v3_alias_interop_tests.rs(CHECK identity, wrapper/blocker split both directions, HMAC-not-jsonb routing, blockers raise),property/cross_name_routing.rs, anddocs/reference/aliases.mdis present (the CHANGELOG link resolves).
cross_name_operators() deriving the cross surface from the Domain/Domain signature set (with a pinned-symbol assertion) and the symmetric alias×alias handling are particularly nice.
Blocking: release-process reconciliation
- No changeset for a user-facing feature. Per the repo's release discipline (CLAUDE.md /
.changeset/README.md), every releasable change needs a.changeset/*.mdandCHANGELOG.mdis not hand-edited — Changesets owns the version bump and the generatedpackages/eql/CHANGELOG.md. This PR adds no changeset, so as it stands the alias feature won't bump the version (this is aminor) and won't appear in the release notes. Please add aminorchangeset. - This revives the frozen root
CHANGELOG.md. The PR reintroduces an## [Unreleased]section (and rewrites the "how to read this file" header to describe[Unreleased]promotion) on a changelog that is currently the frozen pre-3.0 archive (its newest section is[2.3.1]; 3.x lives inpackages/eql/CHANGELOG.md). Moving the project back to a hand-maintained root changelog is a real workflow decision that conflicts with the active Changesets model and with every other in-flight PR (e.g. #421). If that migration is intended, let's make it an explicit, standalone decision (and update CLAUDE.md); it shouldn't land implicitly inside a feature PR. - CHANGELOG entry links to #370, not this PR (#381). Looks like a stale copy-paste — worth confirming/fixing to whichever PR actually merges.
None of this touches the code, which is ready. Once the changeset/changelog approach is settled I'll happily re-approve.
(Not run here: the DB-backed SQLx tests — they need a PG container + ZeroKMS creds, which CI has; the committed generated surface is the drift-authoritative check and it's clean.)
Adds Postgres-native type-spelling aliases to the EQL v3 encrypted-domain surface, generated from the catalog. Each alias is a full standalone encrypted type — not a view, cast, or wrapper — that shares the canonical payload envelope exactly and interoperates with its canonical twin in both directions via generated cross-name operators.
smallintint2realfloat4integerint4doublefloat8bigintint8numericdecimalUsage
An alias carries the same per-capability variants as its canonical family (
public.<name>,_eq,_ord,_ord_ore,_ord_ope). Type columns with the schema-qualified name:Both-directions interop
An alias value and its canonical twin compare/join correctly, in either operand order. The comparison binds the generated cross-name operator, routing through the encrypted index term (HMAC for equality, ORE/OPE for ordering) — never native
jsonb:Two independent encryptions of the same value therefore compare equal across the alias boundary — native
jsonb =(comparing the raw ciphertextc) would wrongly report them distinct. That silent degradation is exactly what the cross-name operators shadow.Value conversion is a plain cast
No
CREATE CAST(impossible on domains, and unnecessary — both resolve tojsonb).Every one of these names — aliases and canonical spellings — is also a built-in PostgreSQL type, resolved from
pg_catalog, which shadowspublic. A bare unqualified name binds the plaintext built-in, not the encrypted domain:This is not alias-specific —
public.integerbehaves identically. Documented indocs/reference/aliases.md.How it's generated
One
aliasesfield on the catalog'sDomainFamilydrives everything.group_names()(canonical-first) is the single fan-out site; the generator emits a full per-name surface undersrc/v3/scalars/<name>/, plus a cross-name operator file for every unordered pair of group names — shadowing exactly the 9 operators with aDomain/Domainsignature (= <> < <= > >= @> <@ ||), classified from the operator table, not a hardcoded list. Supported ops become public wrappers (eql_v3.*); unsupported ones become internalplpgsqlblockers that raise. Schema placement is unchanged (user domains inpublic, wrappers ineql_v3, blockers ineql_v3_internal); the surface stays self-contained (noeql_v2.*). Aliases are a SQL-surface concern only — no bindings, fixtures, orscalars::matrix tests, so the pinned inventory baseline is untouched.Testing
Every aliased family is tested against its alias at all levels, matrix-driven, on real encrypted fixtures:
group_names, global uniqueness),render_cross_fileboth-directions + alias×alias symmetry, classifier, orphan-sweep (idempotent re-run + stray-artefact sweep), parity gate counting alias dirs.encrypted_domain::property::cross_name_routing) — for all 6 families: cross-name=/<>(HMAC) on_eqand</<=/>/>=(ORE/OPE) on_ord/_ord_ore/_ord_ope, both operand directions, over realeql_v3_<T>_doublesfixtures (equal-plaintext / distinct-ciphertext), with a self-guard that a meaningful pair exists. Wiresreal/doubleinto the doubles fixtures so floats get equal-plaintext encrypted data.v3_alias_interop_tests) — CHECK-identity + public-wrapper/internal-blocker split over all 6 pairs × both directions; unsupported-op raises for< @> <@ ||.v3_uninstall_testsadds apg_operatorteardown assertion.tests/docker-compose.ymlraisesmax_locks_per_transactionto 1024: the enlarged surface makes concurrent#[sqlx::test]install/uninstall transactions exhaust the default shared lock table (CI applies this compose file viamise run postgres:up). Single-connectionDROP SCHEMA … CASCADEat the default 64 is unaffected, so there is no user-facing impact.LOC breakdown
git diff --shortstatreports 112 files, 29,501 insertions / 63 deletions against the merge base (ecae8542), but ~96% is machine-produced, parity-gated SQL:src/v3/scalars/<alias>/(84 files)src/v3/scalars/**__**_cross.sql(6 files)crates/(Rust + minijinja templates)tests/(harness + suites)docs/Summary:
codegen:parity, not human-authored.real/doubledoubles fixtures is gitignored, so it never appears in the diff.aliasescatalog field + theeql-codegenrenderers. Adding another alias is one array entry.Review implication: the 90 generated files do not need line-by-line review —
codegen:parityverifies them. Reviewer attention belongs on the 22 hand-written files: the catalog/codegen change (crates/eql-domains,crates/eql-codegen) and the cross-name test suites (tests/sqlx).Review
Reviewed with CodeRabbit and four parallel dimension agents (architecture, reuse, type-safety, test-coverage). Findings addressed: multi-alias all-pairs fan-out; full ORE/OPE ordering coverage for all families;
CREATE OPERATORpartial extraction (byte-identical); and the schema-qualification safety caveat in docs +CHANGELOG.md.🤖 Generated with Claude Code