On 1/19/21 7:23 AM, Amit Langote wrote:
On Tue, Jan 19, 2021 at 2:06 PM tsunakawa.ta...@fujitsu.com
<tsunakawa.ta...@fujitsu.com> wrote:
From: Amit Langote <amitlangot...@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


Reply via email to