Re: Optimization for updating foreign tables in Postgres FDW

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Optimization for updating foreign tables in Postgres FDW
Date: 2016-03-09 08:30:44
Message-ID: 56DFDF34.8040804@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2016/03/08 2:35, Robert Haas wrote:
> On Mon, Mar 7, 2016 at 7:53 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Another option to avoid such a hazard would be to remove the two changes
>> from ExecInitModifyTable and create ExecAuxRowMarks and junk filters even in
>> the pushdown case. I made the changes because we won't use ExecAuxRowMarks
>> in that case since we don't need to do EvalPlanQual rechecks and because we
>> won't use junk filters in that case since we do UPDATE/DELETE in the
>> subplan. But the creating cost is enough small, so simply removing the
>> changes seems like a good idea.

> Sure, that works.

OK, I removed the changes.

>>> This issue crops up elsewhere as well. The changes to
>>> ExecModifyTable() have the same problem -- in that case, it might be
>>> wise to move the code that's going to have to be indented yet another
>>> level into a separate function. That code also says this:
>>>
>>> + /* No need to provide scan tuple to
>>> ExecProcessReturning. */
>>> + slot = ExecProcessReturning(resultRelInfo,
>>> NULL, planSlot);
>>>
>>> ...but, uh, why not? The comment says what the code does, but what it
>>> should do is explain why it does it.
>>
>> As documented in IterateDMLPushdown in fdwhandler.sgml, the reason for that
>> is that in the pushdown case it's the IterateDMLPushdown's responsiblity to
>> get actually inserted/updated/deleted tuples and make those tuples available
>> to the ExecProcessReturning. I'll add comments.

> Comments are good things to have. :-)

Yeah, I added comments.

>>> On a broader level, I'm not very happy with the naming this patch
>>> uses. Here's an example:
>>>
>>> + <para>
>>> + If an FDW supports optimizing foreign table updates, it still needs
>>> to
>>> + provide <function>PlanDMLPushdown</>, <function>BeginDMLPushdown</>,
>>> + <function>IterateDMLPushdown</> and <function>EndDMLPushdown</>
>>> + described below.
>>> + </para>
>>>
>>> "Optimizing foreign table updates" is both inaccurate (since it
>>> doesn't only optimize updates) and so vague as to be meaningless
>>> unless you already know what it means. The actual patch uses
>>> terminology like "fdwPushdowns" which is just as bad. We might push a
>>> lot of things to the foreign side -- sorts, joins, aggregates, limits
>>> -- and this is just one of them. Worse, "pushdown" is itself
>>> something of a term of art - will people who haven't been following
>>> all of the mammoth, multi-hundred-email threads on this topic know
>>> what that means? I think we need some better terminology here.
>>>
>>> The best thing that I can come up with offhand is "bulk modify". So
>>> we'd have PlanBulkModify, BeginBulkModify, IterateBulkModify,
>>> EndBulkModify, ExplainBulkModify. Other suggestions welcome. The
>>> ResultRelInfo flag could be ri_usesFDWBulkModify.
>>
>> I'm not sure that "bulk modify" is best. Yeah, this would improve the
>> performance especially in the bulk-modification case, but would improve the
>> performance even in the case where an UPDATE/DELETE modifies just a single
>> row. Let me explain using an example. Without the patch, we have the
>> following plan for an UPDATE on a foreign table that updates a single row:
>>
>> postgres=# explain verbose update foo set a = a + 1 where a = 1;
>> QUERY PLAN
>> ----------------------------------------------------------------------------------
>> Update on public.foo (cost=100.00..101.05 rows=1 width=14)
>> Remote SQL: UPDATE public.foo SET a = $2 WHERE ctid = $1
>> -> Foreign Scan on public.foo (cost=100.00..101.05 rows=1 width=14)
>> Output: (a + 1), b, ctid
>> Remote SQL: SELECT a, b, ctid FROM public.foo WHERE ((a = 1)) FOR
>> UPDATE
>> (5 rows)
>>
>> The plan requires two queries, SELECT and UPDATE, to do the update.
>> (Actually, the plan have additional overheads in creating a cursor for the
>> SELECT and establishing a prepared statement for the UPDATE.) But with the
>> patch, we have:
>>
>> postgres=# explain verbose update foo set a = a + 1 where a = 1;
>> QUERY PLAN
>> ---------------------------------------------------------------------------
>> Update on public.foo (cost=100.00..101.05 rows=1 width=14)
>> -> Foreign Update on public.foo (cost=100.00..101.05 rows=1 width=14)
>> Remote SQL: UPDATE public.foo SET a = (a + 1) WHERE ((a = 1))
>> (3 rows)
>>
>> The optimized plan requires just a single UPDATE query to do that! So, even
>> in the single-row-modification case the patch could improve the performance.
>>
>> How about "Direct Modify"; PlanDirectModify, BeginDirectModify,
>> IterateDirectModify, EndDirectModify, ExplainDirectModify, and
>> ri_usesFDWDirectModify.

> Works for me!

Great! I changed the naming. I also updated docs as proposed by you in
a previous email, and rebased the patch to the latest HEAD. Please find
attached an updated version of the patch.

Best regards,
Etsuro Fujita

Attachment Content-Type Size
fdw-dml-pushdown-v10.patch binary/octet-stream 94.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Joel Jacobson 2016-03-09 08:34:19 Re: Disabling an index temporarily
Previous Message Kyotaro HORIGUCHI 2016-03-09 08:29:49 Re: PATCH: index-only scans with partial indexes