Thanks Fabien for the comments.
On 2020-11-25 11:29 p.m., Fabien COELHO wrote:
>
> Hello David,
>
>> The previous patch was based on branch "REL_13_STABLE". Now, the
>> attached new patch v2 is based on master branch. I followed the new
>> code structure using appendPQExpBuffer to append the new clause
>> "using TABLEAM" in a proper position and tested. In the meantime, I
>> also updated the pgbench.sqml file to reflect the changes.
>
> My 0.02€: I'm fine with the added feature.
>
> The patch lacks minimal coverage test. Consider adding something to
> pgbench tap tests, including failures (ie non existing method).
I added 3 test cases to the tap tests, but not sure if they are
following the rules. One question about the failures test, my thoughts
is that it should be in the no_server test cases, but it is hard to
verify the input as the table access method can be any name, such as
zheap, zedstore or zheap2 etc. Any suggestion will be great.
>
> The option in the help string is not at the right ab place.
Fixed in v3 patch.
>
> I would merge the tableam declaration to the previous one with a
> extended comments, eg "tablespace and access method selection".
Updated in v3 patch.
>
> escape_tableam -> escaped_tableam ?
Updated in v3 patch.
By the way, I saw the same style for other variables, such as
escape_tablespace, should this be fixed as well?
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
