Re: [HACKERS] UPDATE of partition key

On 4 January 2018 at 02:52, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> I'll try to look at the tests tomorrow and also do some testing. So
> far I've only read the code and the docs.

There are a few more things I noticed on another pass I made today:

20. "carried" -> "carried out the"

+ would have identified the newly updated row and carried
+ <command>UPDATE</command>/<command>DELETE</command> on this new row

21. Extra new line

+ <xref linkend="ddl-partitioning-declarative-limitations">.
+
</para>

22. In copy.c CopyFrom() you have the following code:

/*
* We might need to convert from the parent rowtype to the
* partition rowtype.
*/
map = proute->partition_tupconv_maps[leaf_part_index];
if (map)
{
Relation partrel = resultRelInfo->ri_RelationDesc;

tuple = do_convert_tuple(tuple, map);

/*
* We must use the partition's tuple descriptor from this
* point on. Use a dedicated slot from this point on until
* we're finished dealing with the partition.
*/
slot = proute->partition_tuple_slot;
Assert(slot != NULL);
ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
ExecStoreTuple(tuple, slot, InvalidBuffer, true);
}

Should this use ConvertPartitionTupleSlot() instead?

23. Why write;

last_resultRelInfo = mtstate->resultRelInfo + mtstate->mt_nplans;

when you can write;

last_resultRelInfo = mtstate->resultRelInfo[mtstate->mt_nplans];?

24. In ExecCleanupTupleRouting(), do you think that you could just
have a special case loop for (mtstate && mtstate->operation ==
CMD_UPDATE)?

/*
* If this result rel is one of the UPDATE subplan result rels, let
* ExecEndPlan() close it. For INSERT or COPY, this does not apply
* because leaf partition result rels are always newly allocated.
*/
if (is_update &&
resultRelInfo >= first_resultRelInfo &&
resultRelInfo < last_resultRelInfo)
continue;

Something like:

if (mtstate && mtstate->operation == CMD_UPDATE)
{
ResultRelInfo *first_resultRelInfo = mtstate->resultRelInfo;
ResultRelInfo *last_resultRelInfo =
mtstate->resultRelInfo[mtstate->mt_nplans];

for (i = 0; i < proute->num_partitions; i++)
{
ResultRelInfo *resultRelInfo = proute->partitions[i];

/*
* Leave any resultRelInfos that belong to the UPDATE's subplan
* list. These will be closed during executor shutdown.
*/
if (resultRelInfo >= first_resultRelInfo &&
resultRelInfo < last_resultRelInfo)
continue;

ExecCloseIndices(resultRelInfo);
heap_close(resultRelInfo->ri_RelationDesc, NoLock);
}
}
else
{
for (i = 0; i < proute->num_partitions; i++)
{
ResultRelInfo *resultRelInfo = proute->partitions[i];

ExecCloseIndices(resultRelInfo);
heap_close(resultRelInfo->ri_RelationDesc, NoLock);
}
}

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services