Hello Amit,
>> I would not bother to create a patch for so small an improvement. This
>> makes sense in passing because the created function makes it very easy,
>> but otherwise I'll just drop it.
>
> I would prefer to drop for now.
Attached v13 does that, I added a comment instead. I do not think that it
is an improvement.
> + else
> + {
> + fprintf(stderr, "unexpected partition method: \"%s\"\n", ps);
> + exit(1);
> + }
>
> If we can't catch that earlier, then it might be better to have some
> version-specific checks rather than such obscure code which is
> difficult to understand for others.
Hmmm. The code simply checks for the current partitioning and fails if the
result is unknown, which I understood was what you asked, the previous
version was just ignoring the result.
The likelyhood of postgres dropping support for range or hash partitions
seems unlikely.
This issue rather be raised if an older partition-enabled pgbench is run
against a newer postgres which adds a new partition method. But then I
cannot guess when a new partition method will be added, so I cannot put a
guard with a version about something in the future. Possibly, if no new
method is ever added, the code will never be triggered.
> I have made a few modifications in the attached patch.
> * move the create partitions related code into a separate function.
Why not. Not sure it is an improvement.
> * make the check related to number of partitions consistent i.e check
> partitions > 0 apart from where we print which I also want to change
> but let us first discuss one of the above points
I switched two instances of >= 1 to > 0, which had 1 instance before.
> * when we don't found pgbench_accounts table, error out instead of continuing
I do not think that it is a a good idea, but I did it anyway to move
things forward.
> * ensure append_fillfactor doesn't assume that it has to append
> fillfactor and removed fillfactor < 100 check from it.
Done, which is too bad.
> * improve the comments around query to fetch partitions
What? How?
There are already quite a few comments compared to the length of the
query.
> * improve the comments in the patch and make the code look like nearby
> code
This requirement is to fuzzy. I re-read the changes, and both code and
comments look okay to me.
> * pgindent the patch
Done.
> I think we should try to add some note or comment that why we only
> choose to partition pgbench_accounts table when the user has given
> --partitions option.
Added as a comment on the initPartition function.
--
Fabien.
