Re: [HACKERS] UPDATE of partition key - Mailing list pgsql-hackers

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