Re: Concurrency bug in UPDATE of partition-key

On 7 June 2018 at 11:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Jun 5, 2018 at 8:03 PM, Amit Khandekar <amitdkhan.pg@gmail.com>
> wrote:
>>
>> Attached is a rebased patch version. Also included it in the upcoming
>> commitfest :
>> https://commitfest.postgresql.org/18/1660/
>>
>
> Doesn't this belong to PostgreSQL 11 Open Items [1] or are you proposing it
> as a feature enhancement for next version?

Yes, it needs to be in the Open items. I will have it added in that list.

>
>>
>> In the rebased version, the new test cases are added in the existing
>> isolation/specs/partition-key-update-1.spec test.
>>
>
>   if (!tuple_deleted)
> - return NULL;
> + {
> + /*
> + * epqslot will be typically NULL. But when ExecDelete() finds
> + * that another transaction has concurrently updated the same
> + * row, it re-fetches the row, skips the delete, and epqslot is
> + * set to the re-fetched tuple slot. In that case, we need to
> + * do all the checks again.
> + */
> + if (TupIsNull(epqslot))
> + return NULL;
> + else
> + {
> + slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
> + tuple = ExecMaterializeSlot(slot);
> + goto lreplace;
> + }
> + }
>
> I think this will allow before row delete triggers to be fired more than
> once.  Normally, if the EvalPlanQual testing generates a new tuple, we don't
> fire the triggers again.

If there are BR delete triggers, the tuple will be locked using
GetTupleForTrigger(). So the subsequent EvalPlanQual testing won't be
run, since the tuple is already locked due to triggers having run.

But that leads me to think : The same concurrency issue can occur in
GetTupleForTrigger() also. Say, a concurrent session has already
locked the tuple, and GetTupleForTrigger() would wait and then return
the updated tuple in its last parameter newSlot. In that case, we need
to pass this slot back through ExecBRDeleteTriggers(), and further
through epqslot parameter of ExecDelete(). But yes, in this case, we
should avoid calling this trigger function the second time.

If you agree on the above, I will send an updated patch.


> Now, one possibility could be that we don't skip
> the delete after a concurrent update and still allow inserts to use a new
> tuple generated by EvalPlanQual testing of delete.  However, I think we need
> to perform rechecks for update to check if the modified tuple still requires
> to be moved to new partition, right or do you have some other reason in
> mind?

Yes, that's the reason we need to start again from lreplace; i.e., for
checking not just the partition constraints but all the other checks
that were required earlier, because the tuple has been modified. It
may even end up moving to a different partition than the one chosen
earlier.



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