Re: UPDATE of partition key

For some reason, my reply got sent to only Amit Langote instead of
reply-to-all. Below is the mail reply. Thanks Amit Langote for
bringing this to my notice.

On 31 March 2017 at 16:54, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 31 March 2017 at 14:04, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/03/28 19:12, Amit Khandekar wrote:
>>> In the section 5.11 "Partitioning" => subsection 5 "Caveats", I have
>>> removed the caveat of not being able to update partition key. And it
>>> is now replaced by the caveat where an update/delete operations can
>>> silently miss a row when there is a concurrent UPDATE of partition-key
>>> happening.
>>
>> Hmm, how about just removing the "partition-changing updates are
>> disallowed" caveat from the list on the 5.11 Partitioning page and explain
>> the concurrency-related caveats on the UPDATE reference page?
>
> IMHO this caveat is better placed in Partitioning chapter to emphasize
> that it is a drawback specifically in presence of partitioning.
>
>> +    If an <command>UPDATE</command> on a partitioned table causes a row to
>> +    move to another partition, it is possible that all row-level
>> +    <literal>BEFORE</> <command>UPDATE</command> triggers and all row-level
>> +    <literal>BEFORE</> <command>DELETE</command>/<command>INSERT</command>
>> +    triggers are applied on the respective partitions in a way that is
>> apparent
>> +    from the final state of the updated row.
>>
>> How about dropping "it is possible that" from this sentence?
>
> What the statement means is : "It is true that all triggers are
> applied on the respective partitions; but it is possible that they are
> applied in a way that is apparent from final state of the updated
> row". So "possible" applies to "in a way that is apparent..". It
> means, the user should be aware that all the triggers can change the
> row and so the final row will be affected by all those triggers.
> Actually, we have a similar statement for UPSERT involved with
> triggers in the preceding section. I have taken the statement from
> there.
>
>>
>> +    <command>UPDATE</command> is done by doing a <command>DELETE</command> on
>>
>> How about: s/is done/is performed/g
>
> Done.
>
>>
>> +    triggers are not applied because the <command>UPDATE</command> is
>> converted
>> +    to a <command>DELETE</command> and <command>UPDATE</command>.
>>
>> I think you meant DELETE and INSERT.
>
> Oops. Corrected.
>
>>
>> +        if (resultRelInfo->ri_PartitionCheck &&
>> +            !ExecPartitionCheck(resultRelInfo, slot, estate))
>> +        {
>>
>> How about a one-line comment what this block of code does?
>
> Yes, this was needed. Added a comment.
>
>>
>> -         * Check the constraints of the tuple.  Note that we pass the same
>> +         * Check the constraints of the tuple. Note that we pass the same
>>
>> I think that this hunk is not necessary.  (I've heard that two spaces
>> after a sentence-ending period is not a problem [1].)
>
> Actually I accidentally removed one space, thinking that it was one of
> my own comments. Reverted back this change, since it is a needless
> hunk.
>
>>
>> +         * We have already run partition constraints above, so skip them below.
>>
>> How about: s/run/checked the/g?
>
> Done.
>
>> @@ -2159,6 +2289,7 @@ ExecEndModifyTable(ModifyTableState *node)
>>          heap_close(pd->reldesc, NoLock);
>>          ExecDropSingleTupleTableSlot(pd->tupslot);
>>      }
>> +
>>      for (i = 0; i < node->mt_num_partitions; i++)
>>      {
>>          ResultRelInfo *resultRelInfo = node->mt_partitions + i;
>>
>> Needless hunk.
>
> Right. Removed.
>
>>
>> Overall, I think the patch looks good now.  Thanks again for working on it.
>
> Thanks Amit for your efforts in reviewing the patch. Attached is v6
> patch that contains above points handled.



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