Re: pgbench - extend initialization phase control - Mailing list pgsql-hackers

On Wed, Nov 6, 2019 at 6:23 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> Hello,
>
> >>> - for (step = initialize_steps; *step != '\0'; step++)
> >>> + for (const char *step = initialize_steps; *step != '\0'; step++)
> >
> > But I still wonder why we should apply such change here.
>
> Because it removes one declaration and reduces the scope of one variable?
>
> > If there is the reason why this change is necessary here,
>
> Nope, such changes are never necessary.
>
> > I'm OK with that. But if not, basically I'd like to avoid the change.
> > Otherwise it may make the back-patch a bit harder
> > when we change the surrounding code.
>
> I think that this is small enough so that it can be managed, if any back
> patch occurs on the surrounding code, which is anyway pretty unlikely.
>
> > Attached is the slightly updated version of the patch. Based on your
> > patch, I added the descriptions about logging of "g" and "G" steps into
> > the doc, and did some cosmetic changes. Barrying any objections,
> > I'm thinking to commit this patch.
>
> I'd suggest:
>
> "to print one message each ..." -> "to print one message every ..."
>
> "to print no progress ..." -> "not to print any progress ..."
>
> I would not call "fprintf(stderr" twice in a row if I can call it once.

Thanks for the suggestion!
I updated the patch in that way and committed it!

This commit doesn't include the change "for (const char ...)"
and "merge two fprintf into one" ones that we were discussing.
Because they are trivial but I'm not sure if they are improvements
or not, yet. If they are, probably it's better to apply such changes
to all the places having the similar issues. But that seems overkill.

>
> > While reviewing the patch, I found that current code allows space
> > character to be specified in -I. That is, checkInitSteps() accepts
> > space character. Why should we do this?
>
> > Probably I understand why runInitSteps() needs to accept space character
> > (because "v" in the specified string with -I is replaced with a space
> > character when --no-vacuum option is given).
>
> Yes, that is the reason, otherwise the string would have to be shifted.
>
> > But I'm not sure why that's also necessary in checkInitSteps(). Instead,
> > we should treat a space character as invalid in checkInitSteps()?
>
> I think that it may break --no-vacuum, and I thought that there may be
> other option which remove things, eventually. Also, having a NO-OP looks
> ok to me.

As far as I read the code, checkInitSteps() checks the initialization
steps that users specified. The initialization steps string that
"v" was replaced with blank character is not given to checkInitSteps().
So ISTM that dropping the handling of blank character from
checkInitSteps() doesn't break --no-vacuum.

Regards,

-- 
Fujii Masao