From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: partition routing layering in nodeModifyTable.c
Date: Tue, 13 Oct 2020 13:12:59 +0300
Message-ID: <ddae2239-4932-fb08-e501-9768cee871fc@iki.fi> (raw)
In-Reply-To: <CA+HiwqENbJMzoD1VAoNWFLcnam0zJS34Vy0o5AuhW-UVq1LV6A@mail.gmail.com>
References: <20190718010911.l6xcdv6birtxiei4@alap3.anarazel.de>
<CA+HiwqGXSE5772PqYTtQa_mSf-h5xCz4vf-Aq4wLuTQtLCe6ZA@mail.gmail.com>
<CA+HiwqFM7fmVHbTt-AxQwk_6pjEzbX4iOEu8pGV+=9+7Kvc0Fw@mail.gmail.com>
<30269.1583091776@sss.pgh.pa.us>
<CA+HiwqH1XMX9S7eUaOefz2BKJHaYnHnKyP2McG5MORcX=YHNiA@mail.gmail.com>
<424F4298-7826-40DC-B12B-37EFE60EE243@yesql.se>
<CA+HiwqFaXvPRpYjuw_x5jFvL7XtAH2kXCRaWAg3N+5ppfyr4fw@mail.gmail.com>
<73fe6ce6-d2a1-bd4b-17cf-9deba4e91467@iki.fi>
<CA+HiwqGAVa9J9hNLH7tVCs13UqJMP-9RU_T5uAWEAfsg8VEXGQ@mail.gmail.com>
<19c23dd9-89ce-75a3-9105-5fc05a46f94a@iki.fi>
<CA+HiwqHtriD3HquH5=uT5caHd4jiEoxeb5JD2JSZRm8gDXsRxw@mail.gmail.com>
<CA+HiwqGEmiib8FLiHMhKB+CH5dRgHSLc5N5wnvc4kym+ZYpQEQ@mail.gmail.com>
<9bfb949e-21b6-e0f7-c9c9-58710d11e7b1@iki.fi>
<CA+HiwqHX4gt6b7e=xM3aG837Hfv=q5hEkQ=E1BSi54ob=_cQaA@mail.gmail.com>
<3603af69-eb52-4891-5d50-669cb03090a6@iki.fi>
<CA+HiwqENbJMzoD1VAoNWFLcnam0zJS34Vy0o5AuhW-UVq1LV6A@mail.gmail.com>
On 13/10/2020 07:32, Amit Langote wrote:
> On Tue, Oct 13, 2020 at 1:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> It occurred to me that if we do that (initialize the array lazily),
>> there's very little need for the PlannedStmt->resultRelations list
>> anymore. It's only used in ExecRelationIsTargetRelation(), but if we
>> assume that ExecRelationIsTargetRelation() is only called after InitPlan
>> has initialized the result relation for the relation, it can easily
>> check es_result_relations instead. I think that's a safe assumption.
>> ExecRelationIsTargetRelation() is only used in FDWs, and I believe the
>> FDWs initialization routine can only be called after ExecInitModifyTable
>> has been called on the relation.
>>
>> The PlannedStmt->rootResultRelations field is even more useless.
>
> I am very much tempted to remove those fields from PlannedStmt,
> although I am concerned that the following now assumes that *all*
> result relations are initialized in the executor initialization phase:
>
> bool
> ExecRelationIsTargetRelation(EState *estate, Index scanrelid)
> {
> if (!estate->es_result_relations)
> return false;
>
> return estate->es_result_relations[scanrelid - 1] != NULL;
> }
>
> In the other thread [1], I am proposing that we initialize result
> relations lazily, but the above will be a blocker to that.
Ok, I'll leave it alone then. But I'll still merge resultRelations and
rootResultRelations into one list. I don't see any point in keeping them
separate.
I'm tempted to remove ExecRelationIsTargetRelation() altogether, but
keeping the resultRelations list isn't really a big deal, so I'll leave
that for another discussion.
>>> Actually, maybe we don't need to be so paranoid about setting up
>>> es_result_relations in worker.c, because none of the downstream
>>> functionality invoked seems to rely on it, that is, no need to call
>>> ExecInitResultRelationsArray() and ExecInitResultRelation().
>>> ExecSimpleRelation* and downstream functionality assume a
>>> single-relation operation and the ResultRelInfo is explicitly passed.
>>
>> Hmm, yeah, I like that. Similarly in ExecuteTruncateGuts(), there isn't
>> actually any need to put the ResultRelInfos in the es_result_relations
>> array.
>>
>> Putting all this together, I ended up with the attached. It doesn't
>> include the subsequent commits in this patch set yet, for removal of
>> es_result_relation_info et al.
>
> Thanks.
>
> + * We put the ResultRelInfos in the es_opened_result_relations list, even
> + * though we don't have a range table and don't populate the
> + * es_result_relations array. That's a big bogus, but it's enough to make
> + * ExecGetTriggerResultRel() find them.
> */
> estate = CreateExecutorState();
> resultRelInfos = (ResultRelInfo *)
> palloc(list_length(rels) * sizeof(ResultRelInfo));
> resultRelInfo = resultRelInfos;
> + estate->es_result_relations = (ResultRelInfo **)
> + palloc(list_length(rels) * sizeof(ResultRelInfo *));
>
> Maybe don't allocate es_result_relations here?
Fixed.
> Anyway, other than my concern about ExecRelationIsTargetRelation()
> mentioned above, I think the patch looks good.
Ok, committed. I'll continue to look at the rest of the patches in this
patch series now.
- Heikki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, amitlangote09@gmail.com, daniel@yesql.se, etsuro.fujita@gmail.com, andres@anarazel.de
Subject: Re: partition routing layering in nodeModifyTable.c
In-Reply-To: <ddae2239-4932-fb08-e501-9768cee871fc@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox