Re: partition routing layering in nodeModifyTable.c

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Daniel Gustafsson <daniel(at)yesql(dot)se>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, amitdkhan(dot)pg(at)gmail(dot)com
Subject: Re: partition routing layering in nodeModifyTable.c
Date: 2020-10-07 12:07:20
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 07/10/2020 12:50, Amit Langote wrote:
> On Tue, Oct 6, 2020 at 12:45 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> It would be better to set it in make_modifytable(), just
>> after calling PlanDirectModify().
> Actually, that's how it was done in earlier iterations but I think I
> decided to move that into the FDW's functions due to some concern of
> one of the other patches that depended on this patch. Maybe it makes
> sense to bring that back into make_modifytable() and worry about the
> other patch later.

On second thoughts, I take back my earlier comment. Setting it in
make_modifytable() relies on the assumption that the subplan is a single
ForeignScan node, on the target relation. The documentation for
PlanDirectModify says:

> To execute the direct modification on the remote server, this
> function must rewrite the target subplan with a ForeignScan plan node
> that executes the direct modification on the remote server.
So I guess that assumption is safe. But I'd like to have some wiggle
room here. Wouldn't it be OK to have a Result node on top of the
ForeignScan, for example? If it really must be a simple ForeignScan
node, the PlanDirectModify API seems pretty strange.

I'm not entirely sure what I would like to do with this now. I could
live with either version, but I'm not totally happy with either. (I like
your suggestion below)

Looking at this block in postgresBeginDirectModify:

> /*
> * Identify which user to do the remote access as. This should match what
> * ExecCheckRTEPerms() does.
> */
> Assert(fsplan->resultRelIndex >= 0);
> dmstate->resultRelIndex = fsplan->resultRelIndex;
> rtindex = list_nth_int(resultRelations, fsplan->resultRelIndex);
> rte = exec_rt_fetch(rtindex, estate);
> userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();

That's a complicated way of finding out the target table's RTI. We
should probably store the result RTI in the ForeignScan in the first place.

>> Another idea is to merge "resultRelIndex" and a "range table index" into
>> one value. Range table entries that are updated would have a
>> ResultRelInfo, others would not. I'm not sure if that would end up being
>> cleaner or messier than what we have now, but might be worth trying.
> I have thought about something like this before. An idea I had is to
> make es_result_relations array indexable by plain RT indexes, then we
> don't need to maintain separate indexes that we do today for result
> relations.

That sounds like a good idea. es_result_relations is currently an array
of ResultRelInfos, so that would leave a lot of unfilled structs in the
array. But in on of your other threads, you proposed turning
es_result_relations into an array of pointers anyway

- Heikki

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey Borodin 2020-10-07 12:27:21 Re: Yet another fast GiST build
Previous Message 2020-10-07 11:52:15 RE: libpq debug log