On Tue, Sep 19, 2023 at 4:14 PM Dean Rasheed <dean.a.rash...@gmail.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
