Re: UPDATE of partition key

On Wed, Oct 25, 2017 at 11:40 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> Below I have addressed the remaining review comments :

The changes to trigger.c still make me super-nervous. Hey THOMAS
MUNRO, any chance you could review that part?

+ /* The caller must have already locked all the partitioned tables. */
+ root_rel = heap_open(root_relid, NoLock);
+ *all_part_cols = NULL;
+ foreach(lc, partitioned_rels)
+ {
+ Index rti = lfirst_int(lc);
+ Oid relid = getrelid(rti, rtables);
+ Relation part_rel = heap_open(relid, NoLock);
+
+ pull_child_partition_columns(part_rel, root_rel, all_part_cols);
+ heap_close(part_rel, NoLock);

I don't like the fact that we're opening and closing the relation here
just to get information on the partitioning columns. I think it would
be better to do this someplace that already has the relation open and
store the details in the RelOptInfo. set_relation_partition_info()
looks like the right spot.

+void
+pull_child_partition_columns(Relation rel,
+ Relation parent,
+ Bitmapset **partcols)

This code has a lot in common with is_partition_attr(). I'm not sure
it's worth trying to unify them, but it could be done.

+ * 'num_update_rri' : number of UPDATE per-subplan result rels. For INSERT,

Instead of " : ", you could just write "is the".

+ * For Updates, if the leaf partition is already present in the
+ * per-subplan result rels, we re-use that rather than
initialize a
+ * new result rel. The per-subplan resultrels and the
resultrels of
+ * the leaf partitions are both in the same canonical
order. So while

It would be good to explain the reason. Also, Updates shouldn't be
capitalized here.

+ Assert(cur_update_rri <= update_rri +
num_update_rri - 1);

Maybe just cur_update_rri < update_rri + num_update_rri, or even
current_update_rri - update_rri < num_update_rri.

Also, +1 for Amit Langote's idea of trying to merge
mt_perleaf_childparent_maps with mt_persubplan_childparent_maps.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company