src/oracle_test/regress/sql/constraints.sql (1)

1044-1366: Comprehensive test coverage for constraint ENABLE/DISABLE syntax.

The test suite thoroughly exercises all constraint types (PRIMARY KEY, UNIQUE, CHECK, NOT NULL) with all ENABLE/DISABLE and VALIDATE/NOVALIDATE combinations. The pattern of creating tables and querying pg_constraint to verify enforcement and validation flags is appropriate for regression testing.

Minor suggestion for clarity:

The comments "Not supported" and "Not supported - XXX NOT VALID supported on ALTER TABLE" could be more explicit about expected behavior. Consider clarifying whether these cases should:

• Produce an error with a HINT message
• Silently treat NOVALIDATE as unsupported
• Create the constraint with specific flag values

This would help future maintainers understand the intended behavior.

src/oracle_test/regress/expected/foreign_key.out (1)

3408-3625: fkpart14 FK ENABLE/DISABLE tests look correct and complete

The new fkpart14 block exercises foreign key ENABLE/DISABLE with VALIDATE/NOVALIDATE across inline, column, and named FKs, and the expected pg_constraint flags and error/HINT combinations all align with the documented mapping:

• ENABLE / ENABLE VALIDATE → conenforced = t, convalidated = t.
• DISABLE / DISABLE NOVALIDATE → conenforced = f, convalidated = f.
• ENABLE NOVALIDATE and DISABLE VALIDATE → explicit “FOREIGN KEY … is not supported” errors with a HINT and no leftover constraints.

If you want even stronger coverage, you could optionally add a tiny DML snippet to confirm that a DISABLE [NOVALIDATE] FK indeed allows inserting non-matching rows and that ENABLE still enforces, but that’s already well-covered earlier in this file and not strictly necessary here.

src/oracle_test/regress/expected/constraints.out (1)

1692-2311: CREATE TABLE ENABLE/DISABLE constraint tests are thorough and consistent

The ivo_regress_enable block does a good job of:

• Exercising ENABLE/DISABLE with VALIDATE/NOVALIDATE across PRIMARY KEY, UNIQUE, CHECK, and NOT NULL, in both column and table-constraint forms.
• Verifying that supported forms map to the intended conenforced/convalidated combinations and that unsupported forms yield clear, type-specific “<TYPE> <STATE> is not supported.” errors with a HINT and no residual constraints in pg_constraint.
• Including an explicit syntax error case (id NUMBER ENABLE) plus cleanup via DROP SCHEMA … CASCADE and RESET search_path;.

One small optional polish: for cases like PRIMARY KEY DISABLE NOVALIDATE and NOT NULL DISABLE NOVALIDATE, you might consider mentioning DISABLE NOVALIDATE explicitly in the error text (rather than just DISABLE) to more directly reflect the user’s input, but the current behavior is functionally fine.

src/backend/oracle_parser/ora_gram.y (2)

6644-6692: ENABLE/DISABLE grammar now applies to all ConstraintAttributeSpec users (ALTER, DOMAIN, TRIGGER)

By:

• Extending %type to include OraConstraintAttributeElem (line 615),
• Adding OraConstraintAttributeElem into ConstraintAttributeElem and as its own alternative in ConstraintAttributeSpec (lines 6644–6683),
• Introducing the OraConstraintAttributeElem nonterminal and NOVALIDATE keyword (lines 6685–6692, 781–782),

the { ENABLE | DISABLE } [ VALIDATE | NOVALIDATE ] syntax is now accepted anywhere ConstraintAttributeSpec is used, not just in CREATE-table/column constraints. That includes:

• ALTER TABLE ... ALTER CONSTRAINT ... ConstraintAttributeSpec
• CREATE DOMAIN / ALTER DOMAIN constraints
• CREATE CONSTRAINT TRIGGER ... ConstraintAttributeSpec

This is broader than the PR description (“currently applies only to CREATE statements; ALTER and DOMAIN are not supported”), and some of these combinations hit generic processCASbits errors like “%s constraints cannot be marked ENFORCED/NOT ENFORCED”, which may be surprising to users of the new syntax.

If the intent is truly “CREATE only”, consider:

