Subject: Re: POC: postgres_fdw insert batching
To: Amit Langote <amitlangote09@gmail.com>,
 "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>
Cc: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <20200628151002.7x5laxwpgvkyiu3q@development>
 <CA+HiwqEjV00Re-xXaiy3GKntj-B-M8Q1LOt=tGx62ZWzv+y0uw@mail.gmail.com>
 <70d5f38f-1271-234e-c154-3c5c81b5bcf2@enterprisedb.com>
 <CA+HiwqFGENfuyC7w7TD9L8wdJk3+7XwD8Z24Kp7OjJg4+mTjqQ@mail.gmail.com>
 <TYAPR01MB2990BDB626DB2630E6CEA78AFEA70@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <CA+HiwqESrO5BKHtWZZTSbK26OWk-Uhx6DYtdVEkpdaoZ=uW5BA@mail.gmail.com>
 <OSBPR01MB29820BAA17AD3A79A49935EFFEA40@OSBPR01MB2982.jpnprd01.prod.outlook.com>
 <16fcfd4c-2d5e-67f7-fa69-9b5d119bac52@enterprisedb.com>
 <TYAPR01MB299026DB0E91E513664C162FFEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <ba58f299-d50b-f347-4a0f-778cb6ac5588@enterprisedb.com>
 <TYAPR01MB2990A053D902EDF6244090BDFEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <CA+HiwqH-7+L0B7iNWcAe8auWch=-EnbRkLYMG-27K4rUKsWKOQ@mail.gmail.com>
 <TYAPR01MB29903CBD68C9DA42B67D9EB6FEA30@TYAPR01MB2990.jpnprd01.prod.outlook.com>
 <CA+HiwqGiwxSc8kc0PgPef5DgnP7NTRHVyAZJG3zxY4XBivbfLQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <9d94c863-ce7c-ea7f-6b84-834c98929e2f@enterprisedb.com>
Date: Tue, 19 Jan 2021 17:01:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: 
 <CA+HiwqGiwxSc8kc0PgPef5DgnP7NTRHVyAZJG3zxY4XBivbfLQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/19/21 7:23 AM, Amit Langote wrote:
> On Tue, Jan 19, 2021 at 2:06 PM tsunakawa.takay@fujitsu.com
> <tsunakawa.takay@fujitsu.com> wrote:
>> From: Amit Langote <amitlangote09@gmail.com>
>>> I apologize in advance for being maybe overly pedantic, but I noticed
>>> that, in ExecInitModifyTable(), you decided to place the call outside
>>> the loop that goes over resultRelations (shown below), although my
>>> intent was to ask to place it next to the BeginForeignModify() in that
>>> loop.
>>
>> Actually, I tried to do it (adding the GetModifyBatchSize() call after BeginForeignModify()), but it failed.  Because postgresfdwGetModifyBatchSize() wants to know if RETURNING is specified, and ResultRelInfo->projectReturning is created after the above part.  Considering the context where GetModifyBatchSize() implementations may want to know the environment, I placed the call as late as possible in the initialization phase.  As for the future(?) multi-target DML statements, I think we can change this together with other many(?) parts that assume a single target table.
> 
> Okay, sometime later then.
> 
> I wasn't sure if bringing it up here would be appropriate, but there's
> a patch by me to refactor ModfiyTable result relation allocation that
> will have to remember to move this code along to an appropriate place
> [1].  Thanks for the tip about the dependency on how RETURNING is
> handled.  I will remember it when rebasing my patch over this.
> 

Thanks. The last version (v12) should be addressing all the comments and 
seems fine to me, so barring objections I'll get that pushed shortly.

One thing that seems a bit annoying is that with the partitioned table 
the explain (verbose) looks like this:

                      QUERY PLAN
-----------------------------------------------------
  Insert on public.batch_table
    ->  Function Scan on pg_catalog.generate_series i
          Output: i.i
          Function Call: generate_series(1, 66)
(4 rows)

That is, there's no information about the batch size :-( But AFAICS 
that's due to how explain shows (or rather does not) partitions in this 
type of plan.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company