Re: Needless additional partition check in INSERT?

On 2018/06/07 15:02, David Rowley wrote:
> On 7 June 2018 at 17:45, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2018/06/07 13:10, David Rowley wrote:
>>> On 7 June 2018 at 16:05, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>> Or we could just not have a separate function and put the logic that
>>>> determines whether or not to check the partition constraint right before
>>>> the following piece of code in ExecConstraints
>>>>
>>>>     if (check_partition_constraint && resultRelInfo->ri_PartitionCheck &&
>>>>         !ExecPartitionCheck(resultRelInfo, slot, estate))
>>>>         ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
>>>
>>> I don't think so. The function Alvaro is talking about is specific to
>>> inserting a tuple into a table. The place you're proposing to put the
>>> call to it is not.
>>
>> Well, we need to determine whether or not to check the partition
>> constraint exactly before inserting a tuple into a table and that's also
>> when ExecConstraints is called, so this objection is not clear to me.
>>
>> I'm saying that instead of encapsulating that logic in a new function and
>> calling it from a number of places, refactor things such that we call
>> ExecConstraints unconditionally and decide there which constraints to
>> check and which ones to not.  Having that logic separated from
>> ExecConstraints is what caused us to have this discussion in the first place.
>>
>> I'm attaching a patch here to show what I'm saying.
> 
> I might not have fully explained what I meant. I'm guilty of thinking
> it was pretty clear when I wrote my objection.
> 
> I meant:
> 
> ExecConstraints is not only called during INSERT and COPY TO, it's
> also used during UPDATE [1].

Thank you for clarifying.

> What you're proposing seems to be adding a check for
> trig_insert_before_row into a function that will be called during
> UPDATE.
> 
> Can you explain why you think that's a good thing to do? I'm don't
> really see why UPDATE should care about the presence, (or lack of) a
> BEFORE ROW INSERT trigger.

trig_insert_before_row is checked only if tuple routing has occurred
(resultRelInfo->ri_PartitionRoot != NULL), which can only be true if
ExecConstraints is called either from CopyFrom or ExecInsert.  You may
know that even UPDATE tuple routing internally calls ExecInsert for the
actual routing to occur.  The point here is that we want to avoid doing
the partition constraint check if tuple routing has occurred, except the
presence of a BR trigger will require it to be performed.  Tuple routing
only happens in CopyFrom or ExecInsert, so checking trig_insert_before_row
here doesn't sound wildly odd to me.

There is one twist though.  It may happen that ExecUpdate ends up
performing tuple routing for one row and thus will have ri_PartitionRoot
set.  For another row, tuple routing may not be needed as the updated
version of the row satisfies the partition constraint and now because
ri_PartitionRoot has been set, ExecConstraint would end up performing the
partition constraint check redundantly if trig_insert_before_row is true.

Thanks,
Amit