• Using a separate nonterminal (e.g. OraConstraintAttributeSpec) wired only into the CREATE‑time constraint productions, or
• Guarding unsupported call sites early (e.g. reject any CAS_ENABLE/CAS_DISABLE bits when constrType is "TRIGGER" or invoked from ALTER/DOMAIN paths) with a clear Oracle‑style error/HINT.

Otherwise, please update the PR docs/tests to explicitly call out that ALTER/DOMAIN also accept the syntax and verify the behavior you want at those sites.

Also applies to: 615-615, 781-782

---

4278-4482: Column-level ENABLE/DISABLE [NO]VALIDATE behavior looks consistent, but a few edge cases need decisions

The new OraConstraintAttributeElem usages in ColConstraintElem:

• NOT NULL_P OraConstraintAttributeElem
• UNIQUE ... OraConstraintAttributeElem
• PRIMARY KEY ... OraConstraintAttributeElem
• CHECK (...) opt_no_inherit OraConstraintAttributeElem
• REFERENCES ... key_actions OraConstraintAttributeElem

plus the updated processCASbits give:

• REFERENCES ... DISABLE NOVALIDATE → is_enforced = false, skip_validation = true, initially_valid = false (as desired for “not enforced, not validated”).
• CHECK ... DISABLE NOVALIDATE → same pattern with is_enforced = false and initially_valid = false.
• PRIMARY KEY|UNIQUE|NOT NULL ... DISABLE[*] → hard error (“... DISABLE is not supported”) as per the PR text.
• PRIMARY KEY|UNIQUE|NOT NULL ... ENABLE[*] → treated as redundant, logged, and ignored.

Two things to lock down:

1. Plain DISABLE vs DISABLE NOVALIDATE for CHECK/FK
   Right now both map to CAS_DISABLE (i.e., treated as “NOT ENFORCED and NOT VALID”). If you intend to mirror Oracle’s distinction between DISABLE VALIDATE and DISABLE NOVALIDATE, you might at least want to:

   • Continue erroring on DISABLE VALIDATE (as you do now), and
   • Decide whether bare DISABLE should be an alias for DISABLE NOVALIDATE (current behavior) or be rejected as ambiguous.

2. Column-level NOT NULL with ENABLE syntax
   NOT NULL ENABLE is accepted but only logged at LOG level and treated identically to bare NOT NULL. If you want users to see that ENABLE is redundant/Oracle‑only, consider WARNING/NOTICE instead of LOG, or just silently ignore it and document as accepted Oracle noise.

These are more semantics/UX than correctness issues, but clarifying them now will prevent surprising behavior once people start relying on the Oracle compatibility layer.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/include/nodes/parsenodes.h (3)

1765-1776: MergeWhenClause update_delete linkage

The update_delete self-pointer on MergeWhenClause is a reasonable way to represent Oracle-style UPDATE ... DELETE combinations. Just ensure this new field is included in copy/equal/read/write support for MergeWhenClause so rules and stored queries round-trip correctly.

---

3250-3260: CreateSeqStmt seq_type semantics should be documented

The new char seq_type flag (“is oracle compatible type or original type”) is fine structurally, but it would benefit from a clearer comment describing the allowed values and their meaning (e.g., which char indicates Oracle-compatible behavior vs native). That will avoid future ambiguity when interpreting this field.

---

3611-3655: New DO / InlineCodeBlock / CALL fields: check lifecycle and node support

The additions:

• DoStmt.paramsmode / DoStmt.paramslen
• InlineCodeBlock.params
• CallStmt.hostvariable and CallStmt.callinto

all look consistent with extending PL/SQL-style call/DO support. A few points to verify:

• InlineCodeBlock and CallStmt are used outside parse trees at execution time; ensure ownership/lifetime of ParamListInfo params and OraParamRef *hostvariable is clear so there are no dangling pointers.
• Confirm all three structs are updated in copy/equal/read/write helpers as appropriate (for DoStmt and CallStmt, which are parse nodes; InlineCodeBlock is tagged nodetag_only but still flows through some infrastructure).

src/backend/commands/typecmds.c (1)

1032-1040: Domain constraint switch correctly rejects ENABLE/DISABLE attributes

