Re: Declarative partitioning - another take

Thanks for the review.

On 2017/04/29 1:25, Robert Haas wrote:
> On Fri, Apr 28, 2017 at 2:13 AM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>> By the way, code changes I made in the attached are such that a subsequent
>>>> patch could implement firing statement-level triggers of all the tables in
>>>> a partition hierarchy, which it seems we don't want to do. Should then
>>>> the code be changed to not create ResultRelInfos of all the tables but
>>>> only the root table (the one mentioned in the command)? You will see that
>>>> the patch adds fields named es_nonleaf_result_relations and
>>>> es_num_nonleaf_result_relations, whereas just es_root_result_relation
>>>> would perhaps do, for example.
>>>
>>> It seems better not to create any ResultRelInfos that we don't
>>> actually need, so +1 for such a revision to the patch.
>>
>> OK, done. It took a bit more work than I thought.
>
> So, this seems weird, because rootResultRelIndex is initialized even
> when splan->partitioned_rels == NIL, but isn't actually valid in that
> case. ExecInitModifyTable seems to think it's always valid, though.

OK, rootResultRelIndex is now set to a value >= 0 only when the node
modifies a partitioned table. And then ExecInitModifyTable() checks if
the index is valid before initializing the root table info.

> I think the way that you've refactored fireBSTriggers and
> fireASTriggers is a bit confusing. Instead of splitting out a
> separate function, how about just having the existing function begin
> with if (node->rootResultRelInfo) resultRelInfo =
> node->rootResultRelInfo; else resultRelInfo = node->resultRelInfo; ?
> I think the way you've coded it is a holdover from the earlier design
> where you were going to call it multiple times, but now that's not
> needed.

OK, done that way.

> Looks OK, otherwise.

Attached updated patch.

Thanks,
Amit