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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