Extending the DefineDomain constraint-type switch to include CONSTR_ATTR_ENABLE and CONSTR_ATTR_DISABLE alongside CONSTR_ATTR_ENFORCED / CONSTR_ATTR_NOT_ENFORCED ensures that Oracle-style ENABLE/DISABLE attributes on domains are rejected consistently, which aligns with the stated “not supported for domains” behavior.

If you want to be extra explicit for Oracle users, you might consider slightly broadening the error text (e.g., mention ENABLE/DISABLE in the message or HINT), but the current wording is functionally correct.

src/oracle_test/regress/sql/foreign_key.sql (1)

2390-2558: Consider adding DML operations to verify actual constraint behavior.

The fkpart14 test block comprehensively tests the syntax and metadata storage for FK constraints with ENABLE/DISABLE/VALIDATE/NOVALIDATE clauses. However, it only queries pg_constraint to verify the flags—it doesn't verify that the constraints actually enforce (or don't enforce) the expected behavior through INSERT/UPDATE/DELETE operations.

💡 Suggested enhancement

Consider adding DML verification for at least a few representative cases. For example:

-- After creating ENABLE constraint
INSERT INTO fk_r VALUES (1, 'test');
INSERT INTO fk_p1 VALUES (1, 'valid');    -- should succeed
INSERT INTO fk_p1 VALUES (999, 'invalid'); -- should fail

-- After creating DISABLE constraint  
INSERT INTO fk_p4 VALUES (999, 'invalid'); -- should succeed (not enforced)

-- Clean up
DELETE FROM fk_p1;
DELETE FROM fk_p4;
DELETE FROM fk_r;

This would provide functional validation in addition to the metadata checks.

As per coding guidelines, these are SQL test files that should ensure comprehensive coverage of features.

src/backend/parser/parse_utilcmd.c (1)

4311-4316: Inconsistent error formatting for duplicate clause detection.

The error for multiple ENABLE/DISABLE clauses includes an errhint in the DISABLE case (line 4315-4316) but not in the ENABLE case (line 4264). For consistency, either both should have the hint or neither should.

🔧 Proposed fix: Remove unnecessary hint for consistency

 			if (saw_enforced)
 				ereport(ERROR,
 						(errcode(ERRCODE_SYNTAX_ERROR),
 						 errmsg("multiple ENABLE/DISABLE clauses not allowed"),
-						 errhint("Update your CONSTRAINT definition."),
 						 parser_errposition(cxt->pstate, con->location)));

src/backend/oracle_parser/ora_gram.y (3)

4568-4627: ENABLE/DISABLE [NO]VALIDATE ConstraintAttr semantics need downstream handling

The new ConstraintAttr variants for:

• ENABLE [VALIDATE|NOVALIDATE]
• DISABLE [VALIDATE|NOVALIDATE]

populate contype = CONSTR_ATTR_ENABLE/DISABLE plus skip_validation and initially_valid. This is reasonable, but behavior depends on parse/analyze code actually interpreting these fields for CONSTR_ATTR_ENABLE/CONSTR_ATTR_DISABLE (e.g., in parse_utilcmd.c).

If downstream still only looks at the legacy CONSTR_ATTR_[NOT_]DEFERRABLE/... values, these new attributes may be silently ignored.

Consider:

• Verifying that all Constraint-attribute handling switch statements cover the new CONSTR_ATTR_ENABLE / CONSTR_ATTR_DISABLE variants.
• Adding a defensive default: in such switches that errors if an unsupported attribute contype is seen, to avoid silent no-ops.

Would you like me to generate a small rg/ast-grep script to scan for CONSTR_ATTR_ usages and confirm all cases handle the new values?

---

6733-6750: OraConstraintAttributeElem → CAS_ mapping is fine but lacks conflict checks*

Mapping:

• ENABLE → CAS_ENABLE
• DISABLE → CAS_DISABLE
• VALIDATE → CAS_VALIDATE (no-op, default)
• NOVALIDATE → CAS_NOVALIDATE

is straightforward and works with later processCASbits() remapping. However, ConstraintAttributeSpec’s conflict checks only look at CAS_NOT_DEFERRABLE/DEFERRABLE, CAS_INITIALLY_*, and CAS_NOT_ENFORCED/ENFORCED, so combinations like ENABLE DISABLE or ENFORCED DISABLE are not detected there and currently rely entirely on processCASbits() behavior.

