Re: CHECK Constraint Deferrable

On Thu, Sep 7, 2023 at 1:25 PM Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
>
> Attached is v2 of the patch, rebased against the latest HEAD.

I have done some initial reviews, and here are my comments.  More
detailed review later.  Meanwhile, you can work on these comments and
fix all the cosmetics especially 80 characters per line

1.
+
+ (void) CreateTrigger(trigger, NULL, RelationGetRelid(rel),
+ InvalidOid, constrOid, InvalidOid, InvalidOid,
+ InvalidOid, NULL, true, false);

heap.c is calling CreateTrigger but the inclusion of
"commands/trigger.h" is missing.

2.
- if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
+ if ((failed = ExecRelCheck(resultRelInfo, slot, estate,
checkConstraint, &recheckConstraints)) != NULL && !recheckConstraints)


Why recheckConstraints need to get as output from ExecRelCheck?  I
mean whether it will be rechecked or nor is already known by the
caller and
Whether the constraint failed or passed is known based on the return
value so why do you need to extra parameter here?

3.
-void
+bool
 ExecConstraints(ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot, EState *estate)
+ TupleTableSlot *slot, EState *estate, checkConstraintRecheck checkConstraint)
 {

- if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
+ if ((failed = ExecRelCheck(resultRelInfo, slot, estate,
checkConstraint, &recheckConstraints)) != NULL && !recheckConstraints)

 take care of postgres coding style and break line after 80
characters.  Check other places as well in the patch.

4.
+ if (checkConstraint == CHECK_RECHECK_ENABLED && check[i].ccdeferrable)
+ {
+ *recheckConstraints = true;
+ }

Remove curly brackets around single-line block

5.
+typedef enum checkConstraintRecheck
+{
+ CHECK_RECHECK_DISABLED, /* Recheck of CHECK constraint is disabled, so
+ * DEFERRED CHECK constraint will be
+ * considered as non-deferrable check
+ * constraint.  */
+ CHECK_RECHECK_ENABLED, /* Recheck of CHECK constraint is enabled, so
+ * CHECK constraint will be validated but
+ * error will not be reported for deferred
+ * CHECK constraint. */
+ CHECK_RECHECK_EXISTING /* Recheck of existing violated CHECK
+ * constraint, indicates that this is a
+ * deferred recheck of a row that was reported
+ * as a potential violation of CHECK
+ * CONSTRAINT */
+} checkConstraintRecheck;

I do not like the naming convention here, especially the words
RECHECK, DISABLE, and ENABLE. And also the name of the enum is a bit
off.  We can name it more like a unique constraint
YES, PARTIAL, EXISTING


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com