cockroachdb / cockroachdb/cockroach
sql: ALTER TYPE ... DROP VALUE misses untyped enum constants in triggers
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
#173504 is fixed by cockroachlabs/cockroach#3895, which teaches `ALTER TYPE ... DROP VALUE` to scan a table's trigger WHEN conditions and inlined function bodies. That scan matches a constant only where the stored expression carries its type. A constant whose type is merely *implied* by the column it is compared against is stored verbatim, matches nothing, and the drop is allowed — reproducing the original #173504 failure. This issue tracks that remainder.
Measured against the raw descriptor (`crdb_internal.pb_to_json` over `system.descriptor`), for `CREATE TYPE enum_t AS ENUM ('a','b','c')` and a table with column `c enum_t`:
| construct | stored form | detected |
|---|---|---|
| `WHEN ((NEW).c = 'a'::enum_t)` | `((new).c = 'a'::@100106)` | yes |
| `WHEN (... ANY ('{a}'::enum_t[]))` | `((new).c = ANY ('{a}'::@100107))` | yes |
| `WHEN (... ANY (ARRAY['a']::enum_t[]))` | `ARRAY[b' ':::@100106]:::@100107::@100107` | yes |
| body `SELECT ('a'::enum_t IS NULL)` | `b'@':::@100106` | yes |
| **`WHEN ((NEW).c = 'a')`** | `((new).c = 'a')` | **no** |
| **body `IF NEW.c = 'a'`** | `IF new.c = 'a'` | **no** |
Note `SHOW CREATE TRIGGER` is misleading here: it re-resolves for display and renders the bare constant as `WHEN (new).c = 'a':::public.enum_t`, implying a type reference that is not in the descriptor. Read `information_schema.triggers.action_condition` or the descriptor directly.
**To Reproduce**
```sql
CREATE TYPE enum_t AS ENUM ('a', 'b', 'c');
CREATE TABLE tab (i INT PRIMARY KEY, c enum_t);
CREATE FUNCTION noop() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$ BEGIN RETURN NEW; END $$;
CREATE TRIGGER trg BEFORE INSERT ON tab
FOR EACH ROW WHEN ((NEW).c = 'a') EXECUTE FUNCTION noop();
ALTER TYPE enum_t DROP VALUE 'a'; -- succeeds; should be rejected
INSERT INTO tab VALUES (1, 'b'); -- ERROR: invalid input value for enum enum_t: "a" (22P02)
```
The body variant is the same with the WHEN clause removed and the function body replaced by `IF NEW.c = 'a' THEN RETURN NULL; END IF; RETURN NEW;`.
Both are pinned as expected-wrong-behaviour assertions in the `enum_value_drop_with_triggers_untyped_gap` subtest of `logic_test/triggers`, added by cockroachlabs/cockroach#3895. They flip to plain `2BP01` rejections when this is fixed.
**Expected behavior**
`ALTER TYPE enum_t DROP VALUE 'a'` fails with `2BP01 could not remove enum value "a" as it is being used in trigger "trg" of "tab"`, as it already does for the explicitly-cast spellings.
**Additional data — root cause and a validated fix direction**
*WHEN clause.* [`buildWhenForTrigger`](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/opt/optbuilder/create_trigger.go#L141) already fully type-checks the WHEN expression against the table's row type and then discards the typed tree; scbuild serializes the [original untyped AST](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_trigger.go#L115). This is asymmetric with the body path, which [does](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/opt/optbuilder/create_trigger.go#L108) assign its built result back.
Storing the typed form instead was prototyped and verified end-to-end: the constant then serializes as `(new).c = b'@':::@100106`, which the existing scan already recognizes, with no change to `type_change.go`. Two wrinkles found:
- Assigning `typedWhen` directly produces `((@@1).c = x'40':::@100106)`, a syntax error on reparse. `FmtSerializable` and `FmtCheckEquivalence` are the [identical bit set](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/sem/tree/format.go#L263), so [`scopeColumn.Format`](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/opt/optbuilder/scope_column.go#L205) always emits `@@N`. Walking the typed tree to put `NEW`/`OLD` back as names fixes it.
- Type checking drops the outer parens, so `SHOW CREATE TRIGGER` then emits non-reparseable `WHEN (new).c = 'a':::public.enum_t`. Needs a `ParenExpr` wrap.
*Body.* [`buildFunctionForTrigger` serializes `stmt.AST`](https://github.com/cockroachlabs/cockroach/blob/bbe7588b561d5742362ae7721cdd1063c5e9f458/pkg/sql/opt/optbuilder/create_trigger.go#L306). The PL/pgSQL builder rewrites embedded *SQL statements* in place — hence `SELECT ('a'::enum_t IS NULL)` becoming an annotation — but not `IF`-condition expressions. Separate fix in the PL/pgSQL builder; the WHEN change does not help it.
*Two follow-on concerns for either approach:*
1. **Not retroactive.** Triggers already stored keep the bare form, so a serialization-side fix alone leaves existing descriptors exposed. Needs an upgrade migration or a scan-side fallback.
2. **Catalog output.** `information_schema.triggers.action_condition` and `pg_catalog.pg_trigger.tgqual` return the raw stored string, so they would start showing `(new).c = b'@':::@100106` for the common case. They already leak `@100106` today for explicitly-cast constants; routing them through `ParseTriggerWhenExprForDisplay` (as `SHOW CREATE TRIGGER` does) becomes required rather than optional.
A scan-side alternative — matching a bare `StrVal` sitting opposite a reference to a column of the enum's type — is self-contained and retroactive, but it is shape matching: it misses `IN ('a','b')`, function arguments, and `CASE`.
**Suggested scope**
- [ ] Store the type-checked WHEN expression; restore `NEW`/`OLD` names and outer parens
- [ ] Type PL/pgSQL `IF`-condition expressions when the body is inlined onto the table
- [ ] Route `information_schema.triggers` / `pg_catalog.pg_trigger` through the display formatter
- [ ] Decide migration vs. scan-side fallback for descriptors already written
- [ ] Flip the `enum_value_drop_with_triggers_untyped_gap` assertions to `2BP01`
**Environment**
master at `bbe7588b561`; applies to every version with trigger support.
Jira issue: CRDB-67459
Epic CRDB-65516
Contributor guide
Research direction
Start with pkg/sql/opt/optbuilder/create_trigger.go, especially buildWhenForTrigger and buildFunctionForTrigger, then inspect the PL/pgSQL builder and trigger display formatting paths named in the issue. Reproduce the gap with logic_test/triggers and review the enum_value_drop_with_triggers_untyped_gap assertions. Done means untyped WHEN and IF references are rejected with 2BP01, existing descriptors are handled, catalog output remains reparsable, and the assertions pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100