Re: Needless additional partition check in INSERT?

On 10 May 2018 at 15:26, David Rowley <david(dot)rowley(at)2ndquadrant(dot)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;

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company