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
