Re: POC: postgres_fdw insert batching - Mailing list pgsql-hackers
From | Tomas Vondra |
---|---|
Subject | Re: POC: postgres_fdw insert batching |
Date | |
Msg-id | ebbd67c6-054a-f5eb-39a9-95e088f1c985@enterprisedb.com Whole thread Raw |
In response to | Re: POC: postgres_fdw insert batching (Amit Langote <amitlangote09@gmail.com>) |
Responses |
RE: POC: postgres_fdw insert batching
("tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>)
|
List | pgsql-hackers |
On 1/21/21 2:53 AM, Amit Langote wrote: > On Thu, Jan 21, 2021 at 10:42 AM Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> On 1/21/21 2:24 AM, Amit Langote wrote: >>> On Thu, Jan 21, 2021 at 9:56 AM Tomas Vondra >>> <tomas.vondra@enterprisedb.com> wrote: >>>> On 1/21/21 1:17 AM, Zhihong Yu wrote: >>>>> Hi, >>>>> The assignment to resultRelInfo is done when junk_filter_needed is true: >>>>> >>>>> if (junk_filter_needed) >>>>> { >>>>> resultRelInfo = mtstate->resultRelInfo; >>>>> >>>>> Should the code for determining batch size access mtstate->resultRelInfo >>>>> directly ? >>>>> >>>> >>>> IMO the issue is that code iterates over all plans and moves to the next >>>> for each one: >>>> >>>> resultRelInfo++; >>>> >>>> so it ends up pointing past the last element, hence the failures. So >>>> yeah, either the code needs to move before the loop (per my patch), or >>>> we need to access mtstate->resultRelInfo directly. >>> >>> Accessing mtstate->resultRelInfo directly would do. The only >>> constraint on where this block should be placed is that >>> ri_projectReturning must be valid as of calling >>> GetForeignModifyBatchSize(), as Tsunakawa-san pointed out upthread. >>> So, after this block in ExecInitModifyTable: >>> >>> /* >>> * Initialize RETURNING projections if needed. >>> */ >>> if (node->returningLists) >>> { >>> .... >>> /* >>> * Build a projection for each result rel. >>> */ >>> resultRelInfo = mtstate->resultRelInfo; >>> foreach(l, node->returningLists) >>> { >>> List *rlist = (List *) lfirst(l); >>> >>> resultRelInfo->ri_returningList = rlist; >>> resultRelInfo->ri_projectReturning = >>> ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps, >>> resultRelInfo->ri_RelationDesc->rd_att); >>> resultRelInfo++; >>> } >>> } >>> >> >> Right. But I think Tom is right this should initialize ri_BatchSize for >> all the resultRelInfo elements, not just the first one. Per the attached >> patch, which resolves the issue both on x86_64 and armv7l for me. > > +1 in general. To avoid looping uselessly in the case of > UPDATE/DELETE where batching can't be used today, I'd suggest putting > if (operation == CMD_INSERT) around the loop. > Right, that's pretty much what I ended up doing (without the CMD_INSERT check it'd add batching info to explain for updates too, for example). I'll do a bit more testing on the attached patch, but I think that's the right fix to push. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
pgsql-hackers by date: