Hello,
> Please complete fixes for the documentation. At least the following sentence should be fixed:
> ```
> The last two lines report the number of transactions per second, figured with and without counting the time to start
databasesessions.
> ```
Indeed. I scanned the file but did not find other places that needed
updating.
>> -starting vacuum...end.
>
> I think any other options should be disabled in the example, therefore please leave this line.
Yes.
>> + for (int i = 0; i < nstate; i++)
>> + {
>> + state[i].state = CSTATE_CHOOSE_SCRIPT;
>> + }
>
> I'm not sure but I think braces should be removed in our coding conventions.
Not sure either. I'm not for having too many braces anyway, so I removed
them.
>> + /* GO */
>> + pthread_barrier_wait(&barrier);
>
> The current implementation is too simple. If nthreads >= 2 and connection fails in the one thread,
> the other one will wait forever.
> Some special treatments are needed in the `done` code block and here.
Indeed. I took your next patch with an added explanation. I'm unclear
whether proceeding makes much sense though, that is some thread would run
the test and other would have aborted. Hmmm.
>>> (that is, it can be disabled)
>>
>> On reflection, I'm not sure I see a use case for not running the barrier
>> if it is available.
>
> If the start point changes and there is no way to disable this feature,
> the backward compatibility will be completely violated.
> It means that tps cannot be compared to older versions under the same conditions,
> and It may hide performance-related issues.
> I think it's not good.
ISTM that there is another patch in the queue which needs barriers to
delay some initialization so as to fix a corner case bug, in which case
the behavior would be mandatory. The current submission could add an
option to skip the barrier synchronization, but I'm not enthousiastic to
add an option and remove it shortly later.
Moreover, the "compatibility" is with nothing which does not make much
sense. When testing with many threads and clients, the current
implementation make threads start when they are ready, which means that
you can have threads issuing transactions while others are not yet
connected or not even started, so that the actually measured performance
is quite awkward for a short bench. ISTM that allowing such a backward
compatible strange behavior does not serve pg users. If the user want the
old unreliable behavior, they can use a old pgbench, and obtain unreliable
figures.
For these two reasons, I'm inclined not to add an option to skip these
barriers, but this can be debatted further.
Attached 2 updated patches.
--
Fabien.
