Hello, Boris
Thank you for making new productivity measurements. I`ve reviewed the v7
version and have some feedback below.
1. Perhaps we should rename functions initRowMethodBin to
initRowMethodBinary, initPopulateTableText to initPopulateTableTextCopy,
initPopulateTableBinary to initPopulateTableBinaryCopy,
initGenerateDataClientSideText to initGenerateDataClientSideTextFrmt,
initGenerateDataClientSideBinary to initGenerateDataClientSideBinaryFrmt
for better clarity.
2. The --help output currently describes only the 'M' mode. Should we
also add a description for the 'S' mode for completeness?
3. I`m wondering if the default value for the 'filler' column in
initCreateTables is necessary? The current functionality seems
unaffected, so perhaps we could avoid changing this function to keep the
diff minimal.
4. I noticed that vanilla client functionality in M mode is not
implemented. Is there a specific reason for this? It seems feasible to
implement by passing a counter, similar to how it`s done in
initPopulateTableBinary. If there are reasons not to implement it, in
mode Mg pgbench should not run mode Sg, it just pg_fatal.
5. In mode client binary format generation c, It would be the right
thing to do implement write progress of generating data and 'quiet' mode,
as it already implement in client text format generation g.
6. In bufferData, when len == 1, we call bufferCharData, which already
increments bin_copy_buffer_length. However, at the end of bufferCharData
we increment it again, leading to a double increment.
7. I suggest adding column count in function initPopulateTableBinary,
and initBranchBinary, initTellerBinary, initAccountBinary, to pass it
from initAccountBinary to init_row, and use it in functions
initBranchBinary, initTellerBinary, initAccountBinary. This will
increase the readability of the code, and remove the magic numbers in
the addColumnCounter call.
8. I think check and install data_generation_type in function
checkInitSteps is not quite right. In the current realization, pgbench
allows run data generation many times (dtCdtC...), so i suggest do not
touching this functionality. My suggestion would be to revert all the
changes from function checkInitSteps, set the data_generation_type in
switch in runInitSteps and remove call function checkInitSteps
from main.
Best regards,
Egor
