On 2018/04/06 8:33, Alvaro Herrera wrote:
>> @@ -1717,8 +1691,8 @@ expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,
>> * parentrte already has the root partrel's updatedCols translated to match
>> * the attribute ordering of parentrel.
>> */
>> - if (!*part_cols_updated)
>> - *part_cols_updated =
>> + if (!root->partColsUpdated)
>> + root->partColsUpdated =
>> has_partition_attrs(parentrel, parentrte->updatedCols, NULL);
>
> Hmm, surely this should be |= to avoid resetting a value set in a
> previous call to this function?
It won't be, no? We set it only if it hasn't been already. Note that
there is one PlannerInfo per sub-query, so we determine this information
independently for each sub-query.
> In the previous coding it wasn't
> necessary because it was a local variable ... (though, isn't it a bit
> odd to have this in PlannerInfo? seems like it should be in
> resultRelInfo, but then you already have it there so I suppose this one
> does *more*)
Hmm, you'd think that we can figure this out in the executor itself, and
hence don't to have this in PlannerInfo or in ModifyTable. But IIRC,
during the discussion of the update tuple routing patch, it became clear
that it's best do that here, given the way things are now wrt the timing
of partition/inheritance tree expansion. An update query may modify the
partition key of a table at any arbitrary level and we have to look at all
the tables in the partition tree in this planning phase anyway, so it's
also the best time to see it if the query's modifiedCols overlaps with the
partition key of some table in the tree. Once we've found that it does
for some table (most likely the root), we're done, that is, we know we got
some "partColsUpdated".
I realized that I had gotten rid of has_default_part from RelOptInfo but
hadn't deleted a comment about it; attached patch to fix that.
Thanks,
Amit