Given the PR description mentions future tests for “duplicate/opposite clauses”, it may be worth:

• Either adding explicit conflict checks for CAS_ENABLE/CAS_DISABLE and CAS_VALIDATE/CAS_NOVALIDATE in ConstraintAttributeSpec, or
• Documenting that these combinations are undefined for now so tests don’t accidentally rely on the current behavior.

---

22201-22345: processCASbits: ENABLE/DISABLE/[NO]VALIDATE mapping mostly correct, but be careful with unsupported callers

The updated processCASbits():

• Maps:
  • CAS_ENABLE → CAS_ENFORCED
  • CAS_DISABLE → CAS_NOT_ENFORCED
  • CAS_NOVALIDATE → CAS_NOT_VALID
• Issues Oracle-style errors/hints when:
  • NOVALIDATE or DISABLE are used on constraint types that don’t support NOT VALID or NOT ENFORCED.

The main semantic points look good, but there are a couple of edge considerations:

• Callers that pass NULL for is_enforced or not_valid now rely on the new if (cas_bits & CAS_DISABLE) / if (cas_bits & CAS_NOVALIDATE) branches for error reporting. That’s fine, but it means any future caller added with NULL pointers will get Oracle-style error text by default, which might not always be desired.
• There is no second-stage conflict check after CAS_ENABLE/CAS_DISABLE are mapped to CAS_ENFORCED/CAS_NOT_ENFORCED, so combinations like ENABLE DISABLE or ENFORCED DISABLE end up with both bits set but are not flagged here (only plain ENFORCED vs NOT ENFORCED conflicts are blocked in ConstraintAttributeSpec). If you care about rejecting such opposite combinations, you may want a small consistency check after the CAS_ENABLE/DISABLE remap.

Overall the mapping is consistent, but it would be good to:

• Audit all processCASbits call sites to ensure that combinations of ENABLE/DISABLE/[NO]VALIDATE either have supported pointer fields or get the desired error behavior.
• Optionally add a post-remap assertion for (CAS_ENFORCED | CAS_NOT_ENFORCED) being non-conflicting if you want to hard-fail on contradictory specs.

Would you like a quick grep-based script to list all processCASbits( call sites and summarize which boolean pointer arguments are NULL vs non-NULL?

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

src/backend/parser/parse_utilcmd.c (1)

4255-4260: Inconsistent error code for "misplaced" error.

The error uses ERRCODE_FEATURE_NOT_SUPPORTED with message "misplaced ENABLE clause", but all other "misplaced" errors in this function (e.g., lines 4152, 4163, 4188, 4230) use ERRCODE_SYNTAX_ERROR.

For consistency, consider using ERRCODE_SYNTAX_ERROR here as well.

🛠️ Proposed fix for consistency

 			{
 				ereport(ERROR,
-						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						(errcode(ERRCODE_SYNTAX_ERROR),
 						 errmsg("misplaced ENABLE clause"),
 						 parser_errposition(cxt->pstate, con->location)));
 			}

src/oracle_test/regress/expected/constraints.out (1)

2585-2631: Consider ordering multi-row pg_constraint outputs to avoid nondeterminism.

The multi-row queries (Lines 2585–2593 and 2625–2631) don’t specify an ORDER BY. This relies on heap/insert order and can be fragile if catalog changes reorder rows. Recommend adding ORDER BY conname in the SQL and updating the expected output accordingly.
As per coding guidelines, “These are expected test outputs from pg_regress. Review for: Correctness of query results; Proper Oracle compatibility behavior; Edge case coverage; Consistency with corresponding .sql test files”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/oracle_test/regress/expected/constraints.out` around lines 2585 - 2631,
The SELECT queries against pg_constraint that return multiple rows (the
statements starting "SELECT conname, contype, conenforced, convalidated FROM
pg_constraint WHERE conrelid = 'enable_noconflict'::regclass;" and the similar
ones for 'disable_noconflict' and 'novalidate_noconflict') are nondeterministic;
add "ORDER BY conname" to each of those SELECTs to produce a stable ordering and
then update the expected output blocks to match the new sorted ordering. Ensure
you modify each multi-row SELECT that queries pg_constraint (referenced above)
and regenerate the expected output so the test comparisons are deterministic.