On 5/21/21 5:03 AM, Bharath Rupireddy wrote:
> On Fri, May 21, 2021 at 1:19 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.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