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

On Wed, Oct 25, 2017 at 11:40 AM, Amit Khandekar <amitdkhan.pg@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers