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

> On 3 January 2018 at 11:42, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>> [...] So it make perfect sense for
>> ExecSetupPartitionTupleRouting() to palloc and return a pointer. Sorry
>> for the noise. Will share the change in an upcoming patch version.
>> Thanks !
>
> ExecSetupPartitionTupleRouting() now returns PartitionTupleRouting *.

Thanks for changing. I've just done almost a complete review of v32.
(v33 came along a bit sooner than I thought).

I've not finished looking at the regression tests yet, but here are a
few things, some may have been changed in v33, I've not looked yet.
Also apologies in advance if anything seems nitpicky.

1. "by INSERT" -> "by an INSERT" in:

    from the original partition followed by <command>INSERT</command> into the

2. "and INSERT" -> "and an INSERT" in:

    a <command>DELETE</command> and <command>INSERT</command>. As far as

3. "due partition-key change" -> "due to the partition-key being changed" in:

 * capture is happening for UPDATEd rows being moved to another partition due
 * partition-key change, then this function is called once when the row is

4. "inserted to another" -> "inserted into another" in:

 * deleted (to capture OLD row), and once when the row is inserted to another

5. "for UPDATE event" -> "for an UPDATE event" (singular), or -> "for
UPDATE events" (plural)

* oldtup and newtup are non-NULL.  But for UPDATE event fired for

I'm unsure if you need singular or plural. It perhaps does not matter.

6. "for row" -> "for a row" in:

* movement, oldtup is NULL when the event is for row being inserted,

Likewise in:

* whereas newtup is NULL when the event is for row being deleted.

7. In the following fragment the code does not do what the comment says:

/*
* If transition tables are the only reason we're here, return. As
* mentioned above, we can also be here during update tuple routing in
* presence of transition tables, in which case this function is called
* separately for oldtup and newtup, so either can be NULL, not both.
*/
if (trigdesc == NULL ||
(event == TRIGGER_EVENT_DELETE && !trigdesc->trig_delete_after_row) ||
(event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) ||
(event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row) ||
(event == TRIGGER_EVENT_UPDATE && ((oldtup == NULL) ^ (newtup == NULL))))
return;

With the comment; "so either can be NULL, not both.", I'd expect a
boolean OR not an XOR.

maybe the comment is better written as:

"so we expect exactly one of them to be non-NULL"

(I know you've been discussing with Robert, so I've not checked v33 to
see if this still exists)

8. I'm struggling to make sense of this:

/*
* Save a tuple conversion map to convert a tuple routed to this
* partition from the parent's type to the partition's.
*/

Maybe it's better to write this as:

/*
* Generate a tuple conversion map to convert tuples of the parent's
* type into the partition's type.
*/

9. insert should be capitalised here and should be prefixed with "an":

/*
* Verify result relation is a valid target for insert operation. Even
* for updates, we are doing this for tuple-routing, so again, we need
* to check the validity for insert operation.
*/
CheckValidResultRel(leaf_part_rri, CMD_INSERT);

Maybe it's better to write:

/*
* Verify result relation is a valid target for an INSERT.  An UPDATE of
* a partition-key becomes a DELETE/INSERT operation, so this check is
* still required when the operation is CMD_UPDATE.
*/

10. The following code would be more clear if you replaced
mtstate->mt_transition_capture with transition_capture.

if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture
&& mtstate->mt_transition_capture->tcs_update_new_table)
{
ExecARUpdateTriggers(estate, resultRelInfo, NULL,
NULL,
tuple,
NULL,
mtstate->mt_transition_capture);

/*
* Now that we have already captured NEW TABLE row, any AR INSERT
* trigger should not again capture it below. Arrange for the same.
*/
transition_capture = NULL;
}

You are, after all, doing:

transition_capture = mtstate->mt_transition_capture;

at the top of the function. There are a few other places you're also
accessing mtstate->mt_transition_capture.

11. Should tuple_deleted and process_returning be camelCase like the
other params?:

static TupleTableSlot *
ExecDelete(ModifyTableState *mtstate,
   ItemPointer tupleid,
   HeapTuple oldtuple,
   TupleTableSlot *planSlot,
   EPQState *epqstate,
   EState *estate,
   bool *tuple_deleted,
   bool process_returning,
   bool canSetTag)

12. The following comment talks about "target table descriptor", which
I think is a good term. In several other places, you mention "root",
which I take it to mean "target table".

 * This map array is required for two purposes :
 * 1. For update-tuple-routing. We need to convert the tuple from the subplan
 * result rel to the root partitioned table descriptor.
 * 2. For capturing transition tuples when the target table is a partitioned
 * table. For updates, we need to convert the tuple from subplan result rel to
 * target table descriptor, and for inserts, we need to convert the inserted
 * tuple from leaf partition to the target table descriptor.

I'd personally rather we always talked about "target" rather than
"root". I understand there's probably many places in the code
where we talk about the target table as "root", but I really think we
need to fix that, so I'd rather not see the problem get any worse
before it gets better.

The comment block might also look better if you tab indent after the
1. and 2. then on each line below it.
Also the space before the ':' is not correct.

13. Does the following code really need to palloc0 rather than just palloc?

/*
* Build array of conversion maps from each child's TupleDesc to the
* one used in the tuplestore.  The map pointers may be NULL when no
* conversion is necessary, which is hopefully a common case for
* partitions.
*/
mtstate->mt_childparent_tupconv_maps = (TupleConversionMap **)
palloc0(sizeof(TupleConversionMap *) * numResultRelInfos);

I don't see any case in the initialization of the array where any of
the elements are not assigned a value, so I think palloc() is fine.

14. I don't really like the way tupconv_map_for_subplan() works. It
would be nice to have two separate functions for this, but looking a
bit more at it, it seems the caller won't just need to always call
exactly one of those functions. I don't have any ideas to improve it,
so this is just a note.

15. I still don't really like the way ExecInitModifyTable() sets and
unsets update_tuple_routing_needed. I know we talked about this
before, but couldn't you just change:

if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_update_before_row &&
operation == CMD_UPDATE)
update_tuple_routing_needed = true;

To:

if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_update_before_row &&
node->partitioned_rels != NIL &&
operation == CMD_UPDATE)
update_tuple_routing_needed = true;

and get rid of:

/*
* If it's not a partitioned table after all, UPDATE tuple routing should
* not be attempted.
*/
if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
update_tuple_routing_needed = false;

looking at inheritance_planner(), partitioned_rels is only set to a
non-NIL value if parent_rte->relkind == RELKIND_PARTITIONED_TABLE.

16. "named" -> "target" in:

 * 'partKeyUpdated' is true if any partitioning columns are being updated,
 * either from the named relation or a descendent partitioned table.

I guess we're calling this one of; root, named, target :-(

17. You still have the following comment in ModifyTableState but
you've moved all those fields out to PartitionTupleRouting:

/* Tuple-routing support info */

18. Should the following not be just called partKeyUpdate (without the 'd')?

bool partKeyUpdated; /* some part key in hierarchy updated */

This occurs in the planner were the part key is certainly being updated.

19. In pathnode.h you've named a parameter partColsUpdated, but the
function in the .c file calls it partKeyUpdated.

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.

Overall, the patch appears to look quite good. Good to see the various
cleanups going in like the new PartitionTupleRouting struct.

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