On Thu, Sep 7, 2023 at 1:25 PM Himanshu Upadhyaya <upadhyaya.himan...@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