Re: Concurrency bug in UPDATE of partition-key

On Fri, Jun 22, 2018 at 10:25 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> On 20 June 2018 at 18:54, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Tue, Jun 19, 2018 at 9:33 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>>
>> 2.
>> ExecBRDeleteTriggers(..)
>> {
>> ..
>> + /* If requested, pass back the concurrently updated tuple, if any. */
>> + if (epqslot != NULL)
>> + *epqslot = newSlot;
>> +
>> if (trigtuple == NULL)
>> return false;
>> +
>> + /*
>> + * If the tuple was concurrently updated and the caller of this
>> + * function requested for the updated tuple, skip the trigger
>> + * execution.
>> + */
>> + if (newSlot != NULL && epqslot != NULL)
>> + return false;
>> ..
>> }
>>
>> Can't we merge the first change into second, something like:
>>
>> if (newSlot != NULL && epqslot != NULL)
>> {
>> *epqslot = newSlot;
>> return false;
>> }
>
> We want to update *epqslot with whatever the value of newSlot is. So
> if newSlot is NULL, epqslot should be NULL. So we can't do that in the
> "if (newSlot != NULL && epqslot != NULL)" condition.
>

Why do you need to update when newslot is NULL? Already *epqslot is
initialized as NULL in the caller (ExecUpdate). I think we only want
to update it when trigtuple is not null. Apart from looking bit
awkward, I think it is error-prone as well because the code of
GetTupleForTrigger is such that it can return NULL with a valid value
of newSlot in which case the behavior will become undefined. The case
which I am worried is when first-time EvalPlanQual returns some valid
value of epqslot, but in the next execution after heap_lock_tuple,
returns NULL. In practise, it won't happen because EvalPlanQual locks
the tuple, so after that heap_lock_tuple shouldn't fail again, but it
seems prudent to not rely on that unless we need to.

For now, I have removed it in the attached patch, but if you have any
valid reason, then we can add back.

>>
>> 4.
>> +/* ----------
>> + * ExecBRDeleteTriggers()
>> + *
>> + * Called to execute BEFORE ROW DELETE triggers.
>> + *
>> + * Returns false in following scenarios :
>> + * 1. Trigger function returned NULL.
>> + * 2. The tuple was concurrently deleted OR it was concurrently updated and the
>> + * new tuple didn't pass EvalPlanQual() test.
>> + * 3. The tuple was concurrently updated and the new tuple passed the
>> + * EvalPlanQual() test, BUT epqslot parameter is not NULL. In such case, this
>> + * function skips the trigger function execution, because the caller has
>> + * indicated that it wants to further process the updated tuple. The updated
>> + * tuple slot is passed back through epqslot.
>> + *
>> + * In all other cases, returns true.
>> + * ----------
>> + */
>> bool
>> ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
>>
..
>
> If it looks complicated, can you please suggest something to make it a
> bit simpler.
>

See attached.

Apart from this, I have changed few comments and fixed indentation
issues. Let me know what you think about attached?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com