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