On 11 May 2018 at 14:50, Amit Khandekar <amitdkhan...@gmail.com> wrote: > On 10 May 2018 at 15:26, David Rowley <david.row...@2ndquadrant.com> wrote: >> Yeah, the comments do need work. In order to make it a bit easier to >> document I changed the way that check_partition_constr is set. This is >> now done with an if/else if/else clause for both COPY and INSERT. >> >> Hopefully, that's easier to understand and prevents further mistakes. >> >> Patch attached. > > The patch looks good in terms of handling the redundant constraint check. > > Since this patch also does some minor code restructuring with the if > conditions, below are some comments on them : > > With the patch, the if condition uses ri_PartitionCheck, and further > ExecConstraints() also uses ri_PartitionCheck to decide whether to > call ExecPartitionCheck(). So instead, if we just don't bother about > ri_PartitionCheck outside ExecConstraints() and let ExecConstraints() > handle it, then the if conditions would get simpler : > > if (resultRelInfo->ri_PartitionRoot == NULL || > (resultRelInfo->ri_TrigDesc && > resultRelInfo->ri_TrigDesc->trig_insert_before_row)) > check_partition_constr = true; > else > check_partition_constr = false;
This looks better (it will avoid unnecessary ExecConstraints() call) : if (resultRelInfo->ri_PartitionRoot == NULL || (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_insert_before_row)) check_partition_constr = resultRelInfo->ri_PartitionCheck; else check_partition_constr = false; > > -- > Thanks, > -Amit Khandekar > EnterpriseDB Corporation > The Postgres Database Company -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company