Re: Add table access method as an option to pgbench

Thanks a lot again for the review and comments.

On 2020-11-25 9:36 p.m., Michael Paquier wrote:
> On Wed, Nov 25, 2020 at 12:13:55PM -0800, David Zhang wrote:
>> 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.
> +     <varlistentry>
> +      <term><option>--table-am=<replaceable>TABLEAM</replaceable></option></term>
> +      <listitem>
> +       <para>
> +        Create all tables with specified table access method
> +        <replaceable>TABLEAM</replaceable>.
> +        If unspecified, default is <literal>heap</literal>.
> +       </para>
> +      </listitem>
> +     </varlistentry>
> This needs to be in alphabetical order.
The order issue is fixed in v3 patch.
> And what you say here is
> wrong.  The default would not be heap if default_table_access_method
> is set to something else.
Right, if user change the default settings in GUC, then the default is 
not `heap` any more.
> I would suggest to use table_access_method
> instead of TABLEAM,
All other options if values are required, the words are all capitalized, 
such as TABLESPACE. Therefore, I used TABLEACCESSMETHOD in stead of 
table_access_method in v3 patch.
> and keep the paragraph minimal, say:
> "Create tables using the specified table access method, rather than
> the default."
Updated in v3 patch.
>
> --table-am is really the best option name?  --table-access-method
> sounds a bit more consistent to me.
Updated in v3 patch.
>
> +       if (tableam != NULL)
> +       {
> +           char       *escape_tableam;
> +
> +           escape_tableam = PQescapeIdentifier(con, tableam, strlen(tableam));
> +           appendPQExpBuffer(&query, " using %s", escape_tableam);
> +           PQfreemem(escape_tableam);
> +       }
Thanks for pointing this out. The order I believe is fixed in v3 patch.
> The order is wrong here, generating an unsupported grammar, see by
> yourself with this command:
> pgbench --partition-method=hash --table-am=heap -i  --partitions 2

Tested in v3 patch, with a command like,

pgbench --partition-method=hash --table-access-method=heap -i --partitions 2

>
> This makes the patch trickier than it looks as the USING clause is
> located between PARTITION BY and WITH.  Also, partitioned tables
> cannot use the USING clause so you need to be careful that
> createPartitions() also uses the correct table AM.
>
> This stuff needs tests.
3 test cases has been added the tap test, but honestly I am not sure if 
I am following the rules. Any comments will be great.
> --
> Michael
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca