Re: Concurrency bug in UPDATE of partition-key

On Tue, Jun 26, 2018 at 11:40 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 25 June 2018 at 17:20, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Mon, Jun 25, 2018 at 3:06 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>>> On 23 June 2018 at 15:46, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>>>
>>>>
>>>> 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.
>>>
>>> But GetTupleForTrigger() updates the epqslot to NULL even when it
>>> returns NULL. So to be consistent with it, we want to do the same
>>> thing for ExecBRDeleteTriggers() : Update the epqslot even when
>>> GetTupleForTrigger() returns NULL.
>>>
>>> I think the reason we are doing "*newSlot = NULL" as the very first
>>> thing in the GetTupleForTrigger() code, is so that callers don't have
>>> to initialise newSlot to NULL before calling GetTupleForTrigger. And
>>> so ExecBRUpdateTriggers() does not initialize newSlot with NULL. We
>>> can follow the same approach while calling ExecDelete().
>>>
>>
>> Yeah, we can do that if it is required.
>
> It is not required as such, and there is no correctness issue.
>
>> I see your point, but I feel that is making code bit less readable.
>
> I did it that way only to be consistent with the existing trigger.c
> code, namely ExecBRUpdateTriggers() where it sets newSlot to NULL
> immediately. If you find some appropriate comments to make that
> snippet more readable, that can work.
>
> Or else, we can go ahead with the way you did it in your patch; and
> additionally mention in the ExecBRDeleteTriggers() header comments
> that epqslot value is undefined if there was no concurrently updated
> tuple passed. To me, explaining this last part seems confusing.
>

Yeah, so let's leave it as it is in the patch.  I think now we should
wait and see what Alvaro has to say about the overall patch.

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