Re: Concurrency bug in UPDATE of partition-key

On 25 June 2018 at 17:20, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Jun 25, 2018 at 3:06 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>> On 23 June 2018 at 15:46, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

>
>> I understand that before calling ExecDelete() epqslot is initialized
>> to NULL, so it is again not required to do the same inside
>> GetTupleForTrigger(), but as per my above explanation, it is actually
>> not necessary to initialize to NULL before calling ExecDelete(). So I
>> can even remove that initialization.
>>
>
> If you remove that initialization, then won't you need to do something
> in ExecDelete() as well because there the patch assigns a value to
> epqslot only if EvalPlanQual returns non-null value?

Ah, right. So skipping the initialization before calling ExecDelete()
is not a good idea then.

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