Re: UPDATE of partition key

Hi Amit,

Thanks for the updated patches.

On 2017/03/28 19:12, Amit Khandekar wrote:
> On 27 March 2017 at 13:05, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>>> Also, there are a few places in the documentation mentioning that such updates cause error,
>>> which will need to be updated.  Perhaps also add some explanatory notes
>>> about the mechanism (delete+insert), trigger behavior, caveats, etc.
>>> There were some points discussed upthread that could be mentioned in the
>>> documentation.
>>> Yeah, I agree. Documentation needs some important changes. I am still
>>> working on them.
> 
> Attached patch v5 has above required doc changes added.
> 
> 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?

> UPDATE row movement behaviour is described in :
> Part VI "Reference => SQL Commands => UPDATE
> 
>> On 4 March 2017 at 12:49, Robert Haas <robertmhaas@gmail.com> wrote:
>>> How about running the BR update triggers for the old partition and the
>>> AR update triggers for the new partition?  It seems weird to run BR
>>> update triggers but not AR update triggers.  Another option would be
>>> to run BR and AR delete triggers and then BR and AR insert triggers,
>>> emphasizing the choice to treat this update as a delete + insert, but
>>> (as Amit Kh. pointed out to me when we were in a room together this
>>> week) that precludes using the BEFORE trigger to modify the row.
>>
>> I checked the trigger behaviour in case of UPSERT. Here, when there is
>> conflict found, ExecOnConflictUpdate() is called, and then the
>> function returns immediately, which means AR INSERT trigger will not
>> fire. And ExecOnConflictUpdate() calls ExecUpdate(), which means BR
>> and AR UPDATE triggers will be fired. So in short, when an INSERT
>> becomes an UPDATE, BR INSERT triggers do fire, but then the BR UPDATE
>> and AR UPDATE also get fired. On the same lines, it makes sense in
>> case of UPDATE=>DELETE+INSERT operation to fire a BR UPDATE trigger on
>> the original table, and then the BR and AR DELETE/INSERT triggers on
>> the respective tables.
>>
>> So the common policy can be :
>> Fire the BR trigger. It can be INESRT/UPDATE/DELETE trigger depending
>> upon what the statement is.
>> If there is a change in the operation, according to what the operation
>> is converted to (UPDATE->DELETE+INSERT or INSERT->UPDATE), all the
>> respective triggers would be fired.
> 
> The current patch already has the behaviour as per above policy. So I
> have included the description of this trigger related behaviour in the
> "Overview of Trigger Behavior" section of the docs. This has been
> derived from the way it is written for trigger behaviour for UPSERT in
> the preceding section.

I tested how various row-level triggers behave and it all seems to work as
you have described in your various messages, which the latest patch also
documents.

Some comments on the patch itself:

-      An <command>UPDATE</> that causes a row to move from one partition to
-      another fails, because the new value of the row fails to satisfy the
-      implicit partition constraint of the original partition.  This might
-      change in future releases.
+      An <command>UPDATE</> causes a row to move from one partition to
another
+      if the new value of the row fails to satisfy the implicit partition
<snip>

As mentioned above, we can simply remove this item from the list of
caveats on ddl.sgml.  The new text can be moved to the Notes portion of
the UPDATE reference page.

+    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?

+    <command>UPDATE</command> is done by doing a <command>DELETE</command> on

How about: s/is done/is performed/g

+    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.

+        if (resultRelInfo->ri_PartitionCheck &&
+            !ExecPartitionCheck(resultRelInfo, slot, estate))
+        {

How about a one-line comment what this block of code does?

-         * 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].)

+         * We have already run partition constraints above, so skip them
below.

How about: s/run/checked the/g?

@@ -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.


Overall, I think the patch looks good now.  Thanks again for working on it.

Thanks,
Amit

[1] https://www.python.org/dev/peps/pep-0008/#comments