Re: [HACKERS] UPDATE of partition key

Below I have addressed the remaining review comments :

On 16 October 2017 at 08:28, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
>
> In ExecInitModifyTable(), can we try to minimize the number of places
> where update_tuple_routing_needed is being set.  Currently, it's being set
> in 3 places:

I think the way it's done seems ok. For each resultRelInfo,
update_tuple_routing_needed is updated in case that resultRel has
partition cols changed. And at that point, we don't have rel opened,
so we can't check if that rel is partitioned. So another check is
required outside of the loop.

>
> +         * qual for each partition. Note that, if there are SubPlans in
> there,
> +         * they all end up attached to the one parent Plan node.
>
> The sentence starting with "Note that, " is a bit unclear.
>
> +        Assert(update_tuple_routing_needed ||
> +               (operation == CMD_INSERT &&
> +                list_length(node->withCheckOptionLists) == 1 &&
> +                mtstate->mt_nplans == 1));
>
> The comment I complained about above is perhaps about this Assert.

That is an existing comment. On HEAD, the "parent Plan" refers to
mtstate->mt_plans[0]. Now in the patch, for the parent node in
ExecInitQual(), mtstate->ps is passed rather than mt_plans[0]. So the
parent plan refers to this mtstate node.

BTW, the reason I had changed the parent node to mtstate->ps is :
Other places in that code use mtstate->ps while initializing
expressions :

/*
* Build a projection for each result rel.
*/
   resultRelInfo->ri_projectReturning =
      ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
                              resultRelInfo->ri_RelationDesc->rd_att);

...........

/* build DO UPDATE WHERE clause expression */
if (node->onConflictWhere)
{
   ExprState  *qualexpr;

   qualexpr = ExecInitQual((List *) node->onConflictWhere,
    &mtstate->ps);
....
}

I think wherever we initialize expressions belonging to a plan, we
should use that plan as the parent. WithCheckOptions are fields of
ModifyTableState.

>
> -            List       *mapped_wcoList;
> +            List       *mappedWco;
>
> Not sure why this rename.  After this rename, it's now inconsistent with
> the code above which handles non-partitioned case, which still calls it
> wcoList.  Maybe, because you introduced firstWco and then this line:
>
> +        firstWco = linitial(node->withCheckOptionLists);
>
> but note that each member of node->withCheckOptionLists is also a list, so
> the original naming.   Also, further below, you're assigning mappedWco to
> a List * field.
>
> +            resultRelInfo->ri_WithCheckOptions = mappedWco;

Done. Reverted mappedWco to mapped_wcoList. And firstWco to first_wcoList.

>
>
> Comments on the optimizer changes:
>
> +get_all_partition_cols(List *rtables,
>
> Did you mean rtable?

I did mean rtables. It's a list of rtables.

>
>
> +        get_all_partition_cols(root->parse->rtable, top_parentRTindex,
> +                             partitioned_rels, &all_part_cols);
>
> Two more spaces needed on the 2nd line.

Done.

>
>
>
> +void
> +pull_child_partition_columns(Bitmapset **bitmapset,
> +                            Relation rel,
> +                            Relation parent)
>
> Nitpick: don't we normally list the output argument(s) at the end?

Agreed. Done.

> Also, "bitmapset" could be renamed to something that conveys what it contains?

Renamed it to partcols

>
> +       if (partattno != 0)
> +           child_keycols =
> +               bms_add_member(child_keycols,
> +                              partattno -
> FirstLowInvalidHeapAttributeNumber);
> +   }
> +   foreach(lc, partexprs)
> +   {
>
> Elsewhere (in quite a few places), we don't iterate over partexprs
> separately like this, although I'm not saying it is bad, just different
> from other places.

I think you are suggesting we do it like how it's done in
is_partition_attr(). Can you please let me know other places we do
this same way ? I couldn't find.

>
> get_all_partition_cols() seems to go over the rtable as many times as
> there are partitioned tables in the tree.  Is there a way to do this work
> somewhere else?  Maybe when the partitioned_rels list is built in the
> first place.  But that would require us to make changes to extract
> partition columns in some place (prepunion.c) where it's hard to justify
> why it's being done there at all.

See below ...

>
>
> + * If all_part_cols_p is non-NULL, *all_part_cols_p is set to a bitmapset
> + * of all partitioning columns used by the partitioned table or any
> + * descendent.
> + *
>
> Dead comment?

Removed.

> Aha, so here's where all_part_cols was being set before...

Yes, and we used to have PartitionedChildRelInfo.all_part_cols field
for that. We used to populate that while traversing through the
partition tree in expand_inherited_rtentry(). I agreed with Dilip's
opinion that this would unnecessarily add up some processing even when
the query is not a DML. And also, we don't have to have
PartitionedChildRelInfo.all_part_cols. For the earlier implementation,
check v18 patch or earlier versions.

>
> +    TupleTableSlot *mt_rootpartition_tuple_slot;
>
> I guess I was complaining about this field where you call root a
> partition.  Maybe, mt_root_tuple_slot would suffice.

Done.

Attached v22 patch.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

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