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

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
+      if the new value of the row fails to satisfy the implicit partition

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

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

@@ -2159,6 +2289,7 @@ ExecEndModifyTable(ModifyTableState *node)
         heap_close(pd->reldesc, NoLock);
     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.


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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to