Re: UPDATE of partition key

Hi Amit,

Thanks for the updated patch.

On 2017/03/23 3:09, Amit Khandekar wrote:
> Attached is v2 patch which implements the above optimization.

Would it be better to have at least some new tests? 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.

@@ -633,6 +634,9 @@ ExecDelete(ItemPointer tupleid,
HeapUpdateFailureData hufd;
TupleTableSlot *slot = NULL;

+ if (already_deleted)
+ *already_deleted = false;
+

concurrently_deleted?

@@ -962,7 +969,7 @@ ExecUpdate(ItemPointer tupleid,
}
else
{
- LockTupleMode lockmode;
+ LockTupleMode lockmode;

Useless hunk.

+ if (!mtstate->mt_partition_dispatch_info)
+ {

The if (pointer == NULL) style is better perhaps.

+ /* root table RT index is at the head of partitioned_rels */
+ if (node->partitioned_rels)
+ {
+ Index root_rti;
+ Oid root_oid;
+
+ root_rti = linitial_int(node->partitioned_rels);
+ root_oid = getrelid(root_rti, estate->es_range_table);
+ root_rel = heap_open(root_oid, NoLock); /* locked by
InitPlan */
+ }
+ else
+ root_rel = mtstate->resultRelInfo->ri_RelationDesc;

Some explanatory comments here might be good, for example, explain in what
situations node->partitioned_rels would not have been set and/or vice versa.

> Now, for
> UPDATE, ExecSetupPartitionTupleRouting() will be called only if row
> movement is needed.
>
> We have to open an extra relation for the root partition, and keep it
> opened and its handle stored in
> mt_partition_dispatch_info[0]->reldesc. So ExecEndModifyTable() closes
> this if it is different from node->resultRelInfo->ri_RelationDesc. If
> it is same as node->resultRelInfo, it should not be closed because it
> gets closed as part of ExecEndPlan().

I guess you're referring to the following hunk. Some comments:

@@ -2154,10 +2221,19 @@ ExecEndModifyTable(ModifyTableState *node)
* Close all the partitioned tables, leaf partitions, and their indices
*
* Remember node->mt_partition_dispatch_info[0] corresponds to the root
- * partitioned table, which we must not try to close, because it is the
- * main target table of the query that will be closed by ExecEndPlan().
- * Also, tupslot is NULL for the root partitioned table.
+ * partitioned table, which should not be closed if it is the main target
+ * table of the query, which will be closed by ExecEndPlan().

The last part could be written as: because it will be closed by ExecEndPlan().

Also, tupslot
+ * is NULL for the root partitioned table.
*/
+ if (node->mt_num_dispatch > 0)
+ {
+ Relation root_partition;

root_relation?

+
+ root_partition = node->mt_partition_dispatch_info[0]->reldesc;
+ if (root_partition != node->resultRelInfo->ri_RelationDesc)
+ heap_close(root_partition, NoLock);
+ }

It might be a good idea to Assert inside the if block above that
node->operation != CMD_INSERT. Perhaps, also reflect that in the comment
above so that it's clearer.

I will set the patch to Waiting on Author.

Thanks,
Amit