Re: CHECK Constraint Deferrable

On Tue, Sep 19, 2023 at 4:14 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:

> > I think we should be able to defer one constraint like in the case of
> > foreign key constraint
> >
>
> Agreed. It should be possible to have a mix of deferred and immediate
> constraint checks. In the example, the tbl_chk_1 is set deferred, but
> it fails immediately, which is clearly not right.
>
> Fixed in V3 patch.

> I would say that it's reasonable to limit the scope of this patch to
> table constraints only, and leave domain constraints to a possible
> follow-up patch.
>
> Sure, Agree.

> A few other review comments:
>
> 1. The following produces a WARNING (possibly the same issue already
> reported):
>
> CREATE TABLE foo (a int, b int);
> ALTER TABLE foo ADD CONSTRAINT a_check CHECK (a > 0);
> ALTER TABLE foo ADD CONSTRAINT b_check CHECK (b > 0) DEFERRABLE;
>
> WARNING: unexpected pg_constraint record found for relation "foo"
>
> fixed in V3 patch.

> 2. I think that equalTupleDescs() should compare the new fields, when
> comparing the 2 sets of check constraints.
>
> Fixed in V3 patch.

> 3. The constraint exclusion code in the planner should ignore
> deferrable check constraints (see get_relation_constraints() in
> src/backend/optimizer/util/plancat.c), otherwise it might incorrectly
> exclude a relation on the basis of a constraint that is temporarily
> violated, and return incorrect query results. For example:
>
> CREATE TABLE foo (a int);
> CREATE TABLE foo_c1 () INHERITS (foo);
> CREATE TABLE foo_c2 () INHERITS (foo);
> ALTER TABLE foo_c2 ADD CONSTRAINT cc CHECK (a != 5) INITIALLY DEFERRED;
>
> BEGIN;
> INSERT INTO foo_c2 VALUES (5);
> SET LOCAL constraint_exclusion TO off;
> SELECT * FROM foo WHERE a = 5;
> SET LOCAL constraint_exclusion TO on;
> SELECT * FROM foo WHERE a = 5;
> ROLLBACK;
>
> Fixed in V3 patch.

> 4. The code in MergeWithExistingConstraint() should prevent inherited
> constraints being merged if their deferrable properties don't match
> (as MergeConstraintsIntoExisting() does, since
> constraints_equivalent() tests the deferrable fields). I.e., the
> following should fail to merge the constraints, since they don't
> match:
>
> DROP TABLE IF EXISTS p,c;
>
> CREATE TABLE p (a int, b int);
> ALTER TABLE p ADD CONSTRAINT c1 CHECK (a > 0) DEFERRABLE;
> ALTER TABLE p ADD CONSTRAINT c2 CHECK (b > 0);
>
> CREATE TABLE c () INHERITS (p);
> ALTER TABLE c ADD CONSTRAINT c1 CHECK (a > 0);
> ALTER TABLE c ADD CONSTRAINT c2 CHECK (b > 0) DEFERRABLE;
>
> I.e., that should produce an error, as happens if c is made to inherit
> p *after* the constraints have been added.
>
> Fixed in V3 patch.

> 5. Instead of just adding the new fields to the end of the ConstrCheck
> struct, and to the end of lists of function parameters like
> StoreRelCheck(), and other related code, it would be more logical to
> put them immediately before the valid/invalid entries, to match the
> order of constraint properties in pg_constraint, and functions like
> CreateConstraintEntry().
>
> Fixed in V3 patch.

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com