From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Andrey V. Lepikhov <a.lepikhov@postgrespro.ru>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: POC: postgres_fdw insert batching
Date: Thu, 14 Jan 2021 13:57:44 +0100
Message-ID: <53187d73-05ad-90c8-b404-db5f250d3bb0@enterprisedb.com> (raw)
In-Reply-To: <CA+HiwqEbnhwVJMsukTP-S9Kv1ynC7Da3yuqSPZC0Y7oWWOwoHQ@mail.gmail.com>
References: <20200628151002.7x5laxwpgvkyiu3q@development>
<OSBPR01MB298268F1A1BACFCE61740CC3FEE20@OSBPR01MB2982.jpnprd01.prod.outlook.com>
<d9a17f23-81cc-fca1-56e6-602bd2ae06b4@enterprisedb.com>
<TYAPR01MB2990A726F963C10DF3B98790FEE00@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<71113ab1-ed31-3dbc-ce07-e081e17473b6@enterprisedb.com>
<TYAPR01MB2990004041DD64597E52759DFEFC0@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<43b5d643-da69-70a5-16d2-d04d0851b723@enterprisedb.com>
<TYAPR01MB2990FB16E86A4E51BB3E700AFEFB0@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<0ab26876-9e89-ef24-094d-35b991672309@enterprisedb.com>
<TYAPR01MB2990EF5668C41AC71EC3EDDDFEFA0@TYAPR01MB2990.jpnprd01.prod.outlook.com>
<9808f343-410e-4441-9a10-42e14a1c6436@enterprisedb.com>
<CA+HiwqFwwRj4_jn75x+0kGFAxEZwJ8UJqr1KAhqyyLjvsY3zCA@mail.gmail.com>
<131a695b-9010-6751-9ea7-3dabe3555f8c@enterprisedb.com>
<d682fcf2-41db-8e8c-1f4c-a1cb4e481bab@enterprisedb.com>
<CA+HiwqEbnhwVJMsukTP-S9Kv1ynC7Da3yuqSPZC0Y7oWWOwoHQ@mail.gmail.com>
On 1/14/21 9:58 AM, Amit Langote wrote:
> Hi,
>
> On Thu, Jan 14, 2021 at 2:41 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> On 1/13/21 3:43 PM, Tomas Vondra wrote:
>>> Thanks for the report. Yeah, I think there's a missing check in
>>> ExecInsert. Adding
>>>
>>> (!resultRelInfo->ri_TrigDesc->trig_insert_after_row)
>>>
>>> solves this. But now I'm wondering if this is the wrong place to make
>>> this decision. I mean, why should we make the decision here, when the
>>> decision whether to have a RETURNING clause is made in postgres_fdw in
>>> deparseReturningList? We don't really know what the other FDWs will do,
>>> for example.
>>>
>>> So I think we should just move all of this into GetModifyBatchSize. We
>>> can start with ri_BatchSize = 0. And then do
>>>
>>> if (resultRelInfo->ri_BatchSize == 0)
>>> resultRelInfo->ri_BatchSize =
>>> resultRelInfo->ri_FdwRoutine->GetModifyBatchSize(resultRelInfo);
>>>
>>> if (resultRelInfo->ri_BatchSize > 1)
>>> {
>>> ... do batching ...
>>> }
>>>
>>> The GetModifyBatchSize would always return value > 0, so either 1 (no
>>> batching) or >1 (batching).
>>>
>>
>> FWIW the attached v8 patch does this - most of the conditions are moved
>> to the GetModifyBatchSize() callback.
>
> Thanks. A few comments:
>
> * I agree with leaving it up to an FDW to look at the properties of
> the table and of the operation being performed to decide whether or
> not to use batching, although maybe BeginForeignModify() is a better
> place for putting that logic instead of GetModifyBatchSize()? So, in
> create_foreign_modify(), instead of PgFdwModifyState.batch_size simply
> being set to match the table's or the server's value for the
> batch_size option, make it also consider the things that prevent
> batching and set the execution state's batch_size based on that.
> GetModifyBatchSize() simply returns that value.
>
> * Regarding the timing of calling GetModifyBatchSize() to set
> ri_BatchSize, I wonder if it wouldn't be better to call it just once,
> say from ExecInitModifyTable(), right after BeginForeignModify()
> returns? I don't quite understand why it is being called from
> ExecInsert(). Can the batch size change once the execution starts?
>
But it should be called just once. The idea is that initially we have
batch_size=0, and the fist call returns value that is >= 1. So we never
call it again. But maybe it could be called from BeginForeignModify, in
which case we'd not need this logic with first setting it to 0 etc.
> * Lastly, how about calling it GetForeignModifyBatchSize() to be
> consistent with other nearby callbacks?
>
Yeah, good point.
>> I've removed the check for the
>> BatchInsert callback, though - the FDW knows whether it supports that,
>> and it seems a bit pointless at the moment as there are no other batch
>> callbacks. Maybe we should add an Assert somewhere, though?
>
> Hmm, not checking whether BatchInsert() exists may not be good idea,
> because if an FDW's GetModifyBatchSize() returns a value > 1 but
> there's no BatchInsert() function to call, ExecBatchInsert() would
> trip. I don't see the newly added documentation telling FDW authors
> to either define both or none.
>
Hmm. The BatchInsert check seemed somewhat unnecessary to me, but OTOH
it can't hurt, I guess. I'll ad it back.
> Regarding how this plays with partitions, I don't think we need
> ExecGetTouchedPartitions(), because you can get the routed-to
> partitions using es_tuple_routing_result_relations. Also, perhaps
I'm not very familiar with es_tuple_routing_result_relations, but that
doesn't seem to work. I've replaced the flushing code at the end of
ExecModifyTable with a loop over es_tuple_routing_result_relations, but
then some of the rows are missing (i.e. not flushed).
> it's a good idea to put the "finishing" ExecBatchInsert() calls into a
> function ExecFinishBatchInsert(). Maybe the logic to choose the
> relations to perform the finishing calls on will get complicated in
> the future as batching is added for updates/deletes too and it seems
> better to encapsulate that in the separate function than have it out
> in the open in ExecModifyTable().
>
IMO that'd be an over-engineering at this point. We don't need such
separate function yet, so why complicate the API? If we need it in the
future, we can add it.
> (Sorry about being so late reviewing this.)
thanks
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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: tomas.vondra@enterprisedb.com, amitlangote09@gmail.com, tsunakawa.takay@fujitsu.com, tomas.vondra@2ndquadrant.com, a.lepikhov@postgrespro.ru
Subject: Re: POC: postgres_fdw insert batching
In-Reply-To: <53187d73-05ad-90c8-b404-db5f250d3bb0@enterprisedb.com>
* 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