Re: pgbench - extend initialization phase control

2019-11-06 11:31 に Fujii Masao さんは書きました:
> 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.
> 
This is a patch which does not allow space character in -I options .

Regard,
Yu Kimura