Re: CHECK Constraint Deferrable

On Tue, Sep 12, 2023 at 2:56 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:

> On Thu, 7 Sept 2023 at 17:26, Himanshu Upadhyaya
> <upadhyaya(dot)himanshu(at)gmail(dot)com> wrote:
> >
> > Attached is v2 of the patch, rebased against the latest HEAD.
>
> Thanks for working on this, few comments:
> 1) "CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t
> text)" is crashing in windows, the same was noticed in CFBot too:
> 2023-09-11 08:11:36.585 UTC [58563][client backend]
> [pg_regress/constraints][13/880:0] LOG: statement: CREATE TABLE
> check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> 2023-09-11 08:11:36.586 UTC [58560][client backend]
> [pg_regress/inherit][15/391:0] LOG: statement: drop table c1;
> ../src/backend/commands/trigger.c:220:26: runtime error: member access
> within null pointer of type 'struct CreateTrigStmt'
> ==58563==Using libbacktrace symbolizer.
>
> The details of CFBot failure can be seen at [1]
>
> I have tried it with my latest patch on windows environment and not
getting any crash with the above statement, will do further analysis if
this patch also has the same issue.

> 2) Alter of check constraint deferrable is not handled, is this
> intentional?
> CREATE TABLE check_constr_tbl (i int CHECK(i<>0) DEFERRABLE, t text);
> postgres=# alter table check_constr_tbl alter constraint
> check_constr_tbl_i_check not deferrable;
> ERROR: constraint "check_constr_tbl_i_check" of relation
> "check_constr_tbl" is not a foreign key constraint
>
> This is not allowed for any constraint type but FOREIGN key. I am not very
sure about if there is any limitation with this so wanted to take opinion
from other hackers on this.

> 3) Should we handle this scenario for domains too:
> CREATE DOMAIN c1_check AS INT CHECK(VALUE > 10);
> create table test(c1 c1_check);
> alter domain c1_check ADD check (VALUE > 20) DEFERRABLE INITIALLY DEFERRED;
>
> begin;
> -- should this be deffered
> insert into test values(19);
> ERROR: value for domain c1_check violates check constraint
> "c1_check_check1"
>
> We are planning to have a follow-up patch once this initial patch is
committed.

> 4) There is one warning:
> heap.c: In function ‘StoreRelCheck’:
> heap.c:2178:24: warning: implicit declaration of function
> ‘CreateTrigger’ [-Wimplicit-function-declaration]
> 2178 | (void) CreateTrigger(trigger, NULL,
> RelationGetRelid(rel),
> |

Fixed in V3 patch.

> ^~~~~~~~~~~~~
>
> 5) This should be added to typedefs.list file:
> +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;
>
> Fixed in V3 patch.

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