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(dot)pg(at)gmail(dot)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(at)gmail(dot)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