Re: Fdw batch insert error out when set batch_size > 65535

On Wed, May 26, 2021 at 6:36 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> On 5/26/21 8:57 AM, Bharath Rupireddy wrote:
> > On Tue, May 25, 2021 at 2:47 PM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>
> >> On Tue, May 25, 2021 at 1:08 PM houzj(dot)fnst(at)fujitsu(dot)com
> >> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >>> Thanks for the comments. I have addressed all comments to the v3 patch.
> >>
> >> Thanks! The patch basically looks good to me except that it is missing
> >> a commit message. I think it can be added now.
> >
> > With v3 patch, I observed failure in postgres_fdw test cases with
> > insert query in prepared statements. Root cause is that in
> > postgresGetForeignModifyBatchSize, fmstate can be null (see the
> > existing code which handles for fmstate null cases). I fixed this, and
> > added a commit message. PSA v4 patch.
> >
>
> Thanks. In what situation is the fmstate NULL? If it is NULL, the
> current code simply skips the line adjusting it. Doesn't that mean we
> may not actually fix the bug in that case?

fmstate i.e. resultRelInfo->ri_FdwState is NULL for EXPLAIN without
ANALYZE cases, below comment says it and we can't get the bug because
we don't actually execute the insert statement. The bug occurs on the
remote server when the insert query with those many query parameters
is submitted to the remote server. I'm not sure if there are any other
cases where it can be NULL.
/*
* In EXPLAIN without ANALYZE, ri_fdwstate is NULL, so we have to lookup
* the option directly in server/table options. Otherwise just use the
* value we determined earlier.
*/
if (fmstate)
batch_size = fmstate->batch_size;
else
batch_size = get_batch_size_option(resultRelInfo->ri_RelationDesc);

> Also, I think it'd be keep the existing comment, probably as the first
> line of the new comment block.

Do you mean to say we need to retain "/* Otherwise use the batch size
specified for server/table. */"? If so, PSA v5.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com