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

On 5/31/21 6:01 AM, Bharath Rupireddy wrote:
> On Mon, May 31, 2021 at 1:21 AM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> Hi,
>>
>> I took at this patch today. I did some minor changes, mostly:
>>
>> 1) change the code limiting batch_size from
>>
>> if (fmstate->p_nums > 0 &&
>> (batch_size * fmstate->p_nums > PQ_QUERY_PARAM_MAX_LIMIT))
>> {
>> batch_size = PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums;
>> }
>>
>> to
>>
>> if (fmstate && fmstate->p_nums > 0)
>> batch_size = Min(batch_size,
>> PQ_QUERY_PARAM_MAX_LIMIT / fmstate->p_nums);
>>
>> which I think is somewhat clearer / more common patter.
>
> Agree, that looks pretty good.
>
>> 2) I've reworded the docs a bit, splitting the single para into two. I
>> think this makes it clearer.
>
> LGTM, except one thing that the batch_size description says "should
> insert in", but it's not that the value entered for batch_size is
> always honoured right? Because this patch might it.
>
> This option specifies the number of rows
> <filename>postgres_fdw</filename>
> should insert in each insert operation. It can be specified for a
>
> So, I suggest to remove "should" and change it to:
>
> This option specifies the number of rows
> <filename>postgres_fdw</filename>
> inserts in each insert operation. It can be specified for a
>

I think the "should" indicates exactly that postgres_fdw may adjust the
batch size. Without it it sounds much more definitive, so I kept it.

>> Attached is a patch doing this. Please check the commit message etc.
>> Barring objections I'll get it committed in a couple days.
>
> One minor comment:
> In the commit message, Int16 is used
> The FE/BE protocol identifies parameters with an Int16 index, which
> limits the maximum number of parameters per query to 65535. With
>
> and in the code comments uint16 is used.
> + * parameters in a batch is limited to 64k (uint16), so make sure we don't
>
> Isn't it uint16 in the commit message too? Also, can we use 64k in the
> commit message instead of 65535?
>

No, the "Int16" refers to the FE/BE documentation, where we use Int16.
But in the C code we interpret it as uint16.

I've added a simple regression test to postgres_fdw, testing that batch
sizes > 65535 work fine, and pushed the fix.

I've considered checking the value in postgres_fdw_validator and just
rejecting anything over 65535, but I've decided against that. We'd still
need to adjust depending on number of columns anyway.

regards

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