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

From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Fdw batch insert error out when set batch_size > 65535
Date: 2021-05-21 13:33:19
Message-ID: 246a2e72-e326-a94e-e8da-beb9b477c61e@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


On 5/21/21 5:03 AM, Bharath Rupireddy wrote:
> On Fri, May 21, 2021 at 1:19 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>> Attaching V2 patch. Please consider it for further review.
> Thanks for the patch. Some more comments:
>
> 1) Can fmstate->p_nums ever be 0 in postgresGetForeignModifyBatchSize?
> By any chance, if it can, I think instead of an assert, we can have
> something like below, since we are using it in the division.
> 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;
> }
> Also, please remove the { and } for the above if condition, because
> for 1 line statements we don't normally use { and }

We used to filter that out in pgindent IIRC but we don't any more.
IMNSHO there are cases when it makes the code more readable, especially
if (as here) the condition spans more than one line. I also personally
dislike having one branch of an "if" statement with braces and another
without - it looks far better to my eyes to have all or none with
braces. But I realize that's a matter of taste, and there are plenty of
examples in the code running counter to my taste here.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2021-05-21 13:46:12 Re: Installation of regress.so?
Previous Message Tom Lane 2021-05-21 13:25:02 Re: Installation of regress.so?