On Fri, Sep 27, 2019 at 2:36 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2019-Sep-26, Fabien COELHO wrote:
> > > pgbench's main() is overly long already, and the new code being added
> > > seems to pollute it even more. Can we split it out into a static
> > > function that gets placed, say, just below disconnect_all() or maybe
> > > after runInitSteps?
> >
> > I agree that refactoring is a good idea, but I do not think it belongs to
> > this patch. The file is pretty long too, probably some functions could be
> > moved to distinct files (eg expression evaluation, variable management,
> > ...).
>
> I'm not suggesting to refactor anything as part of this patch -- just
> that instead of adding that new code to main(), you create a new
> function for it.
>
> > > (Also, we seem to be afraid of function prototypes. Why not move the
> > > append_fillfactor() to *below* the functions that use it?)
> >
> > Because we avoid one more line for the function prototype? I try to put
> > functions in def/use order if possible, especially for small functions like
> > this one.
>
> I can see that ... I used to do that too. But nowadays I think it's
> less messy to put important stuff first, secondary uninteresting stuff
> later. So I suggest to move that new function so that it appears below
> the code that uses it. Not a big deal anyhow.
>
Thanks, Alvaro, both seem like good suggestions to me. However, there
are a few more things where your feedback can help:
a. With new options, we will partition pgbench_accounts and the
reason is that because that is the largest table. Do we need to be
explicit about the reason in docs?
b. I am not comfortable with test modification in
001_pgbench_with_server.pl. Basically, it doesn't seem like we should
modify the existing test to use non-default tablespaces as part of
this patch. It might be a good idea in general, but I am not sure
doing as part of this patch is a good idea as there is no big value
addition with that modification as far as this patch is concerned.
OTOH, as such there is no harm in testing with non-default
tablespaces.
The other thing is that the query used in patch to fetch partition
information seems correct to me, but maybe there is a better way to
get that information.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com